Clojurians Log v2

I’d love to see any feedback on making this more Clojure-ish: I want to read data from a file that may or may not have a weird, useless compression algorithm applied to it. The compression algorithm is just to compress repeating characters, and it works like this: 0xFA signals that the next three bytes are special - the first two get converted to a string, and this is (in hex) the number of times to repeat the character, and then the third byte is the character to be repeated. So if I’m reading a record that’s supposed to have 176 bytes, I want it to read it in, then expand any compressed sequences, up to 176 bytes and return a collection. Here’s what I came up with:

(declare read-bytes)
(defn inflate-compressed
  [is nbr bvec]
  (let [code (repeatedly 3 #(.read is))
        len (Integer/parseInt (->> code
                                   butlast
                                   (map char)
                                   st/join) 16)
        chr (last code)
        exp (vec (concat
                   bvec
                   (repeat len chr)))
        remain (- nbr len)]
    (fn [] (read-bytes is (if (neg? remain) 0 remain) exp))))

(defn compressed?
  [b]
  (= 250 b))

(defn read-bytes
  ([is nbr bvec]
   (if (zero? nbr) bvec
     (let [b (.read is)]
       (cond
         (= -1 b) (when-not (empty? bvec) bvec)
         (compressed? b) (fn [] (inflate-compressed is nbr bvec))
         :else
           (fn [] (read-bytes is (dec nbr) (conj bvec b)))))))
  ([is nbr]
   (trampoline read-bytes is nbr [])))

(when-not (empty? bvec) bvec) => (not-empty bvec)

I found the if in read-bytes hard to read because bvec is on the same line as the condition -- don't do that: it looks like a single-arm if and my first thought was "Use when!" then I was puzzled about it returning nil... until I saw that bvec in there!

The use of trampoline here feels weird. I would try to rewrite it without that, and just use a loop instead maybe?

Thank you! not-empty was exactly what I was looking for there. I see your point about the formatting on the if - that part began life as part of cond until I realized that I needed to check for zero and get out before I read a byte that I might potentially just lose. This whole thing actually started as a loop, and then when I added support for the “compression”, it looked like that needed its own function, and then it looked like two functions that call each other, so that’s why I went with the trampoline. But since there’s no way to exit inflate-compressed without returning a function, it’s true that it doesn’t seem to quite fit. Seems to me that a trampoline properly goes between two functions that can each either return either a function or a value. Now that I’ve got it in a form that’s working, I’ll see about how I might get that back into a loop.

Curious if about thoughts regarding improving something like this, or other alternatives to implementing a stateful iterator in clojure. I know that philosophically it could be considered at odds with the language, but if you had to do it, what’s the best way?

@ndonolli As it stands, that isn't thread-safe. Also (iter!) is not valid (I think you mean (iter! 1)?)

@seancorfield Thanks, would you mind elaborating on why it isn’t thread safe? I was playing with this code in a clojurescript context so I might not see that behavior

(def iter! (stateful-seq (range)))
(future (iter! 1)) (future (iter! 1)) ; could both return (take 1 @cursor) before either swap! occurs

That could give you 0 0 or 0 1 or even 1 0

ah gotcha

Since you want it to be atomic/thread-safe, I'd probably use split-at and use a pair in the atom.

(defn stateful-seq [s]
  (let [s (atom [nil s])]
    (fn [n]
      (first (swap! s #(split-at (if (and (number? n) (pos? n)) n 1) (second %)))))))

Seems to work.

thanks, that makes sense. Thank you @seancorfield!

you can also use the lesser known swap-vals! :

(defn stateful-seq [s]
  (let [cursor (atom s)]
    (fn
      iter!
      ([]
       (iter! 1))
      ([n]
       (let [[old _] (swap-vals! cursor #(drop n %))]
         (take n old))))))

and on principle, I think it's better to crash on an invalid input than to silently coerce the invalid input into some valid one

Is this a good way to make a reducible thing from a <http://java.io|java.io>.InputStream?

(defn reducible ^clojure.lang.IReduceInit [^java.io.InputStream is]
  (reify
    clojure.lang.IReduceInit
    (reduce [_ f init]
      (loop [state init]
        (let [b (.read is)]
          (if (= b -1)
            state
            (let [result (f state b)]
              (if (reduced? result)
                @result
                (recur result)))))))))

when you have something like an InputStream that might be holding onto system resources, you want to make it easy to manage resources. I'm not sure what making an input stream reducible buys you. check out https://juxt.pro/blog/ontheflycollections-with-reducible#_it_gets_useful_when in that example, the reducible wraps the resource and makes sure that the resources get released.

Thanks!

@smith.adriane Ah, yes, I think I like that better (and I agree about just letting it crash on a bad n value).

adding an assert or :pre condition wouldn't hurt

Also, both take and drop have reasonable behavior for negative n.

and zero!

user=> (def iter! (stateful-seq (range)))
#'user/iter!
user=> (iter! -2)
()
user=> (iter! -2)
()
user=> (iter! 1)
(0)
user=> (iter! 10)
(1 2 3 4 5 6 7 8 9 10)
user=> (iter!)
(11)
user=> 

(even for an infinite range it still works nicely)