code-reviews

jaihindhreddy 2018-08-10T02:25:33.000081Z

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 2018-08-10T02:25:49.000071Z

jaihindhreddy 2018-08-10T02:26:42.000066Z

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

seancorfield 2018-08-10T02:34:56.000155Z

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)

2018-08-10T02:38:19.000075Z

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

2018-08-10T02:39:05.000153Z

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

seancorfield 2018-08-10T02:40:00.000049Z

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

seancorfield 2018-08-10T02:42:18.000065Z

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

seancorfield 2018-08-10T02:44:00.000037Z

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

2018-08-10T02:44:09.000046Z

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

jaihindhreddy 2018-08-10T02:46:29.000093Z

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

jaihindhreddy 2018-08-10T02:46:40.000082Z

Didn't know there was a map without the laziness

seancorfield 2018-08-10T02:47:49.000182Z

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 2018-08-10T02:47:53.000113Z

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

seancorfield 2018-08-10T02:49:19.000132Z

(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 @)

seancorfield 2018-08-10T02:49:28.000132Z

(untested but, hey)

jaihindhreddy 2018-08-10T02:50:14.000119Z

Works. Unceremonious and crisp

jaihindhreddy 2018-08-10T02:50:54.000117Z

Huh... worked without the deref

seancorfield 2018-08-10T02:51:42.000083Z

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

seancorfield 2018-08-10T02:52:01.000020Z

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

2018-08-10T02:52:03.000043Z

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

2018-08-10T02:52:24.000097Z

but then you need to transact etc.

seancorfield 2018-08-10T02:52:42.000074Z

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

2018-08-10T02:53:03.000169Z

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

2018-08-10T02:53:18.000135Z

(Iike vars)