10
\$\begingroup\$

I'm posting here my implementation of the game Connect 4. It's obviously a pretty trivial game and code but I'm primarily interested in any advice concerning improvements to the code structure: types, dependencies and coupling, concerns, etc.

I'm interested in attacking more complicated games but I think it's best to start with really simple cases, learn all the best practices and slowly build upon that. I think this game is as good as any to start learning some trivial (for the moment) game implementation.

My code is structured in the following main classes (sorry it's in Spanish):

  • AIEngine: Computer player AI
  • Game: Basic game mechanics
  • Player: Human and computer players
  • Board: Game board
  • Judge: Rule enforcer

There are some additional auxiliary classes and enums:

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Text;

namespace Conect4
{
    internal sealed class AIEngine
    {
        readonly int maximumDepth;
        readonly Random random;

        public AIEngine(DifficultyLevel difficultyLevel)
        {
            this.maximumDepth = (int)difficultyLevel;

            if (maximumDepth < (int)DifficultyLevel.Easy ||
                maximumDepth > (int)DifficultyLevel.Hard)
                throw new ArgumentOutOfRangeException("difficultyLevel");

            this.random = new Random(DateTime.Now.Millisecond);
        }

        public int GetBestMove(Board board, ActivePlayer player)
        {
            if (board == null)
                throw new ArgumentNullException("board");

            var node = new Node(board);
            var possibleMoves = getPossibleMoves(node);
            var scores = new double[possibleMoves.Count];
            Board updatedBoard;

            for (int i = 0; i < possibleMoves.Count; i++)
            {
                board.MakePlay(player, possibleMoves[i], out updatedBoard);
                var variant = new Node(updatedBoard);
                createTree(getOpponent(player), variant, 0);
                scores[i] = scoreNode(variant, player, 0);
            }

            double maximumScore = double.MinValue;
            var goodMoves = new List<int>();

            for (int i = 0; i < scores.Length; i++)
            {
                if (scores[i] > maximumScore)
                {
                    goodMoves.Clear();
                    goodMoves.Add(i);
                    maximumScore = scores[i];
                }
                else if (scores[i] == maximumScore)
                {
                    goodMoves.Add(i);
                }
            }

            return possibleMoves[goodMoves[random.Next(0, goodMoves.Count)]];
        }

        private List<int> getPossibleMoves(Node node)
        {
            var moves = new List<int>();

            for (int i = 0; i < Board.BoardColumns; i++)
            {
                if (node.Board.GetCellState(0, i) == CellStates.Empty)
                {
                    moves.Add(i);
                }
            }

            return moves;
        }

        private void createTree(ActivePlayer player, Node rootNode, int depth)
        {
            if (depth >= maximumDepth)
                return;

            var moves = getPossibleMoves(rootNode);

            foreach (var move in moves)
            {
                Board updatedBoard;
                rootNode.Board.MakePlay(player, move, out updatedBoard);
                var variantNode = new Node(updatedBoard);
                createTree(getOpponent(player), variantNode, depth + 1);
                rootNode.Variants.Add(variantNode);
            }
        }

        private double scoreNode(Node nodo, ActivePlayer player, int depth)
        {
            double score = 0;

            if (Judge.CheckForVictory(player, nodo.Board))
            {
                if (depth == 0)
                {
                    score = double.PositiveInfinity;
                }
                else
                {
                    score += Math.Pow(10.0, maximumDepth - depth);
                }
            }
            else if (Judge.CheckForVictory(getOpponent(player), nodo.Board))
            {
                score += -Math.Pow(100
                    , maximumDepth - depth);
            }
            else
            {
                foreach (var varianteContrincante in nodo.Variants)
                {
                    score += scoreNode(varianteContrincante, player, depth + 1);
                }
            }

            return score;
        }

        private static ActivePlayer getOpponent(ActivePlayer jugador)
        {
            return jugador == ActivePlayer.Red ? ActivePlayer.Yellow : ActivePlayer.Red;
        }

        private class Node
        {
            readonly Board board;
            readonly List<Node> variants;

            public Board Board { get { return board; } }
            public List<Node> Variants { get { return variants; } }

            public Node(Board tablero)
            {
                this.board = tablero;
                this.variants = new List<Node>();
            }
        }
    }

    public class Program
    {
        static void Main(string[] args)
        {
            var game = Game.CreateConsoleGame(DifficultyLevel.Hard, ActivePlayer.Yellow, false);
            game.Play();
        }
    }

    internal static class Judge
    {
        public const int requiredCellsInARow = 4;

        public static bool CheckForVictory(ActivePlayer player, Board tablero)
        {
            if (tablero == null)
                throw new ArgumentNullException("board");

            for (int i = 0; i < Board.BoardRows; i++)
            {
                for (int j = 0; j < Board.BoardColumns; j++)
                {
                    if (tablero.GetCellState(i, j) == (CellStates)player)
                    {
                        if (checkForVictory(tablero, i, j))
                            return true;
                    }
                }
            }

            return false;
        }

        private static bool checkForVictory(Board board, int fila, int columna)
        {
            bool searchRight, searchLeft, searchUp, searchDown;

            searchRight = columna <= Board.BoardColumns - requiredCellsInARow;
            searchLeft = columna >= requiredCellsInARow - 1;
            searchUp = fila > Board.BoardRows - requiredCellsInARow;
            searchDown = fila <= Board.BoardRows - requiredCellsInARow;

            if (searchRight)
            {
                if (checkCells(board.GetCellState(fila, columna),
                                    board.GetCellState(fila, columna + 1),
                                    board.GetCellState(fila, columna + 2),
                                    board.GetCellState(fila, columna + 3)))
                    return true;
            }

            if (searchLeft)
            {
                if (checkCells(board.GetCellState(fila, columna),
                                    board.GetCellState(fila, columna - 1),
                                    board.GetCellState(fila, columna - 2),
                                    board.GetCellState(fila, columna - 3)))
                    return true;
            }

            if (searchUp)
            {
                if (checkCells(board.GetCellState(fila, columna),
                                    board.GetCellState(fila - 1, columna),
                                    board.GetCellState(fila - 2, columna),
                                    board.GetCellState(fila - 3, columna)))
                    return true;
            }

            if (searchDown)
            {
                if (checkCells(board.GetCellState(fila, columna),
                                    board.GetCellState(fila + 1, columna),
                                    board.GetCellState(fila + 2, columna),
                                    board.GetCellState(fila + 3, columna)))
                    return true;
            }

            if (searchLeft && searchUp)
            {
                if (checkCells(board.GetCellState(fila, columna),
                                    board.GetCellState(fila - 1, columna - 1),
                                    board.GetCellState(fila - 2, columna - 2),
                                    board.GetCellState(fila - 3, columna - 3)))
                    return true;
            }

            if (searchLeft && searchDown)
            {
                if (checkCells(board.GetCellState(fila, columna),
                                    board.GetCellState(fila + 1, columna - 1),
                                    board.GetCellState(fila + 2, columna - 2),
                                    board.GetCellState(fila + 3, columna - 3)))
                    return true;
            }

            if (searchRight && searchUp)
            {
                if (checkCells(board.GetCellState(fila, columna),
                                    board.GetCellState(fila - 1, columna + 1),
                                    board.GetCellState(fila - 2, columna + 2),
                                    board.GetCellState(fila - 3, columna + 3)))
                    return true;
            }

            if (searchRight && searchDown)
            {
                if (checkCells(board.GetCellState(fila, columna),
                                    board.GetCellState(fila + 1, columna + 1),
                                    board.GetCellState(fila + 2, columna + 2),
                                    board.GetCellState(fila + 3, columna + 3)))

                    return true;
            }

            return false;
        }

        private static bool checkCells(params CellStates[] celdas)
        {
            Debug.Assert(celdas.Length == requiredCellsInARow);

            for (int i = 1; i < requiredCellsInARow; i++)
            {
                if (celdas[i] != celdas[0])
                    return false;
            }

            return true;
        }
    }

    public class Game
    {
        private Board board;
        private Player humanPlayer, computerPlayer, activePlayer;
        private IODevice iODevice;

        public static Game CreateConsoleGame(DifficultyLevel difficultyLevel, ActivePlayer colorOrdenador, bool computerHasFirstTurn)
        {
            return new ConsoleGame(difficultyLevel, colorOrdenador, computerHasFirstTurn, IODevice.CreateConsoleDevice());
        }

        private Game(DifficultyLevel difficultyLevel, ActivePlayer computerColor, bool computerHasFirstTurn, IODevice iODevice)
        {
            board = Board.EmptyBoard;
            this.iODevice = iODevice;
        }

        public ActivePlayer ActivePlayerColor { get { return this.activePlayer.Color; } }
        public Board Board { get { return board; } }
        public IODevice UserInterface { get { return iODevice; } }

        private void changeActivePlayer()
        {
            if (activePlayer == humanPlayer)
            {
                activePlayer = computerPlayer;
            }
            else
            {
                activePlayer = humanPlayer;
            }
        }

        public virtual void Play()
        {
            activePlayer.RequestMove(board);
        }

        private class ConsoleGame : Game
        {
            public ConsoleGame(DifficultyLevel difficultyLevel, ActivePlayer computerColor, bool computerPlaysFirst, IODevice iODevice)
                : base(difficultyLevel, computerColor, computerPlaysFirst, iODevice)
            {
                computerPlayer = Player.CreateComputerPlayer(computerColor, difficultyLevel, iODevice);
                humanPlayer = Player.CreateHumanPlayer(computerColor == ActivePlayer.Red ? ActivePlayer.Yellow : ActivePlayer.Red, iODevice);

                if (computerPlaysFirst)
                {
                    activePlayer = computerPlayer;
                }
                else
                {
                    activePlayer = humanPlayer;
                }

                this.iODevice = iODevice;
            }

            public override void Play()
            {
                while (true)
                {
                    iODevice.Output("");
                    iODevice.Output(board.ToString());
                    iODevice.Output("");

                    int move = activePlayer.RequestMove(board);

                    if (!board.MakePlay(activePlayer.Color, move, out board))
                    {
                        iODevice.Output("Row is full. Try again.");
                        continue;
                    }

                    if (Judge.CheckForVictory(activePlayer.Color, board))
                    {
                        iODevice.Output(board.ToString());
                        iODevice.Output("");

                        if (activePlayer == computerPlayer)
                        {
                            iODevice.Accept("I'm sorry player {0}. I won again...", humanPlayer.Color);
                        }
                        else
                        {
                            iODevice.Accept("Congratulations player {0}! ¡You won!", humanPlayer.Color);
                        }
                        break;
                    }

                    if (board.NumberOfEmptyCells == 0)
                    {
                        iODevice.Output(board.ToString());
                        iODevice.Output("");
                        iODevice.Accept("¡Draw! I didnt loose...again");
                        break;
                    }

                    changeActivePlayer();
                }
            }
        }
    }

    internal class Player
    {
        public static Player CreateHumanPlayer(ActivePlayer color, IODevice iODevice)
        {
            return new HumanConsolePlayer(color, iODevice);
        }

        public static Player CreateComputerPlayer(ActivePlayer color, DifficultyLevel difficultyLevel, IODevice iODevice)
        {
            return new ComputerConsolePlayer(color, difficultyLevel, iODevice);
        }

        private readonly ActivePlayer playerColor;
        private readonly IODevice iODevice;

        private Player(ActivePlayer colorJugador, IODevice iODevice)
        {
            if (colorJugador != ActivePlayer.Red && colorJugador != ActivePlayer.Yellow)
                throw new ArgumentOutOfRangeException("playerColor");

            this.playerColor = colorJugador;
            this.iODevice = iODevice;
        }

        public ActivePlayer Color { get { return playerColor; } }

        public virtual int RequestMove(Board tablero)
        {
            return -1;
        }

        private class ComputerConsolePlayer : Player
        {
            private readonly AIEngine engine;

            public ComputerConsolePlayer(ActivePlayer color, DifficultyLevel difficulty, IODevice iODevice)
                : base(color, iODevice)
            {
                engine = new AIEngine(difficulty);
            }

            public override int RequestMove(Board board)
            {
                Debug.Assert(board != null);

                var move = engine.GetBestMove(board, playerColor);
                iODevice.Output("Player {0}'s turn. Hmmm...I'll play: {1}", playerColor, move);
                iODevice.Output("");
                return move;
            }
        }

        private class HumanConsolePlayer : Player
        {
            public HumanConsolePlayer(ActivePlayer color, IODevice iOdevice)
                : base(color, iOdevice) { }

            public override int RequestMove(Board tablero)
            {
                Debug.Assert(tablero != null);

                while (true)
                {
                    var input = (string)iODevice.Request("Player {0}'s turn: ", playerColor);
                    iODevice.Output("");
                    int jugada = -1;

                    if (int.TryParse(input, out jugada))
                    {
                        if (jugada < 0 || jugada >= Board.BoardColumns)
                        {
                            iODevice.Output("Column number must be within 0 and 6. Try again.");
                            continue;
                        }
                        else
                        {
                            return jugada;
                        }
                    }
                    else
                    {
                        iODevice.Output("'{0}' is not a column number. Try again.", input);
                    }
                }
            }
        }
    }

    public sealed class Board
    {
        public const int BoardRows = 6, BoardColumns = 7;
        public static readonly Board EmptyBoard = new Board();

        private readonly CellStates[,] cells;
        private readonly int numberOfEmptyCells;

        private Board()
        {
            cells = new CellStates[BoardRows, BoardColumns];
            numberOfEmptyCells = BoardRows * BoardColumns;
        }

        private Board(Board board, int numberOfEmptyCells)
        {
            if (board == null)
                throw new ArgumentNullException("board");

            if (numberOfEmptyCells < 0 || numberOfEmptyCells > BoardRows * BoardColumns)
                throw new ArgumentOutOfRangeException("numberOfEmptyCells");

            cells = new CellStates[BoardRows, BoardColumns];

            if (board != null)
            {
                for (int i = 0; i < BoardRows; i++)
                {
                    for (int j = 0; j < BoardColumns; j++)
                    {
                        cells[i, j] = board.cells[i, j];
                    }
                }
            }

            this.numberOfEmptyCells = numberOfEmptyCells;
        }

        public int NumberOfEmptyCells
        {
            get
            {
                return numberOfEmptyCells;
            }
        }

        public CellStates GetCellState(int row, int column)
        {
            if (row < 0 || row >= BoardRows)
                throw new ArgumentOutOfRangeException("row");

            if (column < 0 || column >= BoardColumns) throw new ArgumentOutOfRangeException("column");

            return cells[row, column];
        }

        public bool MakePlay(ActivePlayer player, int column, out Board board)
        {
            if (column < 0 || column >= BoardColumns) throw new ArgumentOutOfRangeException("column");

            if (cells[0, column] != CellStates.Empty)
            {
                board = this;
                return false;
            }

            board = new Board(this, numberOfEmptyCells - 1);

            int i;

            for (i = BoardRows - 1; i > -1; i--)
            {
                if (cells[i, column] == CellStates.Empty)
                    break;
            }

            board.cells[i, column] = (CellStates)player;
            return true;
        }

        [DebuggerStepThrough]
        public override string ToString()
        {
            var builder = new StringBuilder();
            var header = "  0   1   2   3   4   5   6";
            var divisor = "-----------------------------";
            builder.AppendLine(header);
            builder.AppendLine(divisor);

            for (int i = 0; i < cells.GetLength(0); i++)
            {
                for (int j = 0; j < cells.GetLength(1); j++)
                {
                    var str = cells[i, j] == CellStates.Empty ? "| · " : (cells[i, j] == CellStates.Red ? "| X " : "| O ");
                    builder.Append(str);
                }

                builder.Append('|');
                builder.AppendLine();
                builder.AppendLine(divisor);
            }

            return builder.ToString(0, builder.Length - 1);
        }
    }

    public class IODevice
    {
        public static IODevice CreateConsoleDevice() { return new ConsoleDevice(); }
        private IODevice()
        {
        }

        public virtual void Output(string message, params object[] arguments)
        {
            throw new NotImplementedException();
        }

        public virtual object Request(string message, params object[] arguments)
        {
            throw new NotImplementedException();
        }

        public virtual void Accept(string message, params object[] arguments)
        {
            throw new NotImplementedException();
        }

        private class ConsoleDevice : IODevice
        {
            public override void Output(string message, params object[] arguments)
            {
                Console.WriteLine(string.Format(message, arguments));
            }

            public override object Request(string message, params object[] arguments)
            {
                Console.Write(string.Format(message, arguments));
                return Console.ReadLine();
            }

            public override void Accept(string message, params object[] arguments)
            {
                Console.Write(string.Format(message, arguments));
                Console.ReadKey();
            }
        }
    }

    public enum ActivePlayer
    {
        Yellow = 1,
        Red = 2
    }

    public enum CellStates
    {
        Empty = 0,
        Yellow = ActivePlayer.Yellow,
        Red = ActivePlayer.Red
    }

    public enum DifficultyLevel
    {
        Easy = 1,
        Medium = 3,
        Hard = 4
    }
}
\$\endgroup\$
  • 3
    \$\begingroup\$ Is there any chance you could translate the code to English? A lot of reviewers are English and your question is also phrased in English. Some context might be lost via google translate. I appreciate this is a lot of work so if not we'll just have to wait until someone fluent in Spanish comes along :-) \$\endgroup\$ – Dan Pantry Jul 10 '15 at 23:18
  • \$\begingroup\$ @DanPantry I'm not in favour of asking people to translate their code just to have it reviewed. Something tends to get lost in translation. \$\endgroup\$ – 200_success Jul 10 '15 at 23:19
  • \$\begingroup\$ OK, my bad then! Sorry. \$\endgroup\$ – Dan Pantry Jul 10 '15 at 23:19
  • 1
    \$\begingroup\$ @DanPantry Nah, its no bother. The code is not that long and if any of you is going to spend some of their time looking through my code, the least I can do is make it easier. I've updated the question with code in english. \$\endgroup\$ – InBetween Jul 10 '15 at 23:47
  • \$\begingroup\$ Dittos, @DanPantry. Doesn't hurt to ask. The Code Review forum owes each OP a quality effort. Good code imbues intent. It declares, defines, and manipulates a domain (connect-4 in this case) and helps the reader understand that domain and the implementation. Done exceptionally well, the code "reads in the domain language". How could a non spanish speaker/reader be expected to review the code? And besides one of the rules is that questions be in English. \$\endgroup\$ – radarbob Jul 11 '15 at 3:32
4
\$\begingroup\$

Many of the loops can be replaced with LINQ to increase readability and safety. Also, method names, even private ones, should be UpperCamelCase. Let's first look at the getPossibleMoves method:

private List<int> getPossibleMoves(Node node)
{
    var moves = new List<int>();

    for (int i = 0; i < Board.BoardColumns; i++)
    {
        if (node.Board.GetCellState(0, i) == CellStates.Empty)
        {
            moves.Add(i);
        }
    }

    return moves;
}

The method body can be made into a one-liner using LINQ:

private List<int> getPossibleMoves(Node node)
{
    return Enumerable.Range(0, Board.BoardColumns)
                     .Where(x => node.Board.GetCellState(0, x) == CellStates.Empty)
                     .ToList();
}

The following at the end of the scoreNode method:

foreach (var varianteContrincante in nodo.Variants)
{
    score += scoreNode(varianteContrincante, player, depth + 1);
}

can be replaced with the following LINQ:

score += nodo.Variants.Sum(x => scoreNode(x, player, depth + 1));

Let's look at the CheckForVictory method:

public static bool CheckForVictory(ActivePlayer player, Board tablero)
{
    if (tablero == null)
        throw new ArgumentNullException("board");

    for (int i = 0; i < Board.BoardRows; i++)
    {
        for (int j = 0; j < Board.BoardColumns; j++)
        {
            if (tablero.GetCellState(i, j) == (CellStates)player)
            {
                if (checkForVictory(tablero, i, j))
                    return true;
            }
        }
    }

    return false;
}

The for loops can be replaced using LINQ:

public static bool CheckForVictory(ActivePlayer player, Board tablero)
{
    if (tablero == null)
        throw new ArgumentNullException("board");

    return
       (from x in Enumerable.Range(0, Board.BoardRows)
        from y in Enumerable.Range(0, Board.BoardColumns)
        where tablero.GetCellState(x, y) == (CellStates)player &&
              checkForVictory(tablero, x, y)
        select true).Any();
}

The checkForVictory method has a lot of repetition in it. Here's my try at reducing the repetition. Some might consider my version ugly. Go with what is more readable to you.

private static bool checkForVictory(Board board, int fila, int columna)
{
    bool searchRight = columna <= Board.BoardColumns - requiredCellsInARow;
    bool searchLeft = columna >= requiredCellsInARow - 1;
    bool searchUp = fila > Board.BoardRows - requiredCellsInARow;
    bool searchDown = fila <= Board.BoardRows - requiredCellsInARow;

    var xsRight = Enumerable.Range(columna, 4).ToList();
    var xsLeft = Enumerable.Range(0, 4).Select(x => columna - x).ToList();
    var ysDown = Enumerable.Range(fila, 4).ToList();
    var ysUp = Enumerable.Range(0, 4).Select(x => fila - x).ToList();

    return
        (searchRight && checkCells(xsRight.Select(x => board.GetCellState(fila, x)).ToArray())) ||
        (searchLeft && checkCells(xsLeft.Select(x => board.GetCellState(fila, x)).ToArray())) ||
        (searchUp && checkCells(ysUp.Select(y => board.GetCellState(y, columna)).ToArray())) ||
        (searchDown && checkCells(ysDown.Select(y => board.GetCellState(y, columna)).ToArray())) ||
        (searchLeft && searchUp && CheckCells(board, ysUp, xsLeft)) ||
        (searchLeft && searchDown && CheckCells(board, ysDown, xsLeft)) ||
        (searchRight && searchUp && CheckCells(board, ysUp, xsRight)) ||
        (searchRight && searchDown && CheckCells(board, ysDown, xsRight));
}

with the helper method:

private static bool CheckCells(Board board, IEnumerable<int> rows, IEnumerable<int> columns)
{
    return checkCells(rows.Zip(columns, (y, x) => board.GetCellState(y, x)).ToArray());
}

The checkCells method can be rewritten using LINQ.

private static bool checkCells(params CellStates[] celdas)
{
    Debug.Assert(celdas.Length == requiredCellsInARow);

    for (int i = 1; i < requiredCellsInARow; i++)
    {
        if (celdas[i] != celdas[0])
            return false;
    }

    return true;
}

Here is the LINQ version:

private static bool checkCells(params CellStates[] celdas)
{
    Debug.Assert(celdas.Length == requiredCellsInARow);

    return celdas.Skip(1).All(x => x == celdas[0]);
}

On to the changeActivePlayer method:

private void changeActivePlayer()
{
    if (activePlayer == humanPlayer)
    {
        activePlayer = computerPlayer;
    }
    else
    {
        activePlayer = humanPlayer;
    }
}

I prefer to use expressions instead of statements when possible. In this case we can replace the if statement with an expression using the conditional operator:

private void changeActivePlayer()
{
    activeplayer = activePlayer == humanPlayer ? computerPlayer : humanPlayer;
}

You can also change the following code in the ConsoleGame constructor:

if (computerPlaysFirst)
{
    activePlayer = computerPlayer;
}
else
{
    activePlayer = humanPlayer;
}

with:

activePlayer = computerPlaysFirst ? computerPlayer : humanPlayer;
\$\endgroup\$
1
\$\begingroup\$

Your IODevice class is breaking Single Responsibility. At the moment it is a device and also is responsible to create other devices. Move the creation of devices into a factory class:

public class DeviceFactory
{
    public static IODevice CreateDevice<T>() where T : IODevice, new()
    {
        return new T();
    }
}

and then use it as: DeviceFactory.CreateDevice()

Similarly, move the creation of player out from the Player class to its own factory. Because the player class accepts arguments, you have to pass the args to create an instance in the Factory, like below:

public class PlayerFactory
{
    public static Player CreatePlayer<T>(ActivePlayer color, IODevice iODevice, DifficultyLevel difficultyLevel)
        where T : Player, new()
    {
        return (T)Activator.CreateInstance(typeof(T), new object[] { color, iODevice, difficultyLevel });
    }
}

Make sure that each child class must have a parameterless constructor. So, Player and all its derived classes should have a parameterless constructor.

Then use the factory as:

PlayerFactory.CreatePlayer<ComputerConsolePlayer>(computerColor, iODevice, difficultyLevel);
PlayerFactory.CreatePlayer<HumanConsolePlayer>(computerColor, iODevice, difficultyLevel);

In a few places, you are instantiating objects by using the new keyword. I suggest using a Dependency Injection like Unity or Windsor.

\$\endgroup\$

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.