Fork me on GitHub
#code-reviews
<
2017-08-04
>
surgo03:08:41

Could anyone help me make sure what I've done is idiomatic Clojure? I'm experienced in functional programming but not so much idiomatic Clojure style. I have a function that turns the output of a SQL query into a message board data structure (it's literally a tree). It works perfectly well, but I'd like to learn how to do it in a more Clojurian way. Snippet is here, along with example thread: https://pastebin.com/Xywtmw4z

noisesmith04:08:22

@surgo I would be tempted to use a pair of hash-maps, one to build an adjacency list representation (key is comment id, val is direct children in a vector), and one to map from id to the full comment data, then build the tree from the leaves up - I suspect that would lead to smaller code. But nothing I see there is un-idiomatic for clojure

surgo04:08:07

Alright, thanks. Mostly I was concerned by the amount of my (recur ...) usage. I kept thinking, "surely there's a function like map / reduce to do this"

noisesmith04:08:12

actually it might be the opposite for the adjacency list - child to parent, in order to build bottom up

noisesmith04:08:35

the sign there would be if you are consuming a sequence one end to the other

noisesmith04:08:05

that is, for being able to use a clojure higher order function, you want sequential input consumption, which I don't think fits your code

surgo04:08:23

ah yes, makes sense

surgo04:08:11

I mean, I'm consuming the sql results one end to the other buuut the structure it's forming isn't quite as simple

noisesmith04:08:54

right - my suggestion would start with something a lot like (let [comments (group-by :id sql-results) parents (reduce (fn [m child] (update m (:id child) (fnil conj []) (:parent child))) {} sql-results)] ...)

noisesmith04:08:28

(clearly I am too tired to be doing this, so many edits, heh

surgo04:08:51

well, I see where you're going with it anyway

surgo04:08:15

You know, it occurs to me that I can simplify it a lot more than that due to the fact that sql results already comes both in order and with depth information. Why I didn't realize that the first time around is beyond me.

noisesmith04:08:22

oh yeah - if you keep a hash-map tracking where to find each parent, each child is guaranteed to come after the parent and can be attached via an assoc-in call on an accumulator in a reduce

noisesmith04:08:20

the acc in the reduce would be something like {:node-paths {} :tree {}} where node-paths map from an existing id, to the vector you would pass to update-in to find that item in the tree

noisesmith04:08:45

so inserting would involve said update in, plus appending a new id at the end of a new entry in the node-paths

noisesmith04:08:52

anyway, there is an idiom here (which is also good programming practice) of not doing multiple linear searches on a sequential input, and instead leveraging associative data structures