6
\$\begingroup\$

I'm a Clojure beginner and I implemented Game of life to learn how I should code things in Clojure.

(def state (ref [
    [false false false false false]
    [false false true  false false]
    [false false true  false false]
    [false false true  false false]
    [false false false false false]]))

(defn get-state [state x y]
  (nth (nth state y) x))

(defn count-true-neighbours [state x y]
  (let [size (count state)]
    (->>
      (for [p (range (- x 1) (+ x 2))
            q (range (- y 1) (+ y 2))
            :when (not (and (= x p) (= y q)))]
        [p q])
      (map (fn [coord]
        (let [[p q] coord]
          (if (or (< p 0) (>= p size) (< q 0) (>= q size))
            false
            (get-state state p q)))))
      (filter true?)
      (count))))

(defn next-state-of [state x y]
  (let [alive (get-state state x y)
        alive-neighbours (count-true-neighbours state x y)]
    (cond
      (and (not alive) (= alive-neighbours 3)) true
      (and alive (or (= alive-neighbours 2) (= alive-neighbours 3))) true
      :else false)))

(defn next-state [state]
  (let [size (count state)]
    (mapv
      (fn [row]
        (mapv
          #(next-state-of state %1 row)
          (range 0 size)))
      (range 0 size))))

(defn pretty-print [state]
  (let [size (count state)]
    (loop [i 0]
      (if (< i size)
        (do
          (println
            (->>
              (nth state i)
              (mapv #(if %1 "■ " "□ "))
              (reduce str)))
          (recur (inc i)))))))

(defn tick
  ([times] (tick 0 times))
  ([i times]
    (if (< i times)
      (do
        (dosync
          (println (str i ":"))
          (pretty-print @state)
          (ref-set state (next-state @state)))
        (recur (inc i) times))
      nil)))

(defn -main [& args]
  (tick 5))

I used ref to learn how to manage state although it seems like an overkill for this tiny program.

As you see my code is terrible, but I couldn't improve this by myself.

I already checked this answer, but I think I did this in a quite different way and need a different advice.

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Your code actually isn't terrible, but there are a few places where it can be improved.


First, the ref. It's entirely unnecessary in this case. What I'd change to keep everything immutable:

Change your declaration at the top to just:

(def initial-state [[false false false false false]
                    [false false true  false false]
                    [false false true  false false]
                    [false false true  false false]
                    [false false false false false]])

This now defines, as the name suggests, the initial state of the game. This will no longer be used to store the running state.

Now, you can change tick to get rid of the need for ref-set:

(defn tick [state times]
  (loop [i 0 ; Moved the counter into a loop
         acc-state state] ; Keeping track of the running state
   (when (< i times)
     (println (str i ":"))
     (pretty-print acc-state)

     ; Recur into the loop instead of the function itself
     (recur (inc i) (next-state acc-state)))))

(defn -main [& args]
  (tick initial-state 5))

Note how I just made the running state an accumulator of a loop. I think the whole design should be rethought though. Really, you could just get the successive states by using iterate, and take as many as you want. Lets say you want to get the first five states after the initial-state:

(take 5 (iterate next-state initial-state))

Now, your entire tick function can basically be written as:

; Obviously, change 5 to however many states you want to produce
(doseq [state (take 5 (iterate next-state initial-state))]
  (pretty-print state))

I would change pretty-print to a format style function that returns a string instead of printing directly. That way, the caller can make use of your prettying, but decide how/when they want to display it.


Your count-true-neighbors function is huge. I'd break it down into a few discreet parts:

  • You have a part that produces a list of neighbors to check
  • You have a part that does bounds checking

I'd change it to something more like:

(defn neighbors-of [x y]
  (for [p (range (- x 1) (+ x 2))
        q (range (- y 1) (+ y 2))
        :when (not (and (= x p) (= y q)))]
    [p q]))

(defn inbounds? [x y size]
  (and (<= 0 x (dec size)) ; Comparison operators can be chained!
       (<= 0 y (dec size))))

(defn count-true-neighbours [state x y]
  (let [size (count state)]
    (->> (neighbors-of x y)
      (map (fn [[p q]] ; You can deconstruct here directly
             (when (inbounds? p q size)
               (get-state state p q))))
      (filter true?)
      (count))))

Your mapping function could be changed to:

(map (fn [[p q]]
       (and (inbounds? p q size)
            (get-state state p q))))

Although the benefit here is subjective.

Things to note:

  • <= and other comparison operators can be used to check more than two arguments! I'm making use of that in inbounds? to neaten it up a bit.
  • I reduced your bounds check in the map down to (when (inbounds? p q size) ...). when returns nil (falsey) if the condition fails, so you don't need to explicitly return false.

(or (= alive-neighbours 2) (= alive-neighbours 3)))

Can also be written as:

(#{2 3} alive-neighbors)

It's a matter of style which you prefer. I prefer the latter personally.


(nth (nth state y) x))

Can also be written as:

(get-in state [y x])    

There's a few things to note in pretty-print:

(reduce str strings)

Isn't wrong, but a more idiomatic way of writing it would be:

(apply str strings)

With:

(mapv #(if %1 "■ " "□ "))

You add explicit spaces after the display characters. It may be cleaner to use join to add the spaces. This limits the repetition, and gets rid of the trailing space. This also lets you just use the character literals directly.

And when you use threading macros (->>), you put the expression being threaded on the next line. Personally, I prefer to put the threaded expression on the same line as the ->>. This makes it clearer what's being threaded.

Taking that all together, I'd write that whole bit as:

(->> (nth state i)
     (mapv #(if %1 \■ \□))
     (s/join " ")
     (apply str)))

Where s is an alias for clojure.string.

\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.