Fork me on GitHub
#code-reviews
<
2018-08-10
>
jaihindhreddy-duplicate02:08:33

Not lucky enough so far to use Clojure in a professional setting but I've been messing around with it lately (watched a lot of talks, reading SICP & Joy of Clojure), came across this in reddit: https://www.reddit.com/r/Clojure/comments/2i1dem/help_me_make_this_fn_betterprettier/ One of my pastimes is to spend yak-shaving levels of time on a piece of code trying to make it as elegant as possible.

jaihindhreddy-duplicate02:08:42

I know it's a bit long but any criticism is greatly appreciated 🙂

seancorfield02:08:56

My first thought is that it's not idiomatic to map println over a sequence. Instead of (dorun (map println coll)) instead do (run! println coll)

noisesmith02:08:19

is there a standard way to run transducers for side effects though?

noisesmith02:08:05

(you can do it with transduce, but that means setting up the accumulation machinery with nils)

seancorfield02:08:00

I don't find seen/`append` to be intuitive or readable. I think they over-complicate things

seancorfield02:08:18

My feeling is that something like this is clearer:

(defn say-hello
  [names]
  (->> names
       (mapv (greeter))
       (conj "Goodbye")
       (run! println)))
where greeter is a function that returns a closure over an empty set of "seen" names...

seancorfield02:08:00

(defn greeter
  []
  (let [seen (atom #{})]
    (fn [a-name]
      ...)))

noisesmith02:08:09

I do see the appeal of using chained transducers for chained operations though, instead of just nesting calls

jaihindhreddy-duplicate02:08:29

@seancorfield That's nice! Today I Learned of mapv.

jaihindhreddy-duplicate02:08:40

Didn't know there was a map without the laziness

seancorfield02:08:49

Given this is all about side-effects (printing), I would have just replaced the original recursive solution with a doseq, followed by an imperative (println "Goodbye") with no conditionals (except on the "seen" part, which could be a local atom in the say-hello function...

jaihindhreddy-duplicate02:08:53

I keep forgetting map can take multiple collections, then trying to find a zip function then rediscover this every once in a while. Variadic arities are beautiful

seancorfield02:08:19

(defn say-hello
  [names]
  (let [seen (atom #{})]
    (doseq [n names]
      (if (@seen n)
        (println "Welcome back" n "!")
        (do
          (swap! seen conj n)
          (println "Hello" n "!"))))
    (println "Goodbye")))
(edited to add @)

seancorfield02:08:28

(untested but, hey)

jaihindhreddy-duplicate02:08:14

Works. Unceremonious and crisp

jaihindhreddy-duplicate02:08:54

Huh... worked without the deref

seancorfield02:08:42

It didn't work for me without the deref. Works with it.

seancorfield02:08:01

(if (seen n) ...) would fail since an atom is not a fn

noisesmith02:08:03

if you use a ref instead, it does work without a deref though

noisesmith02:08:24

but then you need to transact etc.

seancorfield02:08:42

Ah. I have never used ref -- never needed to coordinate multiple stateful vars.

noisesmith02:08:03

user=> ((ref #{:a}) :a)
:a