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

(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?".

You can also index it yourself:

(defn index
  [xs k]
  (reduce
   (fn [m x]
     (let [ik (get x k)]
       (assoc m ik (conj (get m ik #{}) x)))) {} xs))

(def collected-people
  (index people :person/name))

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

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}

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.

when

;;; Don't
(when test
  (do expr1
      expr2))

;;; Do
(when test
  expr1
  expr2)

let

;;; Don't
(let bindings
  (do expr1
      expr2))

;;; Do
(let bindings
  expr1
  expr2)

Function body

;;; Don't
(fn []
  (do expr1
      expr2))

;;; Do
(fn []
  expr1
  expr2)

More

Same goes for try , catch , and any other macro with a type signature & body .

Avoid trivial threading:

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

And remember to thread with style :

->
->>

Thread Instead Of A Deep Call Stack

;;; Don't
(defn h [x] ...)
(defn g [x] ... (h x))
(defn f [x] ... (g x))

;;; Do
(-> x f g h)

some->

;;; Don't
(when-let [x1 (f0 x0)]
  (when-let [x2 (f1 x1)]
    (when-let [x3 (f2 x2)]
      (f3 x3))))

;;; Do
(some-> x0 f0 f1 f2 f3)

;;; Don't
(let [x (if (nil? x) nil (x f0))
      x (if (nil? x) nil (x f1))
      x (if (nil? x) nil (x f2))]
  (if (nil? x) nil (f3 x)))

;;; Do
(some-> x0 f0 f1 f2 f3)

cond->

(let [x (if (p0 x) (f0 x) x)
      x (if (p1 x) (f1 x) x)]
  (if (p2 x) (f2 x) x))

(cond-> x
  (p0 x) f0
  (p1 x) f1
  (p2 x) f2)

(defmacro cond*->
  [expr & clauses]
  (assert (even? (count clauses)))
  (let [g (gensym)
        steps (map (fn [[test step]] `(if (-> ~g ~test) (-> ~g ~step) ~g))
                   (partition 2 clauses))]
    `(let [~g ~expr
           ~@(interleave (repeat g) (butlast steps))]
       ~(if (empty? steps)
          g
          (last steps)))))

(cond*-> x
         p0 f0
         p1 f1
         p2 f2)

let Shadowing

(let [x (f0 x)
      x (f1 x)
      x (f2 x)]
  (f3 x))

(-> x f0 f1 f2 f3)

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.

build-persons
comp

Avoid An Explosion In Input Arguments

(defn f
  [fizz buzz foo bar quux ohgod enough])

Prefer A map To Key-Value Rest Arguments

(defn f
  ([x y]
   (f x y :a 1 :b 2))
  ([x y & {a :a b :b}]
   ,,,))

(f x y :a 3 :b 4)

;;; vs.

(defn f
  ([x y]
   (f x y {:a 1 :b 2}))
  ([x y {a :a b :b}]
  ,,,))

(f x y {:a 3 :b 4})

Prefer Returning Maps Over Returning Positional Values

(defn sieve
  [p xs]
  [(filter p xs) (remove p xs)])

(first (sieve even? (range 9)))
;; => (0 2 4 6 8)

;;; vs.

(defn sieve
  [p xs]
  {:true (filter p xs) :false (remove p xs)})

(:true (sieve even? (range 9)))
;; => (0 2 4 6 8)