code-reviews

quoll 2016-08-22T17:04:43.000035Z

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 🙂

slipset 2016-08-22T17:26:40.000037Z

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!))

2016-08-22T17:39:53.000038Z

@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.

quoll 2016-08-22T18:04:31.000040Z

@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

quoll 2016-08-22T18:07:43.000041Z

@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.

quoll 2016-08-22T18:09:48.000042Z

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.

slipset 2016-08-22T18:09:54.000043Z

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

quoll 2016-08-22T18:12:29.000044Z

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.

quoll 2016-08-22T18:14:25.000045Z

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

slipset 2016-08-22T18:22:50.000046Z

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

slipset 2016-08-22T18:23:33.000047Z

But I’m running into cloelessness here...

quoll 2016-08-22T18:25:44.000048Z

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

slipset 2016-08-22T18:26:03.000050Z

I stand corrected 🙂

slipset 2016-08-22T18:27:58.000051Z

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