Taking Uncle Bob to school

Preface

Uncle Bob is a man shrouded in mystery, a man of opinions and contradictions - a developer and agile enthusiast. I hadn't heard about Uncle Bob until recently when he popped up on my radar as he was giving a talk about Clojure in Amsterdam. I asked around and learned that he wrote a book called "Clean code" and that he likes Ruby, which just makes no sense to me at all - How do you unify a tool as unstable, unpredictable, untamed as Ruby (second only to Perl) with Clean Code? Beats me, but maybe I should read his book.

Recently I saw that Uncle Bob had written an Orbit Simulator, so I figured I'd read through it point out a few areas where the code could be more idiomatic, to help out Bob and other Ruby developers looking to take on Clojure.

Orbital Overview

The Orbit Simulator is found here. Its very easy to run as Uncle Bob has made the package compilable to uberjar and thereby executable. Basically it generates about 500 objects all located at random positions around the sun and then simultates their orbit around the sun, checking for collisions, applying forces, that kind of thing. This particular simulation performs awfully because from top to bottom its expressed using functional sequences, which is great. You can optimize this a bit, but not a lot, without loosing the purely functional but for a more performant version you'd have to rewrite the core to something a little less functional, which would defeat the purpose of the demo.

Idiomatic Lisp

There are many things specific to Clojure that I want to run through, but before we get to that we need to examine the very basics: Our Lisp heritage. As the fantastic Mr. Fogus talked about in his blogpost on Community Standards, it's important not to alienate yourself from the community around you by inventing new odd personlized formats for your code. It will undoubtedly mean that a great deal of Clojurians will have a hard time looking at your code. Notice how Mr. Fogus uses his own brand of comma/period placement in that blogpost, that's how silly he thinks homemade Lisp formats are and I agree.

Uncle Bob has unknowingly put himself on Mr. Fogus bad side, because he writes Lisp like so:

(defn random-position [sun-position]
  (let [
        r (+ (rand 300) 30)
        theta (rand (* 2 Math/PI))
        ]
    (position/add sun-position (position/make (* r (Math/cos theta)) (* r (Math/sin theta))))
    )
  )

Now to the untrained eye I dont know what that looks like. But to an eye trained on Lisp, it looks awful. Here's the idiomatic version:

(defn random-position [sun-position]
  (let [r     (+ (rand 300) 30)
        theta (rand (* 2 Math/PI))]
    (position/add sun-position (position/make (* r (Math/cos theta)) (* r (Math/sin theta))))))

Thats the kind of mistake that is easy to make when you take on a new language, however I've been kind of enough to clean up every instance in his codebase :)

Namespaces

Lets have a look at namespace declarations. In Clojure we declare namespaces using the very versatile ns macro. Here's Uncle Bobs code:

(ns orbit.world
  (:import (java.awt Color Dimension)
    (javax.swing JPanel JFrame Timer JOptionPane)
    (java.awt.event ActionListener KeyListener))
  (:use clojure.contrib.import-static))

(import-static java.awt.event.KeyEvent VK_LEFT VK_RIGHT VK_UP VK_DOWN)
(require ['physics.object :as 'object])
(require ['physics.vector :as 'vector])
(require ['physics.position :as 'position])

Although you see the ns macro doing both imports and uses here, it actually does require as well so here's a more idiomatic version:

(ns orbit.world
  (:import (java.awt Color Dimension)
           (javax.swing JPanel JFrame Timer JOptionPane)
           (java.awt.event ActionListener KeyListener))
  (:use clojure.contrib.import-static)
  (:require [physics.object   :as object]
            [physics.vector   :as vector]
            [physics.position :as position]))

(import-static java.awt.event.KeyEvent VK_LEFT VK_RIGHT VK_UP VK_DOWN)

Concurrency Semantics

The code does a good job picking which places are immutable and which can mutate and be shared between threads. Particularily the world itself is a reference type as it must be exposed to the renderer. Unfortunately Uncle Bob decided to use the STM (Refs) for this job, which is a sub-optimal choice and here's why:

Synchronous coordinated change: Refs

Refs are great for when you need coordinated change, ie. the ability to look at two things at once, coordinating their change of state. When you need coordination there's no way around refs, but when you don't you should really find another way as the STM adds quite a bit of overhead to every transaction.

Synchronous uncoordinated change: Atoms

For synchronous uncoordinated change, atoms are your best pick. They're fast, they automatically respin if need be and they enforce all of the concurrency safety switches you need to forget about race conditions. For simply exposing the world to the renderer, atoms will do just fine.

Unsynchronous uncoordinated change: Agents

When you want something to go off, into another thread and go about its business and you don't care when or how it happens and require no coordination, agents are you best friend. Changes are still atomic, however not instant from the view of the caller.

Rewriting for atoms

Atoms are only slightly different in use from the STM. They use swap! instead of alter and don't require the surrounding dosync. Lets start with Uncle Bobs code:

(defn update-world [world controls]
  (dosync
   (alter world #(object/update-all %))))

The first thing to notice, is that controls is being passed as an argument, which it doesn't need to be. I assume thats legacy code. Secondly, from the docs it's clear that alter takes a ref as its first argument and a function as its second. But what is not being leveraged in this piece of code is that you can add an arbitrary amount of parameters to that function, like:

(defn update-world [world controls]
  (dosync
   (alter world object/update-all)))

Rewriting that to atoms looks like so:

(defn update-world [world controls]
  (swap! world object/update-all))

Here's another example which makes it clear how much is gained by using the functions idiomatically

(defn magnify [factor controls world]
  (dosync
   (let [sun-position (:position (find-sun @world))
         new-mag (* factor (:magnification @controls))]
     (alter controls #(assoc % :magnification new-mag))
     (alter controls #(assoc % :center sun-position))
     (alter controls #(assoc % :clear true)))))

Written idiomatically, also using atoms looks like:

(defn magnify [factor controls world]
  (dosync
   (let [sun-position (:position (find-sun @world))
         new-mag      (* factor (:magnification @controls))]
     (swap! controls assoc
            :magnification new-mag
            :center        sun-position
            :clear         true))))

Looks better, performs better and is more idiomatic.

Idiomatic Conditionals

There are 3 main ways of doing conditionals in Clojure: cond, condp and case. The Orbit Sim uses cond rather heavily and in place where condp makes alot more sense. condp is like cond, but makes the code more readable when the predicate is the same for all dispatch routes. Here's the original code:

(defn color-by-mass [{m :mass}]
  (cond
   (< m 1)  Color/black
   (< m 2)  (Color. 210 105 30)
   (< m 5)  Color/red
   (< m 10) (Color. 107 142 35)
   (< m 20) Color/magenta
   (< m 40) Color/blue
   :else (Color. 255 215 0)))

Here we see, that all predicates have something to do with m being smaller than something else. In condp we have to swap the position of the arguments, but that still gives us cleaner code. Notice the implicit :else at the bottom:

(defn color-by-mass [{m :mass}]
  (condp > m
    1  Color/black
    2  (Color. 210 105 30)
    5  Color/red
    10 (Color. 107 142 35)
    20 Color/magenta
    40 Color/blue
    (Color. 255 215 0)))

In this case, the win is decent, but in the next example it's huge:

(defn- quit-key? [c]
  (= \q c))

(defn- plus-key? [c]
  (or (= \+ c) (= \= c)))

(defn- minus-key? [c]
  (or (= \- c) (= \_ c)))

(defn- space-key? [c]
  (= \space c))

(defn- trail-key? [c]
  (= \t c))

(defn handle-key [c world controls]
  (cond
    (quit-key? c) (System/exit 1)
    (plus-key? c) (magnify 1.1 controls world)
    (minus-key? c) (magnify 0.9 controls world)
    (space-key? c) (magnify 1.0 controls world)
    (trail-key? c) (toggle-trail controls)))

Style-wise I think Uncle Bob prefers to err on the verbose size, which is good in terms of clarity - And indeed his code is very easy to follow. However in this case I think he took it took far. My rewrite would look like this:

(defn handle-key [c world controls]
  (condp = c
    \q     (System/exit 1)
    \+     (magnify 1.1 controls world)
    \-     (magnify 0.9 controls world)
    \space (magnify 1.0 controls world)
    \t     (toggle-trail controls)))

That saves us about 15 lines of code and sacrifices little or no clarity. In the above example all the test-constants are literals so you could re-write this using case instead of condp and that would give you constant time dispatch. Here's the faster version which also has an implicit else value (nil), for when the user presses an unmapped key. If you dont have this, you'll see exceptions.

(defn handle-key [c world controls]
  (case c
    \q     (System/exit 1)
    \+     (magnify 1.1 controls world)
    \-     (magnify 0.9 controls world)
    \space (magnify 1.0 controls world)
    \t     (toggle-trail controls)
    nil))

Being a little minimalistic

There are two reasons that the code is so slow. The first is that fact that it allocates an insane amount of objects. For the collision test Uncle Bob allocates all possible combinations of the 500 objects on every single frame, which takes ages (1 second) - And later the GC has to come clean this up again. The second reason, is that the code is doing a gazillion lookups since everything is put in structs. For the most part, this makes a lot of sense in terms of readability, but in two cases I think it goes too far: position.clj and vector.clj. These 2 files do pretty much the same thing, which is operations on coordinates. Most of us understand [x y] and dont need {:x x, :y y} right? So if I was writing something like that, I'd keep it slim when dealing with coordinates.

This simplifies a lot of things like:

(defn origin? [p]
  (and
    (zero? (:x p))
    (zero? (:y p))))

Which then simply becomes:

(defn origin? [p]
  (every? zero? p))

Or

(defn add [p q]
  (struct position
    (+ (:x p) (:x q))
    (+ (:y p) (:y q))))

Which becomes:

(defn add [[x1 y1] [x2 y2]]
  [(+ x1 x2) (+ y1 y2)])

And finally, some of the functions in vector are generating anonymous functions everytime they're called, which is also quite a slow-down. So to speed things up a little change

(defn distance [p q]
  (letfn [(square [x] (* x x))]
    (Math/sqrt
      (+
        (square (- (:x p) (:x q)))
        (square (- (:y p) (:y q)))))))

into

(defn distance [[x1 y1] [x2 y2]]
  (Math/sqrt
   (+
    (Math/pow (- x1 x2) 2)
    (Math/pow (- y1 y2) 2))))

And finally, be dynamic!

Whenever you're writing code in Clojure make sure you have a good interactive development setup. Coding, compiling to uberjar, running is not a good idea. Coding, compiling, restarting repl is not good. It makes sense to be able to restart the program at will, cleaning up memory, re-evaluating functions while running, never even closing the JPanel. I think Uncle Bobs code was not intended for further development, but rather presentation so I won't use it as an example, but a real time-saver is to also ensure a good interactive setup.

Conclusion

Uncle Bob - We're glad to have you aboard, I hope you have lots of fun with Clojure and that the tips provided here are appreciated. To the rest of you - I dont write the standards for Clojure development, but you'll find them here: Guidelines. If I've missed any core idioms in this post, please point them out.

Generally I will say, that in demonstrating Clojure the Orbit Simulator does a very good job. Usually what people struggle with initially is adapting to the functional paradigmes, which Uncle Bob seems to have gotten right in the first try! There may be a little too much use of loop (where reduce could be used instead), but generally Im very impressed with the app. Hope to see more high quality demos from you in the future! You can find my modifications: here.

About the author:

Lau Jensen is the founder and owner of Best In Class a danish consultancy company which specializes in Clojure development.

Lau is also one of the instructors driving the Conj Labs initiative. If you would like to be notified once new blogposts are published, you can follow Lau on Twitter.