Orbit in Clojure

anonymous about 12 hours later:

• • Code is hard to read since it is not using idiomatic lisp/clojure style. Please don’t put closing parens/braces on new line, since it is not readable. Use indentation to distinguish code blocks instead. If you are not suer about indentation, just use emacs clojure-mode defaults which does quite a good job at is.

  • Sorry, but this is real blocker and makes code painful to read, so I can’t comment on the code itself.

Julian Birch about 15 hours later:

• • Being a C# programmer, I don’t really have a problem with the parens. I’d probably use map and filter more in the physics code. e.g. (defn accelerate-all [os] (map accelerate os)) At that point, you probably don’t need the function at all. That’s true of pretty much all of the -all functions except collide-all.

  • Don’t know if you care about this, but I think collide-all might have an edge case: when three objects collide simultaneously, I think it goes wrong. (Sorry, no repl handy to check this…)

  • Oh yes, and I’d call “vector” vector2d. :)

Steve about 16 hours later:

• • Not a single comment, so much for clean code.

Derrick Spell about 17 hours later:

• • haha … everyone seems so testy today.

  • I’ve never looked at Clojure code before, but a cursory glance at this and I can see the basic structure. Seems like formatting is very readable. Even without a working knowledge of Clojure I can follow along without comments.

  • I look forward to digging in a little closer after work tonight!

klang about 18 hours later:

• • Leiningen support is nice to have .. it makes life easier, so I forked and added that convenience.

Chris Vest 1 day later:

• • I forked the lein support repo and took a late-night stab at the indenting. Not that I claim my indenting is idiomatic, but it’s probably closer.

Julian Birch 1 day later:

• • I couldn’t leave collide-all alone, so here’s another version: first, observe that collide? is reflexive and symmetric but not necessarily transitive. But for collisions, we want it to be transitive. So, we need a function group-by-nontransitive-relation. Then all we need to do is merge the results together.

  • A function like this should do that

  • (defn merge-all [should-merge?, merge, coll]      (map         (partial reduce merge)         (group-by-nontransitive-relation should-merge? coll)))

• and then collide-all becomes

  • (defn collide-all [coll]     (merge-all collides? collide coll))

  • Finally, I need to write the group by function. So, I can use a hashmap to do the grouping, using the key as the first element that is part of the collision and values as the objects that will collide. (We throw away the keys at the end with a call to vals.) Since we’re iterating through a list and producing a result, reduce is the way to go.

• I can’t come up with a simpler way of expressing it than this, which annoys me:

  • ; Couple of observations ; hashMap in this code is a hashmap whose values are lists  Dictionary<T, List<T>> in C# ; In particular, (second %) is the second element in the hashMap sequence, and so is a list itself ; Worst case scenario is when the item encountered matches multiple sets.  Then the sets need to be merged (defn group-by-nontransitive-relation [related?, coll]     "Returns a sequence of sequences, corresponding to the equivalence groupings specified by the non-transitive relation 'related?'."      (letfn [         (isMatch [coll, o] (some (partial related? o) coll))         (find-matching-groups [hashMap, o] (filter #(-> % second (isMatch o)) hashMap))         (add-groups-to-lookup [hashMap, o, groups]             (assoc                  (reduce dissoc hashMap (map first groups))                  (ffirst groups)                  (conj (reduce concat (map second groups)) o))) ; Merge the equivalence sets         (add-to-lookup [hashMap, o] (if-let              [matching-groups (not-empty (find-matching-groups hashMap o))]             (add-groups-to-lookup hashMap o matching-groups)                 ; add to the current key's values             (assoc hashMap o [o])))]                 ; new key with only one entry         ; Creates a hashmap of related objects keyed by the first element in the equivalence class         ; Then just throws the keys away with a call to "vals"      (vals (reduce add-to-lookup '{} coll))))

• A quick test you can run in the repl:

  • (defn is-close? [x y] (or (= x (inc y)) (= y (inc x)))) (merge-all is-close? + [1 3 5 6 10 2 12])

• This should return (6, 11, 10, 12), in no particular order.

Julian Birch 1 day later:

• • Couple of extra observations: turns out that defining the inner functions within add-to-lookup appears to be significantly faster. Of course, inlining completely is faster as well, but not to an extent you’d expect.

  • Sadly, whatever you do, the performance is bad. You can fix this by breaking the algorithm slightly: only checking the keys rather than all the values, which in turn simplifies add-groups-to-lookup. This doesn’t quite get the right answer, but it conserves mass and would probably not be noticeable in practice. (merge-all is-close? 1 3 2) would return (3 3) or (1 5) instead of (6).

@Derrick Spell

• It’s not about testiness. Research shows that “advanced programmers have strong expectations about what programs should look like, and when those expectations are violated their performance drops drastically” [Empirical studies of programming knowledge, E. Soloway and K. Ehrlich]. Needless to say they outperform novices on familiarly “constructed” codebases.