5
\$\begingroup\$

I have written a Tic-Tac-Toe game. I have a class called GameEndAnalyzer that will check if the game has already ended or not.

Would it violate the Single Responsibility Principle if I will add more responsibilities to this class to not only decide if someone has won the game, but who has won (X or O)? What is the best way to add responsibility to this?

public interface IGameEndAnalyzer
{
    bool IsGameEnd(IBoard board);
}
public class GameEndAnalyzer : IGameEndAnalyzer
{        
    public bool IsGameEnd(IBoard board)
    {
        return IsAnyRowHasAllSameValue(board) ||
               IsAnyColumnHasAllSameValue(board) ||
               IsLeftDiagonalHasAllSameValue(board) ||
               IsRightDiagonalHasAllSameValue(board);
    }

    private bool IsAnyRowHasAllSameValue(IBoard board)
    {           
        for (int rowIndex = Board.Row.Top; rowIndex <= Board.Row.Bottom; rowIndex++)
        {
            var isSameValue = board
                       .Cells
                       .WhereInRow(rowIndex)
                       .AllCellIs_O_Or_X();

            if (isSameValue)
            {
                return true;
            }
        }

        return false;
    }

    private bool IsAnyColumnHasAllSameValue(IBoard board)
    {
        for (int columnIndex = Board.Column.Left; columnIndex <= Board.Column.Right; columnIndex++)
        {
            var isSameValue = board
                       .Cells
                       .WhereInRow(columnIndex)
                       .AllCellIs_O_Or_X();

            if (isSameValue)
            {
                return true;
            }
        }

        return false;
    }

    private bool IsLeftDiagonalHasAllSameValue(IBoard board)
    {
        var isSameValue = board
                          .Cells
                          .WhereInDiagonalLeft()
                          .AllCellIs_O_Or_X();

        return isSameValue;
    }

    private bool IsRightDiagonalHasAllSameValue(IBoard board)
    {
        var isSameValue = board
                          .Cells
                          .WhereInDiagonalRight()
                          .AllCellIs_O_Or_X();

        return isSameValue;
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ I rewrote my earlier Tic Tac Toe implementation to address your question and amended my answer; check it out. \$\endgroup\$
    – Dan
    Jan 5, 2014 at 18:43

1 Answer 1

6
\$\begingroup\$

You're using extension methods for cells; why do use for loops for rows? It strikes me as weird. Also, there's plenty of repetition in your analyzer; you can do better by abstracting some things away (more on that below).

To answer your main question, instead of returning bool for whether somebody won, you can return Nullable<Player> where Player is an enum with X and O values. This would not in any way violate single responsibility principle because your class' responsibility is to analyze the game, and determine the winner. There is no point in finding out if somebody has won, if you don't know who that somebody is.

That being said, I think you could still do better with the implementation. Your GameEndAnalyzer analyzes each outcome manually, whereas you could've made it more generic by introducing a concept of “lines” and a few extension methods (somewhat less than you seem to have).

I re-wrote my earlier toy Tic Tac Toe implementation specifically to address your question, with the following separation:

  • Player is a simple enum:

    enum Player {
        X,
        O
    }
    
  • IBoard represents a board with an indexer by row and column and Size.

    interface IBoard
    {
        int Size { get; }
        Player? this [int row, int column] { get; set; }
    }
    

    (really, you don't need anything else in IBoard)

  • The implementation is trivial:

    class Board : IBoard
    {
        Player? [,] _cells;
    
        public int Size {
            get { return _cells.GetLength (0); }
        }
    
        public Board (int size)
        {
            _cells = new Player? [size, size];
        }
    
        public Player? this [int row, int column] {
            get {
                return _cells [row, column];
            } set {
                if (_cells [row, column].HasValue)
                    throw new InvalidOperationException ("The cell is already claimed.");
    
                _cells [row, column] = value;
            }
        }
    }
    
  • IBoardAnalyzer has a single method that scans the board to find the winner, similarly to yours:

    interface IBoardAnalyzer
    {
        Player? DetermineWinner (IBoard board);
    }
    
  • All the actual heavy-lifting for scanning the board lives in BoardExtensions that can enumerate board rows, columns and diagonals. There is also an über-method called SelectAllLines that returns a sequence of all “lines” (rows, columns and diagonals) on the board. Note that BoardExtensions does no analysis; it only provides convenience methods for extracting data out of Board.

    static class BoardExtensions
    {
        public enum DiagonalKind {
            Primary,
            Secondary
        }
    
        public static IEnumerable<IEnumerable<Player?>> SelectAllLines (this IBoard board)
        {
            return board.SelectAllDiagonals ()
                .Concat (board.SelectAllRows ())
                .Concat (board.SelectAllColumns ());
        }
    
        public static IEnumerable<IEnumerable<Player?>> SelectAllRows (this IBoard board)
        {
            return from row in board.SelectIndices ()
                   select board.SelectRow (row);
        }
    
        public static IEnumerable<IEnumerable<Player?>> SelectAllColumns (this IBoard board)
        {
            return from column in board.SelectIndices ()
                   select board.SelectColumn (column);
        }
    
        public static IEnumerable<IEnumerable<Player?>> SelectAllDiagonals (this IBoard board)
        {
            return from kind in new [] { DiagonalKind.Primary, DiagonalKind.Secondary }
                   select board.SelectDiagonal (kind);
        }
    
        public static IEnumerable<Player?> SelectRow (this IBoard board, int row)
        {
            return from column in board.SelectIndices ()
                   select board [row, column];
        }
    
        public static IEnumerable<Player?> SelectColumn (this IBoard board, int column)
        {
            return from row in board.SelectIndices ()
                   select board [row, column];
        }
    
        public static IEnumerable<Player?> SelectDiagonal (this IBoard board, DiagonalKind kind)
        {
            return from index in board.SelectIndices ()
                   let row = index
                   let column = (kind == DiagonalKind.Primary)
                       ? index
                       : board.Size - 1 - index
                   select board [row, column];
        }
    
        public static IEnumerable<int> SelectIndices (this IBoard board)
        {
            return Enumerable.Range (0, board.Size);
        }
    }
    
  • The implementation for BoardAnalyzer uses BoardExtensions to find the winner, but in itself is trivial:

    class BoardAnalyzer : IBoardAnalyzer
    {
        public Player? DetermineWinner (IBoard board)
        {
            return (
                from line in board.SelectAllLines ()
                let winner = DetermineLineWinner (line)
                where winner.HasValue
                select winner
            ).FirstOrDefault ();
        }
    
        static Player? DetermineLineWinner (IEnumerable<Player?> line)
        {
            try {
                return line.Distinct ().Single ();
            } catch (InvalidOperationException) {
                return null;
            }
        }
    }
    
  • Finally, I implemented an IO which has a simple interface:

    interface IGameIO
    {
        Tuple<int, int> AskNextMove (Player player, IBoard board);
        void DisplayError (GameError error);
        void DisplayWinner (Player player);
    }
    
  • And just as simple implementation:

    class ConsoleGameIO : IGameIO
    {
        public Tuple<int, int> AskNextMove (Player player, IBoard board)
        {
            Console.WriteLine ("\n\n");
            Console.WriteLine ("{0}, what is your move?\n", FormatPlayer (player));
            Console.WriteLine (FormatBoard (board));
            Console.Write ("\nType A1 to C3: ", FormatPlayer (player));
    
            return ParseMove (Console.ReadLine ().Trim ().ToUpperInvariant ());
        }
    
        public void DisplayError (GameError error)
        {
            switch (error) {
            case GameError.CellAlreadyOccupied:
                Console.WriteLine ("Bad move: cell is already occupied.");
                break;
            case GameError.CouldNotParseMove:
                Console.WriteLine ("Bad move. Valid moves are A1 to C3.");
                break;
            default:
                Console.WriteLine ("Something went wrong.");
                break;
            }
        }
    
        public void DisplayWinner (Player player)
        {
            Console.WriteLine ("Congatulations, {0}! You won.", FormatPlayer (player));
        }
    
        static string FormatBoard (IBoard board)
        {
            return string.Join ("\n", from row in board.SelectAllRows () select FormatRow (row));
        }
    
        static string FormatRow (IEnumerable<Player?> row)
        {
            return string.Join ("|", from cell in row select FormatCell (cell));
        }
    
        static string FormatPlayer (Player player)
        {
            return FormatCell (player);
        }
    
        static string FormatCell (Player? cell)
        {
            return cell.HasValue ? cell.Value.ToString () : "_";
        }
    
        static Tuple<int, int> ParseMove (string input)
        {
            return Tuple.Create (
                input [0] - 'A',
                input [1] - '1'
            );
        }
    }
    
  • Finally, there goes the Game class with the run loop and Main method:

    class Game
    {
        IBoard board;
        IBoardAnalyzer analyzer;
        IGameIO io;
    
        public Game (IBoard board, IBoardAnalyzer analyzer, IGameIO io)
        {
            this.board = board;
            this.analyzer = analyzer;
            this.io = io;
        }
    
        public void Run ()
        {
            Player player = Player.X;
            Player? winner = null;
    
            do {
                Tuple<int, int> move;
                try {
                    move = io.AskNextMove (player, board);
                } catch {
                    io.DisplayError (GameError.CouldNotParseMove);
                    continue;
                }
    
                if (!ValidateMove (move)) {
                    io.DisplayError (GameError.CouldNotParseMove);
                    continue;
                }
    
                try {
                    board [move.Item1, move.Item2] = player;
                } catch (InvalidOperationException) {
                    io.DisplayError (GameError.CellAlreadyOccupied);
                    continue;
                }
    
                player = GetNextPlayer (player);
                winner = analyzer.DetermineWinner (board);
    
            } while (!winner.HasValue);
    
            io.DisplayWinner (winner.Value);
        }
    
        bool ValidateMove (Tuple<int, int> move)
        {
            return (move.Item1 >= 0 && move.Item1 < board.Size)
                && (move.Item2 >= 0 && move.Item2 < board.Size);
        }
    
        static Player GetNextPlayer (Player player)
        {
            switch (player) {
            case Player.X:
                return Player.O;
            case Player.O:
                return Player.X;
            default:
                throw new NotImplementedException ();
            }
        }
    
        public static void Main (string[] args)
        {
            var game = new Game (new Board (3), new BoardAnalyzer (), new ConsoleGameIO ());
            game.Run ();
        }
    }
    

Check it out on Github.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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