code-reviews

surgo 2017-08-04T03:57:41.758097Z

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

2017-08-04T04:07:22.860329Z

@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

surgo 2017-08-04T04:08:07.868187Z

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"

2017-08-04T04:08:12.869091Z

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

2017-08-04T04:08:35.873152Z

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

2017-08-04T04:09:05.878276Z

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

surgo 2017-08-04T04:09:23.881218Z

ah yes, makes sense

surgo 2017-08-04T04:10:11.889035Z

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

2017-08-04T04:11:54.906020Z

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)] ...)

2017-08-04T04:13:28.921118Z

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

2017-08-04T04:13:30.921359Z

)

surgo 2017-08-04T04:13:51.924965Z

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

surgo 2017-08-04T04:13:54.925509Z

Thanks

surgo 2017-08-04T04:17:15.958338Z

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.

2017-08-04T04:18:22.969147Z

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

2017-08-04T04:19:20.978645Z

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

2017-08-04T04:19:45.982558Z

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

2017-08-04T04:20:52.993827Z

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

surgo 2017-08-04T04:21:11.996946Z

indeed