Clojure TicTacToe (logic, no GUI)

Question

I whipped this up last night and was wondering if anyone had any thoughts/comments on it; for example, on:

• Code organisation
• Coding style
• Other improvements
• Semantic errors

(ns tictactoe.core)
(comment "
 Author: Liam Goodacre
 Date: 03/06/12
 This module allows the creation and progression of tictactoe games.
 To create a game, use the make-game function:
 (make-game)
 A game is a map with the following data:
 :play-state :playing | :player1 | :player2 | :draw
 :level-state [:player1|:player2|nil]
 :scores {:player1 Int, :player2 Int, :draw Int}
 :turn :player1 | :player2 | :none
 If the game isn't over, play-state is :playing.
 If play-state is :player1 or :player2, this indicates that that player won.
 If play-state is :draw, then the game completed with no winner.
 level-state is a vector or nine elements.
 An empty cell has the value of nil.
 If a player uses a cell, then the cell has that player's id.
 E.g., if a player1 uses an empty cell, that cell now has the value :player1
 scores is a map from ids to integers.
 It includes score values for draws and for the two players.
 When the game is over, turn is :none.
 Otherwise, it describes which player's turn it is.
 When a new game is created, turn is randomised
 between the two players.
 To play a turn, use the play-turn command.
 For example, assuming 'g' is a predefined game:
 (play-turn g 2)
 Will produce a new game state with play progressed by one move,
 in which the current player used cell 2.
 Calling new-game clears the level data of a game and
 randomises the initial player turn. Scores are the
 only preserved datum.
 The following are game analysis functions:
 get-play-state - what state of play are we in?
 get-turn - whos turn is it?
 get-scores - get the scores
 get-level-state - get the state of the level
 playing? - are we still playing?
 game-over? - is the game over?
 valid-move? - is this a valid move?
")
(declare ;; Public Functions
 ; Game Transforming
 make-game
 new-game
 play-turn
 ; Game Analysing
 get-play-state
 get-turn
 get-scores
 get-level-state
 playing?
 game-over?
 valid-move?
 ; Extra
 get-game-grid)
(declare ;; Private Functions
 ^:private
 ; Transforming
 apply-move
 next-turn
 recalculate-state
 ; Analysis
 calculate-winner
 try-combination)
;; Utility functions
(defmacro ^:private def-
 " A private version of def. "
 [& info] `(def ^:private ~@info))
(defn- within-range
 " Determines if a value lies within an inclusive range. "
 [n x] (fn [v] (and (>= v n) (<= v x))))
;; Initial data for making new games
(def- players [:player1, :player2])
(def- initial-play-state :playing)
(def- initial-level-state (vec (repeat 9 nil)))
(def- initial-scores {:player1 0, :player2 0, :draw 0})
(defn- random-turn [] (players (int (rand 2))))
(def- winning-combinations [[1 2 3][4 5 6][7 8 9][1 5 9][3 5 7][1 4 7][2 5 8][3 6 9]])
;; Public Transforming Functions
(defn make-game []
 " Creates a new game object. "
 { :play-state initial-play-state
 :level-state initial-level-state
 :scores initial-scores
 :turn (random-turn) })
(defn new-game [g]
 " Sets up a game object for the next play. "
 (assoc (make-game) :scores (get-scores g)))
(defn play-turn [game move]
 " Progresses game-play by one move if possible. "
 (if (and (playing? game) (valid-move? game move))
 (-> game
 (apply-move move)
 (recalculate-state)
 (next-turn))
 game))
;; Private Transforming Functions
(defn- apply-move [game move]
 " Progresses game-play by one move. "
 (assoc game
 :level-state
 (assoc (get-level-state game)
 move
 (get-turn game))))
(defn- next-turn [game]
 " Updates which player should play next.
 If no player is currently active, then
 do not switch. "
 (assoc game
 :turn
 (case (get-turn game)
 :none :none
 :player1 :player2
 :player2 :player1)))
(defn- recalculate-state [game]
 " Calculate if there is a winner. If so update
 the play-state, turn, and scores; to reflect
 the result. "
 (let [winner (calculate-winner game)
 scores (get-scores game)]
 (if (= winner :none)
 game
 (-> game
 (assoc :play-state winner, :turn :none)
 (update-in [:scores winner] inc)))))
;; Private Analysis Functions
(defn- calculate-winner [game]
 " Calculates if there is a winner. "
 (let [state (get-level-state game)
 matching (partial try-combination state)]
 (if (some matching winning-combinations)
 (get-turn game)
 (if (some nil? state)
 :none
 :draw))))
(defn- try-combination [state [one two three]]
 " Calculates if a winning combination has occurred. "
 (let [lookup (comp (partial nth state) dec)
 player (lookup one)]
 (and
 (not= player nil)
 (= player (lookup two) (lookup three)))))
;; Public Analysis Functions
(def get-play-state :play-state)
(def get-turn :turn)
(def get-scores :scores)
(def get-level-state :level-state)
(defn playing? [game] (= :playing (get-play-state game)))
(def game-over? (comp not playing?))
(defn valid-move? [game move]
 (and ((within-range 0 8) move)
 (nil? (nth (get-level-state game) move))))
;; Public Extra Functions
(defn get-game-grid [g]
 " Builds a simple string representation
 of the current level state. "
 (clojure.string/join "\n" (map vec (partition 3 (get-level-state g)))))

Answer 1

I would put the long (comment ...) as the namespace's docstring. In fact, I would split that in docstrings because you're essentially describing the system by parts in a centralized place, while you could describe it in each part. I would keep the overall "how this game/namespace works/is organized" in namespace's docstring.

Declare is meant for forward declarations, not to expose functions (that's what you're doing there, right?) I didn't see it used like that anywhere, but I like the idea!

(not= player nil) is just player!

In next-turn I would just use a map instead of case (you're essentially mapping, right?)

(defn- next-turn [game]
 (assoc game :turn
 ((get-turn game)
 {:none :none ;; I haven't seen spacing to match vertically used too much, but I don't really dislike it
 :player1 :player2
 :player2 :player1})) ;; Or the other way around `(the-map (get-turn game))`

IMHO, this isn't very idiomatic:

(def get-play-state :play-state)
...

Aren't you better off using :play-state directly? It offers no encapsulation or maintainability whatsoever (am I right guessing this came from an OOP-like mindset?) And you lose information: the fact that :play-state is a keyword. I guess you did it to expose the analysis functions, right? You're better off mentioning the acceptable keywords in the documentation for your public make-game. I would use either (:play-state game) or (game :play-state) (the second will throw a NullPointerException if game is nil, which isn't bad per se.)

This is obviously style but: same goes for Initial data for making new games. I would shove that data directly into the make-game. It'd be already obvious for me that those are the initial states (since you're just bulding a map!)

The code is pretty good. I didn't even read the long documentation text and got how it works in a quick glance. I had some troyble understanding how try-combination works (consider making it more readable) but unfortunately I'm a bit in a hurry. I'll come back later and review that too (consider "not easily readable" my review at the moment.)

That's my humble opinion. The rest is pretty good.

EDIT

(answer to comment)

@LiamGoodacre that's what I thougth. Clojure favors universal (i.e. reusable) behavior for types instead of "interface tied to type", so it's just the other way around compared to OOP where data and actions are heavily tied.

Don't disregard the docstring tips either. Try (doc function-or-namespace) in a REPL to see how important is to have documentation tied to functions/namespaces instead of spread throughout the code or in (comment ...) forms. Documentation generation tools use this metadata too, so it's generally a good idea to do this.

I had a hard time grasping try-combination. Consider reworking it a bit, or maybe just add some more info to the docstring like what's [one two three]. The current dosctring is not every useful, I know what the functions does just by reading the name... it tries combinations, and "Calculates if a winning combination has occurred." is just a fancy way of saying it tries combinations.

This is just a matter of style, but I dislike heavy use of partial, comp, etc. and would prefer #(nth state (dec %)). I think it's easier to read. I also prefer [a b c] or [x y z] to [one two three]. I had a hard time following you were just destructuring a simple array instead of getting some special values that deserved the long one, two, three names :)

Why don't you return the winner in try-combination? Do it like this:

(defn- try-combination [state [x y z]]
 (let [lookup #(nth state (dec %))
 player (lookup x)]
 (and (= player
 (lookup y)
 (lookup z))
 player)))

That way, calculate-winner could return the actual winner and not just the current player, being more correct and reusable IMHO, also allowing you to decouple it from the game and just depending on the current state (e.g. what if you need it later to show the winner in a GUI for recorded games where you only have the final :level-state?)

(defn- calculate-winner [state]
 (let [matching (partial try-combination state)
 winner (find matching winning-combinations)]
 (or winner ; Short-circuit logic is cool for these tricks and VERY readable
 (and (some nil? state)
 :none)
 :draw))))

The more decoupled your functions are, the more reusable. Try working with the bare minimum data structures in a single function, instead of using complex maps.

I would also use a defrecord to define your game data structure.

(defrecord Game [play-state level-state scores turn])

Instance Games like any other Java class, e.g. for make-game :

(Game.
 initial-play-state
 initial-level-state
 initial-scores
 (random-turn))

The good thing: the map interface is still there!

IMHO, it offers some benefits: actually encapsulating the tightly coupled structures into an actual object (is there any benefit in the game being a simple map?), the map keys are "documented" as the record args and you can add interfaces to this object. E.g.: you could get rid of get-game-grid and implement it as java.lang.Object's toString, so you could do (str game) and get game's grid (a nice feature IMHO.)

You can also add customs protocols to these records to encapsulate the tightly coupled interfaces to your Game object (your Public Analysis Functions, although I would discard most of that.) Please note this is very OOP-y and should be kept to a minimum.

Finally: why bother defining functions as private? Screw encapsulation! You'll eventually need these functions outside your namespace for random uninentended tasks (specially if outside people interface with your game, it'd be a cool tic-tac-toe library), so keep these to a minimum (usually helper functions or functions that are tightly coupled with your namespace inner workings.)

• LiamGoodacre: Cheers for all the pointers. My main goal with aliasing the keywords (for example :play-state as get-play-state) was to keep a consistant interface with the game: e.g. game-over? is a function, whereas :play-state is a keyword. Having said that, I now see some benefits to reverting these to keywords: play-state is game data, whereas game-over? is a calculated field.
• Álvaro Cuesta: @LiamGoodacre you're welcome :) I did a heavy edit commenting on the issue and adding some more tips after grasping try-combination.