Critique my Clojure "Game of Life" code

Question

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])
;
; This work is licensed under the Creative Commons Attribution-ShareAlike
; 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)})

(def blinker #{(Cell. 0 0)
               (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)
        (Thread/sleep 250)
        (recur (conj saved-generations generation) (next-generation generation))))))

Answer 1

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)
  ...)

Answer 2

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:

• http://clj-me.cgrand.net/2011/08/19/conways-game-of-life/
• http://programmablelife.blogspot.sg/2012/08/conways-game-of-life-in-clojure.html
• https://github.com/mikera/clojure-golf/blob/master/src/main/clojure/clojure-golf/examples/life.clj

Answer 3

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

---