# Critique my Clojure "Game of Life" code

I'm a Clojure n00b (but am experienced with other languages) and am learning the language as I find time - enjoying it so far, though the strange dreams and accidental use of parenthesis elsewhere (e.g. Google searches) are a bit disconcerting!

Usually when learning a new language the first thing I do is implement Conway's Game of Life, since it's just complex enough to give a good sense of the language, but small enough in scope to be able to whip up in a couple of hours (most of which is spent wrestling with syntax). I did this with Scala a while back, and really appreciated the comments I received.

Anyhoo, having gone through this exercise with Clojure I was hoping to get similar feedback on my efforts so far?

I'm after any kind of feedback - algorithmic improvements, stylistic improvements, alternative APIs or language constructs, disgust at my insistence on checking argument types - whatever feedback you've got, I'm keen to hear it!

;
; Copyright (c) 2012 Peter Monks ([email protected])
;
; 3.0 Unported License. To view a copy of this license, visit
; http://creativecommons.org/licenses/by-sa/3.0/ or send a letter to
; Creative Commons, 444 Castro Street, Suite 900, Mountain View, California,
; 94041, USA.
;

(ns ClojureGameOfLife.core
(:use clojure.set))

(defrecord Cell [^Integer x ^Integer y])

(def block #{(Cell. 0 0) (Cell. 1 0)
(Cell. 0 1) (Cell. 1 1)})

(def beehive #{            (Cell. 1 0) (Cell. 2 0)
(Cell. 0 1)                         (Cell. 3 1)
(Cell. 1 2) (Cell. 2 2)})

(Cell. 0 1)
(Cell. 0 2)})

(def glider #{            (Cell. 1 0)
(Cell. 2 1)
(Cell. 0 2) (Cell. 1 2) (Cell. 2 2)})

(defn flat-map
"Maps the function to the given set, returning a new set (flattened, in the case where f itself
returns a set for each element).
Note: There has to be a better way to do this..."
[f s]
{:pre  [(set? s)]
:post [(set? %)]}
(->> s (map f) (map seq) flatten set))

(defn alive?
"Is the given cell alive in the given generation?"
[generation
cell]
{:pre  [(set?      generation)
(instance? Cell cell)]
:post [(instance? Boolean %)]}
(contains? generation cell))

(defn neighbours
"Returns all of the neighbours of a given cell."
[cell]
{:pre  [(instance? Cell cell)]
:post [(set? %)]}
(let [x (:x cell)
y (:y cell)]

#{
(Cell. (dec x) (dec y)) (Cell. x (dec y)) (Cell. (inc x) (dec y))
(Cell. (dec x) y)                         (Cell. (inc x) y)
(Cell. (dec x) (inc y)) (Cell. x (inc y)) (Cell. (inc x) (inc y))
}))

(defn number-of-living-neighbours
"Returns the number of living neighbours of the given cell in the given generation."
[generation
cell]
{:pre  [(set?      generation)
(instance? Cell cell)]
:post [(instance? Integer %)]}
(count (intersection generation (neighbours cell))))

(defn should-live?
"Should the given cell live in the next generation?"
[generation
cell]
{:pre  [(set?      generation)
(instance? Cell    cell)]
:post [(instance? Boolean %)]}
(let [alive             (alive? generation cell)
living-neighbours (number-of-living-neighbours generation cell)]
(if alive
(or (= 2 living-neighbours) (= 3 living-neighbours))
(= 3 living-neighbours))))

(defn next-generation
"Returns the next-generation board, given a generation."
[generation]
{:pre  [(set? generation)]
:post [(set? %)]}
(set (filter #(should-live? generation %) (flat-map neighbours generation))))

(defn values
"The values of the given generation's given dimension."
[generation
elem]
{:pre [(set? generation)]
:post [(set? %)]}
(set (map elem generation)))

(defn x-values
"The X values of the given generation."
[generation]
{:pre [(set? generation)]
:post [(set? %)]}
(values generation :x))

(defn y-values
"The Y values of the given generation."
[generation]
{:pre [(set? generation)]
:post [(set? %)]}
(values generation :y))

(defn min-x
"Minimum X value of a generation."
[generation]
{:pre [(set? generation)]}
(reduce min (x-values generation)))

(defn min-y
"Minimum Y value of a generation."
[generation]
{:pre [(set? generation)]}
(reduce min (y-values generation)))

(defn max-x
"Maximum X value of a generation."
[generation]
{:pre [(set? generation)]}
(reduce max (x-values generation)))

(defn max-y
"Maximum Y value of a generation."
[generation]
{:pre [(set? generation)]}
(reduce max (y-values generation)))

(defn extend-by-1
"Extends a collection of numbers by 1 in each direction (both up and down)."
[c]
(let [minimum (reduce min c)
maximum (reduce max c)]
(conj c (dec minimum) (inc maximum))))

(defn print-generation
"Prints a generation."
[generation]
{:pre  [(set? generation)]}
(doseq [y (sort (extend-by-1 (y-values generation)))]
(doseq [x (sort (extend-by-1 (x-values generation)))]
(if (alive? generation (Cell. x y))
(print "# ")
(print ". ")))
(println)))

(defn clear-screen
"Clears the screen, using ANSI control characters."
[]
(let [esc (char 27)]
(print (str esc "[2J"))     ; ANSI: clear screen
(print (str esc "[;H"))))   ; ANSI: move cursor to top left corner of screen

(defn -main
"Run Conway's Game of Life."
[& args]
(loop [saved-generations #{}
generation        glider]
(if (not (contains? saved-generations generation))
(do
(clear-screen)
(print-generation generation)
(flush)
(recur (conj saved-generations generation) (next-generation generation))))))

• Take a look at this neat little implementation of life in Clojure: clj-me.cgrand.net/2011/08/19/conways-game-of-life Oct 16, 2012 at 3:37
• Thanks mikera - I'll have to have a closer look at frequencies, mapcat and cells!
– Peter
Oct 16, 2012 at 3:43

My suggestions:

Remove the values function and, as mikera suggests, do these types of things inline. Most clojure programmers will know what you mean.

There is no need to reduce with the max function when max is a variable-arity function.

(reduce max [1 2 3 4]) ;; 4


can be

(apply max [1 2 3 4]) ;; 4


When you reduce, you are effectively doing the following

(max (max (max 1 2) 3) 4) ;; 4


When using apply would equate to

(max 1 2 3 4) ;; 4


To show you an example, max-y could be as follows

(defn max-y
[generation]
(apply max (map :y generation)))


I'm on the fence about this one but you could even go as far as to generalize those 2 functions:

(defn maximum
[k hash]
(apply max (map k hash)))

(maximum :y generation)
(maximum :x generation)


A few more simple tips:

There is an if-not macro that can be useful. But the way you have it is perfectly fine.

(if (not (contains? saved-generations generation)) ... )

(if-not (contains? saved-generations generation) ... )


Also, there is a when-not macro that has an implied do form.

(if (not (contains? saved-generations generation))
(do ... ))

(when-not (contains? saved-generations generation)
...)

• Thanks Kyle - really appreciate the pointers to those functions and macros. I'm discovering that half the trick with Clojure seems to be knowing the core library. Oct 16, 2012 at 16:57
• @Peter, you're welcome and correct. It seems like there is a fn for almost everything in clojure.
– Kyle
Oct 16, 2012 at 17:16
• Regarding your comment "do these types of things inline" - how do you decide whether to inline something or separate it out as an explicitly named function? Oct 16, 2012 at 21:32
• @Peter, it's all personal preference really. If it's not obvious what you are doing, a function usually is better. It's difficult when you are starting out because (apply max (map :y generation)) is not really obvious until you are comfortable with functional programming or clojure specifically.
– Kyle
Oct 16, 2012 at 21:35

Some quick points / ideas:

• The code looks pretty decent overall for a first attempt at Clojure, and you have made good use of the "functional" style. Nice work!
• You have used a defrecord for the cells data structure. This isn't really buying you anything, since you can just use a vector e.g. [1 2] or a map e.g. {:x 1 :y 1} directly. I'd probably use a vector: more lightweight, shorter and therefore more readable, it's clear in the context that it it is an [x y] vector.
• This min-x, min-y, x-values etc. functions add a lot of lines of code but are extremely short functions. I'd probably be more tempted to do these inline, especially since they are only used once or twice.

Also take a look at these neat Game of Life implementations:

• Thanks mikera. As you may have noticed I'm having great trouble letting go of my typed background - in fact I originally implemented it using vectors but got freaked out about people passing in vectors of the wrong length, containing the wrong data types etc. Any hints on getting over my fear of "lack of types"? Oct 16, 2012 at 16:48
• Yes: Unit testing. It's absolutely essential when creating a program of any complexity in a dynamic language without the support of static type checking. Learn to love your test suite. Oct 17, 2012 at 1:29
• Not to take this too off-topic, but while tests are good, they're no substitute for a specification (which is partly what type systems are about). Here's a good article on this topic: bertrandmeyer.com/2012/10/14/… Oct 17, 2012 at 22:13
• @Peter - that's why I'm excited about Typed Clojure (github.com/frenchy64/typed-clojure). OTOH, with the exception of "simple" cases like type checking the majority of specifications can't easily be automatically validated (in the way that executable tests can). I therefore argue you need both tests and specifications for different reasons. Oct 17, 2012 at 23:30

## From :

(defn values
"The values of the given generation's given dimension."
[generation
elem]
{:pre [(set? generation)]
:post [(set? %)]}
(set (map elem generation)))

(defn x-values
"The X values of the given generation."
[generation]
{:pre [(set? generation)]
:post [(set? %)]}
(values generation :x))

(defn y-values
"The Y values of the given generation."
[generation]
{:pre [(set? generation)]
:post [(set? %)]}
(values generation :y))


## To:

(defn values
"The values of the given generation's given dimension."
[elem generation]
{:pre [(set? generation)]
:post [(set? %)]}
(set (map elem generation)))

(def x-value (partial values :x))
(def y-value (partial values :y))


## All min-*/max-* can be compressed to:

(defn get-by
[selector source generation]
{:pre [(set? generation)]}
(apply selector (source generation)))

(def min-x (partial get-by min x-value))
(def max-x (partial get-by max x-value))
(def min-y (partial get-by min y-value))
(def max-x (partial get-by max y-value))


## flat-map can be:

(defn flat-map [f s] (set (mapcat f s)))


• Thanks Ankur. mapcat looks very useful indeed - I'll have to read up some more! Any pointers on good ways of discovering / exploring the core libraries? Oct 16, 2012 at 16:58