Clojure TicTacToe (logic, no GUI)

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?

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: actua

StackExchange Code Review Q#12238, answer score: 3