TicTactics GameBoard Logic

Question

This is my take at the current code-challenge, Ultimate Tic-Tac-Toe.

It all started with a CellValue and a BoardPosition:

/// <summary>
/// Identifies the players, or the possible values a cell can take.
/// </summary>
public enum CellValue
{
    X,
    O
}

/// <summary>
/// Identifies the possible board positions.
/// </summary>
public enum BoardPosition
{
    TopLeft,
    Top,
    TopRight,
    Left,
    Center,
    Right,
    BottomLeft,
    Bottom,
    BottomRight
}

"Wait", I hear you say - "an enum for board positions?" - Absolutely! That allowed me to define a BoardCell interface:

public interface IBoardCell
{
    BoardPosition Position { get; }
    CellValue? Value { get; set; }
    event EventHandler<CellValueChangedEventArgs> CellValueChanged;
}

Inspired by WPF and INotifyPropertyChanged, I'm using events to communicate a change of a cell's value to whoever might be interested (that's the parent board):

public class CellValueChangedEventArgs : EventArgs
{
    private BoardPosition _position;
    BoardPosition Position { get { return _position; } }

    private CellValue? _value;
    CellValue? Value { get { return _value; } }

    public CellValueChangedEventArgs(BoardPosition position, CellValue? value)
    {
        _position = position;
        _value = value;
    }
}

BoardBase[TCell]

Very early in the design process I realized most of the "bigger board" functionality was also needed in the "smaller boards", so I wrote a generic abstract class where the type parameter determines the type of cell:

public abstract class BoardBase<TCell> : IBoardCell
    where TCell : IBoardCell
{
    private readonly IDictionary<BoardPosition, TCell> _cells;
    private readonly BoardEvaluator _evaluator;

    protected BoardBase(BoardEvaluator evaluator, ICellFactory<TCell> cellFactory)
    {
        _evaluator = evaluator;
        _cells = Enum.GetValues(typeof(BoardPosition))
                     .Cast<BoardPosition>()
                     .ToDictionary(position => position, position => cellFactory.Create(position));
        RegisterCellEvents();
    }

    private void RegisterCellEvents()
    {
        foreach (var cell in _cells.Values)
        {
            cell.CellValueChanged += BoardCellValueChanged;
        }
    }

    private void BoardCellValueChanged(object sender, EventArgs e)
    {
        _winner = _evaluator.Evaluate(_cells, out _winningPositions);
        if (_winner != null)
        {
            OnCellValueChanged();
        }
    }

    public event EventHandler<CellValueChangedEventArgs> CellValueChanged;
    private void OnCellValueChanged()
    {
        if (CellValueChanged != null)
        {
            var args = new CellValueChangedEventArgs(_position, Value);
            CellValueChanged(this, args);
        }
    }

    public IReadOnlyDictionary<BoardPosition, TCell> Cells { get { return new ReadOnlyDictionary<BoardPosition, TCell>(_cells); } }

    public virtual TCell this[BoardPosition position]
    {
        get { return _cells[position]; }
        set
        {
            _cells[position] = value;
            _winner = _evaluator.Evaluate(_cells, out _winningPositions);
            if (_winner != null) OnCellValueChanged();
        }
    }

    public CellValue? Value
    {
        get { return _winner; }
        set { throw new NotSupportedException(); }
    }

    public bool IsPlayable()
    {
        return _cells.Values.Any(cell => !cell.Value.HasValue);
    }

    private CellValue? _winner;
    public CellValue? Winner { get { return _winner; } }

    private IEnumerable<BoardPosition> _winningPositions;
    public IEnumerable<BoardPosition> WinningPositions
    {
        get { return _winningPositions; }
    }

    private BoardPosition _position;
    public BoardPosition Position
    {
        get { return _position; }
    }
}

This class is "implemented" like this:

public class GameBoard : BoardBase<SmallBoard>
{
    public GameBoard(BoardEvaluator evaluator, BoardFactory boardFactory)
        : base(evaluator, boardFactory)
    { }
}

public class SmallBoard : BoardBase<BoardCell>
{
    public SmallBoard(BoardEvaluator evaluator, BoardCellFactory cellFactory)
        : base(evaluator, cellFactory)
    { }
}

...which essentially gives a meaningful alias to the generic class. Not sure it's really needed.

Cell Factories

I like abstract factories. This allows me to generate the entire board, simply by enumerating the BoardPosition values:

public interface ICellFactory<TCell> where TCell : IBoardCell
{
    TCell Create(BoardPosition position);
}

public class BoardFactory : ICellFactory<SmallBoard>
{
    private readonly BoardEvaluator _evaluator;
    private readonly BoardCellFactory _cellFactory;

    public BoardFactory(BoardEvaluator evaluator, BoardCellFactory cellFactory)
    {
        _evaluator = evaluator;
        _cellFactory = cellFactory;
    }

    public SmallBoard Create(BoardPosition position)
    {
        return new SmallBoard(position, _evaluator, _cellFactory);
    }
}

public class BoardCellFactory : ICellFactory<BoardCell>
{
    public BoardCell Create(BoardPosition position)
    {
        return new BoardCell(position);
    }
}

BoardEvaluator

The logic that determines whether a board has a winner, and what BoardPosition values contain the winning moves, is encapsulated in this BoardEvaluator class:

public class BoardEvaluator
{
    private readonly Tuple<BoardPosition, BoardPosition, BoardPosition>[]
        _wins = new Tuple<BoardPosition, BoardPosition, BoardPosition>[]
                { 
                    // horizontal wins
                    Tuple.Create(BoardPosition.TopLeft, BoardPosition.Top, BoardPosition.TopRight),
                    Tuple.Create(BoardPosition.Left, BoardPosition.Center, BoardPosition.Right),
                    Tuple.Create(BoardPosition.BottomLeft, BoardPosition.Bottom, BoardPosition.BottomRight),

                    // vertical wins
                    Tuple.Create(BoardPosition.TopLeft, BoardPosition.Left, BoardPosition.BottomLeft),
                    Tuple.Create(BoardPosition.Top, BoardPosition.Center, BoardPosition.Bottom),
                    Tuple.Create(BoardPosition.TopRight, BoardPosition.Right, BoardPosition.BottomRight),

                    // diagonal wins
                    Tuple.Create(BoardPosition.TopLeft, BoardPosition.Center, BoardPosition.BottomRight),
                    Tuple.Create(BoardPosition.BottomLeft, BoardPosition.Center, BoardPosition.TopRight)
                };

    public CellValue? Evaluate<TCell>(IDictionary<BoardPosition, TCell> cells, out IEnumerable<BoardPosition> positions)
        where TCell : IBoardCell
    {
        if (Enum.GetValues(typeof(BoardPosition)).Length != cells.Count) throw new ArgumentException("Invalid cell count.", "cells");

        var winning = Enum.GetValues(typeof(CellValue)).Cast<CellValue>()
                          .Select(value => WinningPositions(cells, value))
                          .SingleOrDefault(tuple => tuple != null);

        positions = winning != null
            ? new[] { winning.Item1, winning.Item2, winning.Item3 }
            : null;

        return winning != null
            ? cells[winning.Item1].Value // all 'winning' positions have the same value.
            : null;
    }

    private Tuple<BoardPosition, BoardPosition, BoardPosition> WinningPositions<TCell>(IDictionary<BoardPosition, TCell> cells, CellValue? value)
        where TCell : IBoardCell
    {
        return _wins.SingleOrDefault(win => cells[win.Item1].Value == value
                                         && cells[win.Item2].Value == value
                                         && cells[win.Item3].Value == value);
    }
}

I think this is where the enum positions deliver their payload, especially in terms of readability. I have to admit, I'm really not crazy about out parameters. I can live with this one, but I'd love to see it gone and turned into a regular return value... or maybe sometimes an out parameter is ok? Is this such a situation?

---

That's about it for now. I also have a IPlayer interface, but that's not implemented yet so I'll keep that for when I want to get the playability reviewed (game mechanics/logic).

So, is this what clean-code looks like? What could be improved?

(I also have some XAML to be reviewed for this project

Answer 1

It all started with a CellValue and a BoardPosition

You could use bool? for a ternary CellValue; but a named enum makes it more self-documenting.

You could code your BoardPosition as a bunch of flyweights:

public class BoardPosition // or struct
{
    public int Row { get; private set; }
    public int Column { get; private set; }

    public static BoardPosition TopLeft = new BoardPosition() { Row = 0; Column = 0; };
    ... etc ...

    // Implicit conversion to int allows it to be used as indexer into array of cells
    public static implicit operator int(BoardPosition self)
    {
        return self.Column + (3 * self.Row);
    }

    // Could similarly define a constructor-operator to convert from int
    // http://msdn.microsoft.com/en-us/library/85w54y0a.aspx
}

That allowed me to define a BoardCell interface

You could use shorter names:

• CellValue -> State or Owner or Player
• BoardPosition -> Position or Location
• IBoardCell -> Cell
• BoardBase -> Cells or BoardT

CellValueChangedEventArgs

You can make this slightly shorter by using auto-implemented properties:

public class CellValueChangedEventArgs : EventArgs
{
    public BoardPosition Position { get; private set; }
    public CellValue? Value { get; private set; }

    public CellValueChangedEventArgs(BoardPosition position, CellValue? value)
    {
        Position = position;
        Value = value;
    }
}

BoardBase[TCell]

Why is this abstract when it contains no abstract methods?

Instead of ICellFactory<TCell> cellFactory using a factory interface, you could define Func<TCell, BoardPosition> cellFactory using a factory delegate.

It's confusing the find member data at the top and at the bottom of the class definition.

Use #region to specify which methods of BoardBase are implementing members defined in / required by IBoardCell.

The Value property would be better named Winner.

IsPlayable should perhaps be false on a won board.

It was surprising to see IDictionary instead of a single- or two-dimensional array.

this[BoardPosition position] { set { ... } } requires the user to create a new cell which they can pass-in. That's difficult and error-prone IMO. Instead they should be allowed to try to alter the CellValue of an existing cell.

I'm really not crazy about out parameters. I can live with this one, but I'd love to see it gone

You could pass-in an Action<IEnumerable<BoardPosition>> instead:

_winner = _evaluator.Evaluate(_cells, found => _winningPositions = found);

Or return a Pair<CellValue, IEnumerable<BoardPosition>>

Or pass-in this so that evaluator can set the WinningPositions property of the passed-in BoardBase.

---

Of the above, IMO the most important review comment is to get rid of TCell this[BoardPosition position] { set { ... } }.

CellValueChangedEventArgs needn't be a subclass of EventArgs. The only property it needs is IBoardCell which contains the new location and new value of the cell.

BoardCellValueChanged could do something with the parameter[s] it's being passed.

Answer 2

I like it,

especially the enums.

Although the Tuple evaluator is super hard to parse.

Definitely think it would be better to wrap that in a WinCondition interface or something which contains the relevant enum flag.

besides if memory serves you can perform flag concatenation for a cleaner compare.

BoardPosition winPosition = BoardPosition.TopLeft | BoardPosition.Top | BoardPosition.TopRight;

and make a list of them:

_winPositions.Any(position => position == currentPosition)

or something....

Answer 3

1. I can see some code duplication. For example this

   private void BoardCellValueChanged(object sender, EventArgs e)
   {
       _winner = _evaluator.Evaluate(_cells, out _winningPositions);
       if (_winner != null)
       {
           OnCellValueChanged();
       }
   }
   

   looks a lot like this

   set
   {
       _cells[position] = value;
       _winner = _evaluator.Evaluate(_cells, out _winningPositions);
       if (_winner != null) OnCellValueChanged();
   }
   

2. I do not like this enum

   public enum CellValue
   {
       X,
       O
   }
   

   for two reasons. First: it doesn't have a third value for "empty" cells. This leads to huge amout of CellValue? all over the place. Having a third state will remove those and imrove code readability. Second: i do not like X and O as member names. I cant say i do not understand those, but there is something fishy about this naming. :) I think Tic and Toc would be much better.

3. Why throw an exception? Why cant you remove the setter? Is this property supposed to be virtual?

   public CellValue? Value
   {
       get { return _winner; }
       set { throw new NotSupportedException(); }
   }
   

4. public CellValue? Winner { get { return _winner; } } - i think you should either change this property name, or change type to IPlayer. Semantically cell being a winner doesnt make much sense.

5. In general i am not sure i can follow how will you derive from BoardBase. IsPlayable implementation for small board doesnt make sense (shouldn't you check for winner instead?). Same goes for Position for large board. I have no idea how Value is going to be used for either boards, and what Value means (not very descriptive). Etc.

6. As for BoardEvaluator i have a few minor concerns. a) I hate Tuples, Pairs, etc. with all my heart and soul. :) THey have this ability to turn even simple code into a mess. This is probably a matter of taste, but i think a simple arrays would be much more readable. b) I do not like multiple winning != null, it makes code hard to follow. A simple if would be better.