Fork me on GitHub
#code-reviews
<
2016-08-22
>
quoll17:08:43

I do like the composition in the grid definition from @amacdougall. On the other hand, it takes the approach of, “make this in a sequence, and then convert the sequences to vectors”. That’s a great approach, but I also wonder about doing it directly:

(defn grid [width height]
  (mapv (fn [y]
          (mapv (fn [x] (create-cell x y)) (range width)))
    (range height)))
Of course, under the covers mapv is just (into [] (map …)), so there’s no actual benefit here. It just “feels” like fewer steps to me 🙂

slipset17:08:40

Just to "well, actually" you 🙂 mapv is a bit more optimized when dealing with a single collection:

([f coll]
     (-> (reduce (fn [v o] (conj! v (f o))) (transient []) coll)
         persistent!))

amacdougall17:08:53

@quoll: the double mapv is what I started with, but the syntax just looked clumsy. I may end up doing that anyway if I really run into some kind of performance issues, but I don't expect any of my grids to be larger than 100 x 100 in practice. 10k cells is peanuts.

quoll18:08:31

@slipset: I’m not sure what you’re getting at here. The calls to create-cell are done lazily as the calls to mapv do their work, and each mapv is making a single vector (which is required in the final output), each one of which gets populated as a transient

quoll18:08:43

@amacdougall: I agree that it looks clumsy. The single for seemed more elegant… but having to massage it into vectors seemed clumsy as well. I figured I’d present the alternative, though I should have been clearer in stating that I don’t believe it’s better in any way.

quoll18:08:48

your original use of for feeding into partition means that the create-cell calls are all primed to be called lazily. Running that into (mapv (partial into [])) means that it ends up with the same number of transient construct/populate calls.

slipset18:08:54

The code I posted was part of the mapv implementation, which shows that for a single collection, mapv is probably more performing than (into []...

quoll18:08:29

Indeed… now that I think about it, since my nested mapv/mapv ends up with the same number of transient/into[]/persistent! calls as the original grid (from @amacdougall) and the fact that code is operating at a higher level of composition… I withdraw the suggestion of a nested mapv. It does the same thing, but it’s at a lower conceptual level.

quoll18:08:25

@slipset: have a look at the source for into. It does the same thing

slipset18:08:50

Yes, but only if to implements IEditableCollection which I assumed [] did not?

slipset18:08:33

But I’m running into cloelessness here...

quoll18:08:44

=> (instance? clojure.lang.IEditableCollection [])
true

slipset18:08:03

I stand corrected 🙂

slipset18:08:58

Cool, I read IEditableCollection as ‘a collection that is editable’, but the definition, https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/IEditableCollection.java, just specifies that the collection need to have a asTransient function