Did some code review yesterday on some recent Clojure code and wanted to list one simple example of some ugly stuff we cleaned up a bit.

I had some code like this:

(defn check-prop-bridges [model]
    (let [class-maps (class-map-names model)
          prop-bridges (prop-bridge-mapping model)]
     
         (map #(bad-class-map-err (first %) (second %))
             (filter #(not (is-class-map? (second %) class-maps)) 
                 prop-bridges))))

I’m skipping lots of supporting code here but in essence the class-map-names function is returning a list of all known names and prop-bridge-mapping is returning a list of vectors in the form [prop-bridge-name class-map-name]. And the point of this code is to look through all of the mappings, find mappings where the class-map-name is unknown, and generate an error for each of those.

I was accomplishing that by first doing a filter to remove good mappings, then using map to generate an error for each remaining bad mapping.

The main thing that was smelly here was all the first and second business. 1) it’s kind of ugly and verbose, 2) there are two functions relying on the positional nature of the data returned, 3) this code is implicitly coupled to the prop-bridge-mapping function creating that intermediate data structure.

The key insight in cleaning this up a bit was to use destructuring in a list comprehension (for) to make the ordering of the passed data more explicit AND perform the filter and map in a single conceptual step:

(for [[p c] prop-bridges 
                 :when (not (is-class-map? c class-maps))]
              (bad-class-map-err p c))))

The list comprehension has the documentation:

user=> (doc for)
-------------------------
clojure.core/for
([seq-exprs body-expr])
Macro
  List comprehension. Takes a vector of one or more
   binding-form/collection-expr pairs, each followed by zero or more
   modifiers, and yields a lazy sequence of evaluations of expr.
   Collections are iterated in a nested fashion, rightmost fastest,
   and nested coll-exprs can refer to bindings created in prior
   binding-forms.  Supported modifiers are: :let [binding-form expr ...],
   :while test, :when test.

That is, something like:

(for [stooge ["Larry" "Curly" "Moe"]]
	(hit-with-pipe stooge))

can be read “for each stooge in this sequence of stooges, perform the hit-with-pipe action.” In the [ ] you see the variable and the sequence from which it is taken (and there can be multiple variables as well).

Going back to our refactored code above, we see that the variable is not just a mapping but actually [p c]. This is destructuring and actually defines the structure of the expected mapping AND binds the first and second elements to p and c.

Additionally, the filtering operation is occurring during the list comprehension using a :when modifier. The body-expr will only be evaluated when the :when modifier returns true.