Idiomatic Clojure: Code Smells

Sometimes you need to concat the results of mapping. Using mapcat is an idiomatic option for this case:

;;; Don't                       Do
(apply concat (map f xs))  =>  (mapcat f xs)

Avoid trivial for (list comprehensions)

;;; Don't                Do
(for [x xs] (f x))  =>  (map f xs)

And yes, I've seen a combination of both:

;;; Please Don't
(apply concat (for [x xs] (f x)))

There's no reason to wrap a function in an anonymous function:

;;; Don't             Do
(map #(f %) xs)  =>  (map f xs)

If you find yourself scanning collections of maps looking for a map where a certain key has a certain value, your collection might be telling you it wants to be associative, not sequential:

(def people
  [{:person/name "Fred"}
   {:person/name "Ethel"}
   {:person/name "Lucy"}])

(defn person-in-people?
  [person people]
  (some #(= person (:person/name %)) people))

(person-in-people? "Fred" people);; => true
(person-in-people? "Bob" people) ;; => nil

Instead, index the collection according to the parameter which interests you.

With group-by:

(def collected-people
  (group-by :person/name people))

(contains? collected-people "Fred");; => true
(contains? collected-people "Bob") ;; => false

With clojure.set/index:

(def collected-people
  (clojure.set/index people [:person/name]))

(contains? collected-people {:person/name "Fred"});; => true
(contains? collected-people {:person/name "Bob"}) ;; => false

This code is readable: "Does collected-people contain a person who's name is Bob?".

An important distinction between group-by and index is that index's behavior is undefined when not dealing with sets. (Thanks to Jozef Wagner for pointing this out). If you're not working with sets, you might lose information!

If you repeatedly filter according to certain attributes, you might want to index according to them, too, with group-by or clojure.set/index.

An important distinction between group-by and index, is that group-by indexes according to a function, so you can presumably bin values as well, for example, by age or height, while index works specifically with keys.

;;; Don't
(into {} (map (fn [[k v]] [k (f v)]) m))
(into {} (for [[k v] m] [k (f v)]))
;;; Do
(reduce-kv (fn [m k v] (assoc m k (f v))) {} m)
;;; Or faster* but less pretty
(persistent!
 (reduce-kv (fn [m k v] (assoc! m k (f v))) (transient {}) m))

Don't write it out manually every time. Turn it into a function and throw it into one of half-dozen util namespaces.

NOTE: the benefits from the transient version increase with the size of the input. For very small maps, the non-transient version is faster.

Clojure maps are collections, not slots. Combined with nil's meaning being "nothing", nil values inside maps are confusing:

(get {:a nil} :a) ;; => nil
(get {:b 111} :a) ;; => nil

Try to avoid inserting nil values into a map.

If it is not a map you control, you can always prune it.

Both implementations are valid:

(defn assoc-some [m k v] (if (nil? v) m (assoc m k v)))
(defn prune-nils [m] (reduce-kv assoc-some {} m))

(prune-nils {:a 1 :b nil :c 2 :d nil}) ;; => {:a 1, :c 2}

(defn remove-nil [m k v] (if (nil? v) (dissoc m k) m))
(defn prune-nils [m] (reduce-kv remove-nil m m))

(prune-nils {:a 1 :b nil :c 2 :d nil}) ;; => {:a 1, :c 2}

merge is clear and easy to work with, but has terrible performance.

If you're more interested in the performance side of things, watch this talk.

Keep in mind while reading the following sections that using merge and select-keys comes at a price. While the general advice is to avoid doing things manually, every rule has an exception.

Be aware there are alternatives as well, but merge still performs poorly in general.

Thanks to Borkdude for highlighting this issue quickly.

The below piece of code conditionally merges m2 into m1 when the values in m2 are not nil:

(let [m1 {}
      m1 (if (nil? (:k m2)) m1 (assoc m1 :k (:k m2)))])

Imagine it being done for every key in m2. There can be 20 Keys. Who can even make sense of the important parts of the code afterwards?

Instead, combine nil pruning with merge, as two steps of understandable data transformations:

(merge m1 (prune-nils m2))

I see this usually going hand-in-hand with manual merges:

{:a (:a m1)
 :b (:b m2)}

Again, this can usually involve pretty big maps. Instead, try to:

(merge (select-keys m1 [:a])
       (select-keys m2 [:b]))

I often see this pattern repeating itself:

(defn foo
  [in]
  (let [m {:k0 (f0 in)} ;; mandatory
        m (if (p1 in) (assoc m :k1 (f1 in)) m) ;; optional
        m (if (p2 in) (assoc m :k2 (f2 in)) m)]
    m))

Instead, use cond->:

(defn foo
  [in]
  (cond-> {:k0 (f0 in)} ;; mandatory
    (p1 in) (assoc :k1 (f1 in)) ;; optional
    (p2 in) (assoc :k2 (f2 in))))

This way, flow control turns into syntax and there's no state to keep track of.

;;; Don't     Do
(= 0 x)  =>  (zero? x)
(> x 0)  =>  (pos? x)
(< x 0)  =>  (neg? x)

Small warning regarding zero?: It expects its argument to be a number and will throw otherwise. Checking for equality to 0 is more permissive.

Some expressions have implicit do blocks in them, making it unnecessary to use a do block.

Avoid trivial threading:

(-> x f)     => (f x)
(-> x (f a)) => (f x a)

And remember to thread with style:

• -> is for collections
• ->> is for sequences

Plenty of macros with binding forms don't need to be nested:

;;; Don't
(let [x 1]
  (let [y 2]
    [x y]))

;; Do
(let [x 1
      y 2]
  [x y])

A special case is that of nested iteration:

;;; Don't
(doseq [x xs]
  (doseq [y ys]
    (println x y)))

;;; Do
(doseq [x xs
        y ys]
  (println x y))

We can sometimes find the following form in code, of do-one-thing then do-one-thing-n-times:

(defn build-person [x] ...)
(defn build-persons [xs] (map build-person xs))

It may not be immediately apparent why it's a code smell.

• It ties implementation to concretion. Now the entire code will be in the context of a sequence of things and not thinking about one thing at a time. We have to mentally jump back and forth between the collection of things and one thing.
• Fragile: we might end up adding more logic to build-persons, which makes us nominative liars, since the name doesn't match what it does anymore. This change now won't be reflected across the system by anyone using only the singular function.
• Composes badly: had we wanted to compose several singular functions, we could have just used a threading macro or comp and be done with it. To compose for a sequential use case we could have used transducers. Now we are tied to a sequential concretion.
• Less reusable: due to all of the above, once a singular function has been dragged to the plural context, we decrease the chances of being able to reuse the singular from one side, and tie the plural down with further details which prevent usage in other contexts, or would necessitate ugly hacks, like mapping it on a sequence with one element.