Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This is my take at the current , 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 looks like? What could be improved?

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

share|improve this question
add comment

3 Answers

up vote 3 down vote accepted

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.

share|improve this answer
    
Nice review. BoardBase<TCell> is abstract because I din't want it directly instantiated - looking at my GameBoard : BoardBase<SmallBoard> and SmallBoard : BoardBase<BoardCell> classes/stubs I'm starting to think you're right, I don't need it to be abstract. It's just GameBoard seemed more descriptive than BoardBase<SmallBoard>... but I can see how it isn't. –  Mat's Mug Feb 6 at 14:41
1  
I din't want it directly instantiated Just make its constructor protected, then. –  ChrisW Feb 6 at 14:48
    
The indexer does violate POLS a little; it works specifically to avoid having to create a new cell: I can use it like gameBoard[BoardPosition.Center][BoardPosition.Left] = CellValue.X. –  Mat's Mug Feb 6 at 15:03
    
@lol.upvote Don't you need a (new) BoardCell or a SmallBoard on the right-hand side of that assignment expression, not a CellValue? Post some unit-test code which shows how you use the APIs. –  ChrisW Feb 6 at 15:48
1  
@lol.upvote Exactly. And by letting user-code poke new state into a cell, therefore Cell needs to use an event handler to notify Game when the cell's state has changed. For that reason (to avoid the complexity of bubbling state change via event handler) I was inspired to ensure that state can only be changed via a method of Game: so that Game knows when state is being changed, and can implement preconditions (detecting illegal plays) and post-conditions (updating winners). If someone plays illegally you might detect that in your event handler, and restore to previous state (!) before throwing. –  ChrisW Feb 7 at 1:35
show 2 more comments

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

share|improve this answer
    
That looks like an interesting approach that takes advantage of the very nature of enums. I like this idea. Very much. –  Mat's Mug Feb 6 at 14:43
add comment
  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.

share|improve this answer
    
The CellValue enum might be better off called PlayerToken - I'm also using it in my IPlayer interface (not in this post) to determine whether a player is playing "X" or "O". Thus, I didn't want to include a "non-value" in that enum, and making a cell's value a Nullable<CellValue> seemed semantically correct. Speaking of semantics, it's also what drove the decision of using Tuple<T1,T2,T3> over anything else (which I did consider) - the tuple, as ugly as it is, seemed the only semantically correct approach in this case. Nice review, you bring very good points! Thanks! –  Mat's Mug Feb 6 at 14:49
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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