Pure Danger Tech


Learning Clojure #13: Making my code suck less

14 Mar 2010

A couple weeks ago I wrote this tortured Clojure code to build up a string I needed:

(defn make-clauses [props]
  (letfn [
    ;; make a single clause from a property name and its index
    (make-clause [prop index]
      (str "make str with " prop " and " index ". "))

    ;; recursive function - take list like ("prop1" 0 "prop2" 1 ...) 
    ;; in remaining and accumulate output string
    (gen-clauses [remaining output]
      (if (empty? remaining)
          (rest (rest remaining)) 
          (str output (make-clause (first remaining) (second remaining))))))]
    ;; kick off the recursion by interleaving the prop list and #s
    (gen-clauses (interleave props (range (count props))) "")))

If you run this with a seq of properties, you’ll get something like this:

user=>  (make-clauses '(foo1 foo2 foo3))
"make str with foo1 and 0. make str with foo2 and 1. make str with foo3 and 2. "

The key interesting thing driving all this mess here was that I wanted to use the position of the property in props within the clause. I did this by building a list of indexes of a length matching the props with: (range (count props)). I then interleaved those indexes and properties to produce a stream like ("foo1" 0 "foo2" 1 ...). The recursion then pulled the first two items off the stream at each step, generated the clause, and accumulated the resulting string.

I was very excited as this was my first tangle with loop/recur (which is the main way to get tail recursion in Clojure). This code did work but in my gut I knew this code sucked on a couple levels. 1) I seemed to be doing a lot of work to build up an arbitrary collection when I had two perfectly good collections to start with. And 2) anytime you’re using recursion it’s best to at least consider whether you can accomplish the same thing with a sequence function without using explicit recursion.

For fun, I sent this out to my colleagues and asked them to make it better. To my joy, they quickly replied with multiple better solutions and I learned at least two important things!

Making map work harder

First, this solution introduced me to an important feature of map that I was unaware of (but I’ve since put to good use multiple times):

(defn make-clauses2 [props] 
  (apply str 
    (map #(format "make str with %s and %d. " %1 %2) 
      (range (count props)))))

This is obviously much simpler and that’s because it avoids needing to interleave the index list and recurse. It does this by leveraging the feature that map takes a function and one or more collections. If multiple collections are fed into map they are all “walked” in parallel and you can access the value of each via %1, %2, etc. You see this where map takes both props and the range output.

Off on a bit of a tangent, if you wanted to lazily produce incremented numbers for the (range (count props)) without needing to count the props (and thus realize all the elements), it occurred to me one way would be to instead do (iterate inc 0). Since that’s arguably better (by not forcing early evaluation of props) and shorter, let’s take that as an improved solution:

(defn make-clauses3 [props] 
  (apply str 
    (map #(format "make str with %s and %d. " %1 %2) 
      (iterate inc 0))))

Instead of using a trailing ” ” in the format string and using str (which leaves a trailing space on the overall completed string), we could instead use str-join to create clauses and interpose spaces just between each clause:

(defn make-clauses4 [props] 
  (str-join " "
    (map #(format "make str with %s and %d." %1 %2) 
      (iterate inc 0))))

I think this version is imminently readable and maps to the intent of the code quite well.


Another interesting alternative makes use of array-map, which I had never before used. An array-map is a simple hash map implemented in an array where the keys are the indexes. This gives you access to the index during each item without needing to grab two items from an interleaved list:

(defn make-clauses5 [props]
  (let [clauseMap (apply array-map
                         (interleave props
                         (iterate inc 0)))]
       (str-join " "
                 (for [key (keys clauseMap)]
                      (format "make str with %s and %d. "
                              (clauseMap key))))))

The let builds the array-map and the key here is that we can obtain the index based on the key using (clauseMap key). The big downside here is that this assumes the keys are unique such that they can be looked up whereas the previous examples did not need to make this assumption. But I found it interesting to learn about array-map regardless.


And of course another novel solution was to leverage the existing indexed function in seq-utils to build the index / seq pairing. This function takes a seq and returns a seq of vectors like [0 a], [1 b], etc. We can rewrite make-clauses4 with this as follows:

(defn make-clauses6 [props] 
  (str-join " "
    (for [[index prop] (indexed props)]
           (format "make str with %s and %d." prop index))))

I enjoyed discovering after the fact that the internals of indexed looked quite familiar:

(defn indexed [s]
  (map vector (iterate inc 0) s))

I hope this was a fun romp through some variants of my initial code. I’m sure someone will come up with something equally interesting in the comments…. ;)