7
\$\begingroup\$

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)))))
\$\endgroup\$
3
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – LiamGoodacre Jul 24 '12 at 22:34
  • \$\begingroup\$ @LiamGoodacre you're welcome :) I did a heavy edit commenting on the issue and adding some more tips after grasping try-combination. \$\endgroup\$ – Álvaro Cuesta Aug 5 '12 at 16:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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