11
\$\begingroup\$

I've started learning MonoGame and C# lately, so in order to get more confident with both and do something not completely trivial, I've built a Naughts and Crosses game. I am using 7.1 version of C# (value tuples here and there...)

The game does the following:

  • Plays Tic-Tac-Toe (AI is unbeatable)
  • Keeps score for player and the computer
  • For now it is a sort of endless match.

The whole project itself can be found at github. Here is a screenshot of the game.Screenshot

I have a few questions. The whole thing seems too big and bloated, although I did try not to repeat myself too much. Is there an idiomatic way to reduce its size?

About the comments in general - Most of my functions are short enough, and taking in account my horrible verbal skills, most of code is still self explanatory. However, I did try to provide XML documentation for all of the functions - Is it considered good practice? Or it is should be done only for the functions I expect to use in future?

About the access level - When should I make a class public and where should I leave its visibility as default? If I've got it right - public classes will be available outside of the project - however mixing public classes with non-public sometimes would not compile.

Static functions/classes - should I have them at all?

Properties with setters/getters, attributes vs properties - when should each one be used? When I had an object exposed by objects I've tried to expose it as a property - is it right way to do it?

In general - I would like to hear how in general this code could be simplified / reorganized / improved while keeping the functionality.

Here's the code itself. Assume that all the classes live in namespace TicTacToe and have the following lines in the beginning of each file:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using Microsoft.Xna.Framework;
using Microsoft.Xna.Framework.Graphics;
using Microsoft.Xna.Framework.Input;

My game is devided into following classes:

TicTacToeGame.cs (Replaces Game1 from MonoGame template - the comments are automatically generated by the template - I've left them as they are for now, because I don't have much to add right now, and anyway I've used it mostly as a wrapper class).

/// <summary>
/// A class to play tic tac toe
/// </summary>
public class TicTacToeGame : Game
{
    GraphicsDeviceManager graphics;
    public SpriteBatch SpriteBatch;
    GameManager gameManager;
    int windowWidth = 1280;
    int windowHeight = 780;

    public TicTacToeGame()
    {
        graphics = new GraphicsDeviceManager(this);
        Content.RootDirectory = "Content";
        graphics.PreferredBackBufferWidth = windowWidth;
        graphics.PreferredBackBufferHeight = windowHeight;
    }

    /// <summary>
    /// Allows the game to perform any initialization it needs to before starting to run.
    /// This is where it can query for any required services and load any non-graphic
    /// related content.  Calling base.Initialize will enumerate through any components
    /// and initialize them as well.
    /// </summary>
    protected override void Initialize()
    {
        this.IsMouseVisible = true;
        base.Initialize();
    }

    /// <summary>
    /// LoadContent will be called once per game and is the place to load
    /// all of your content.
    /// </summary>
    protected override void LoadContent()
    {
        // Create a new SpriteBatch, which can be used to draw textures.
        SpriteBatch = new SpriteBatch(GraphicsDevice);
        gameManager = new GameManager(this);

        // TODO: use this.Content to load your game content here
    }

    /// <summary>
    /// UnloadContent will be called once per game and is the place to unload
    /// game-specific content.
    /// </summary>
    protected override void UnloadContent()
    {
        // TODO: Unload any non ContentManager content here
    }

    /// <summary>
    /// Allows the game to run logic such as updating the world,
    /// checking for collisions, gathering input, and playing audio.
    /// </summary>
    /// <param name="gameTime">Provides a snapshot of timing values.</param>
    protected override void Update(GameTime gameTime)
    {
        if (GamePad.GetState(PlayerIndex.One).Buttons.Back == ButtonState.Pressed || Keyboard.GetState().IsKeyDown(Keys.Escape))
            Exit();

        if (Keyboard.GetState().IsKeyDown(Keys.F10))
        {
            if (!graphics.IsFullScreen)
                graphics.IsFullScreen = true;
            else
                graphics.IsFullScreen = false;
            graphics.ApplyChanges();
        }

        gameManager.Update();


        base.Update(gameTime);
    }

    /// <summary>
    /// This is called when the game should draw itself.
    /// </summary>
    /// <param name="gameTime">Provides a snapshot of timing values.</param>
    protected override void Draw(GameTime gameTime)
    {
        GraphicsDevice.Clear(Color.CornflowerBlue);

        // TODO: Add your drawing code here
        SpriteBatch.Begin();

        gameManager.Draw();

        SpriteBatch.End();

        base.Draw(gameTime);
    }
}

GameManager.cs - responsible for connecting the game pieces

/// <summary>
/// An object repsonsible for controlling the game
/// </summary>
public class GameManager
{
    public TicTacToeGame TheGame { get; private set; }
    public GameLogic Logic { get; private set; }
    public GameIO IO { get; private set; }
    public GameBoard Board { get; private set; }
    private IPlayer player1;
    private IPlayer player2;

    /// <summary>
    /// Allocates memory for object - used to avoid null reference errors
    /// while initializing the fields of the the object
    /// </summary>
    private GameManager()
    {
        TheGame = null;
        Logic = null;
        IO = null;
        Board = null;
    }



    /// <summary>
    /// The game to which the given Manager belongs to.
    /// </summary>
    /// <param name="ticTacToeGame"></param>
    public GameManager(TicTacToeGame ticTacToeGame) : this()
    {
        this.TheGame = ticTacToeGame;
        Board = new GameBoard();
        Logic = new GameLogic(this);
        IO = new GameIO(this);
        player1 = new HumanPlayer("Player", CrossesOrNoughts.Crosses, this, new ScoreCalculator(10, 1, -10));
        player2 = new ComputerPlayer("Computer", CrossesOrNoughts.Naughts, this, new ScoreCalculator(10, 0, -10));
        Logic.AddPlayers(player1, player2);
    }

    public void Reset()
    {
        Board = new GameBoard();
        Logic = new GameLogic(this);

        CrossesOrNoughts tempSymbol = player1.Symbol();
        player1.SetSymbol(player2.Symbol());
        player2.SetSymbol(tempSymbol);

        IPlayer tempPlayer = player1;
        player1 = player2;
        player2 = tempPlayer;

        Logic.AddPlayers(player1, player2);
    }

    /// <summary>
    /// Update the game state
    /// </summary>
    public void Update()
    {
        IO.Update();
    }

    /// <summary>
    /// Display the board on the screen.
    /// </summary>
    public void Draw()
    {
        IO.Draw();
    }

}

GameIO.cs - responsible for interaction with the game - namely graphical output and user input

/// <summary>
/// The class repsonsible for the graphics part of the game
/// </summary>
public class GameIO
{
    GameManager gameManager;
    TicTacToeGame TheGame => gameManager.TheGame;
    GameLogic Logic => gameManager.Logic;
    GameBoard Board => gameManager.Board;
    Vector2 TopLeft => WindowSize * 0.05f;    // top left position of the board
    Vector2 SquareSize => WindowSize / 5;
    Vector2 BoardSize => SquareSize * 3f;
    Vector2 WindowSize => new Vector2(TheGame.GraphicsDevice.Viewport.Width, TheGame.GraphicsDevice.Viewport.Height);
    Texture2D background;                                // background texture
    Texture2D tableBorders;                              // borders between the squares
    Texture2D xImage;                                    // Crosses image
    Texture2D oImage;                                    // Naughts imaage
    Texture2D horizontalLine;                            // Horizontal line image
    Texture2D verticalLine;                              // vertical line image    
    Texture2D westEastDiagonal;                          // an image of diagonal from topleft to buttom right
    Texture2D eastWestDiagonal;                          // an image of diagonal from topright to butttom left
    GameMessage gameMessage;

    private GameIO() { }

    public GameIO(GameManager gameManager)
    {
        this.gameManager = gameManager;
        background = TheGame.Content.Load<Texture2D>("Background");
        tableBorders = TheGame.Content.Load<Texture2D>("TableBorders");
        xImage = TheGame.Content.Load<Texture2D>("X");
        oImage = TheGame.Content.Load<Texture2D>("O");
        horizontalLine = TheGame.Content.Load<Texture2D>("HorizontalLine");
        verticalLine = TheGame.Content.Load<Texture2D>("VerticalLine");
        westEastDiagonal = TheGame.Content.Load<Texture2D>("WestEastDiagonal");
        eastWestDiagonal = TheGame.Content.Load<Texture2D>("EastWestDiagonal");
        gameMessage = new GameMessage(gameManager);
    }


    /// <summary>
    /// Draws a square image on the screen
    /// </summary>
    /// <param name="image">Texture name</param>
    /// <param name="topPosition">Upper border of the image position</param>
    /// <param name="leftPosition">Left border of the image</param>
    /// <param name="height">Height of the image</param>
    /// <param name="width">Widht of the image</param>
    void DrawSquare(Texture2D image, float topPosition, float leftPosition, float width, float height)
    {
        Rectangle destination = new Rectangle((int)topPosition, (int)leftPosition, (int)width, (int)height);
        TheGame.SpriteBatch.Draw(image, destination, Color.White);
    }

    /// <summary>
    /// Draws the back ground of the table
    /// </summary>
    void DrawBackground()
    {
        DrawSquare(background, TopLeft.X, TopLeft.Y, BoardSize.X, BoardSize.Y);
        DrawSquare(tableBorders, TopLeft.X, TopLeft.Y, BoardSize.X, BoardSize.Y);
    }

    /// <summary>
    /// Fills the squares in the game table
    /// </summary>
    void DrawSquares()
    {
        for (int i = 0; i < 3; ++i)
            for (int j = 0; j < 3; ++j)
            {
                Texture2D filling;
                if (Board[i, j] == CrossesOrNoughts.Crosses)
                    filling = TheGame.Content.Load<Texture2D>("X");
                else if (Board[i, j] == CrossesOrNoughts.Naughts)
                    filling = TheGame.Content.Load<Texture2D>("O");
                else filling = null;

                if (filling != null)
                    DrawSquare(filling, TopLeft.X + i * SquareSize.X, TopLeft.Y + j * SquareSize.Y,
                        SquareSize.X, SquareSize.Y);
            }
    }

    /// <summary>
    /// Marks with a line the rows that are all either noughts or crosses
    /// </summary>
    void MarkRows()
    {
        for (int i = 0; i < 3; ++i)
        {
            if (Board.IsRowTaken(i))
            {
                DrawSquare(horizontalLine, TopLeft.X, TopLeft.Y + SquareSize.Y * i, BoardSize.X, SquareSize.Y);
            }
        }
    }

    /// <summary>
    /// Marks the collumns that are all either noughts or crosses
    /// </summary>
    void MarkColumns()
    {
        for (int i = 0; i < 3; ++i)
        {
            if (Board.IsColumnTaken(i))
            {
                DrawSquare(verticalLine, TopLeft.X + SquareSize.X * i, TopLeft.Y, SquareSize.X, BoardSize.Y);
            }
        }
    }

    /// <summary>
    /// Marks the main if it contains all noughts or crosses
    /// </summary>
    void MarkDiagonals()
    {
        if (Board.IsMainDiagonalTaken())
            DrawSquare(westEastDiagonal, TopLeft.X, TopLeft.Y, BoardSize.X, BoardSize.Y);
        if (Board.IsSecondaryDiagonalTaken())
            DrawSquare(eastWestDiagonal, TopLeft.X, TopLeft.Y, BoardSize.X, BoardSize.Y);
    }


    /// <summary>
    /// Draws the game board
    /// </summary>
    public void Draw()
    {
        DrawBackground();
        DrawSquares();
        MarkRows();
        MarkColumns();
        MarkDiagonals();
        PrintScores();

        if (Logic.State == GameLogic.GameState.Over)
        {
            DeclareWinner();
            RestartMessage();
        }
    }

    /// <summary>
    /// Translates 2 dimensional vector to position on the board
    /// </summary>
    /// <param name="clickPosition"></param>
    /// <returns></returns>
    public (int row, int column) PositionOnBoard(Vector2 clickPosition)
    {
        return ((int)((clickPosition.X - TopLeft.X) / SquareSize.X),
                (int)((clickPosition.Y - TopLeft.Y) / SquareSize.Y));
    }

    /// <summary>
    /// Processes mouse input from the user
    /// </summary>
    public void ProcessMouseInput()
    {
        MouseState mouseState = Mouse.GetState();

        if (mouseState.LeftButton == ButtonState.Pressed)
        {
            (int row, int column) = PositionOnBoard(new Vector2(mouseState.X, mouseState.Y));
            Logic.Update(row, column);
        }
    }

    /// <summary>
    /// Processes move that was entered as a pair of numbers
    /// </summary>
    /// <param name="row">Row number</param>
    /// <param name="column">Column number</param>
    public void ProcessDigitalInput(int row, int column)
    {
        Logic.Update(row, column);
    }

    /// <summary>
    /// Get input from player and update the state of the game
    /// </summary>
    public void Update()
    {
        if (Logic.State == GameLogic.GameState.Continue) Logic.CurrentPlayer.MakeMove();
        if (Logic.State == GameLogic.GameState.Over)
        {
            if (Keyboard.GetState().IsKeyDown(Keys.Space))
                gameManager.Reset();
        }
    }

    /// <summary>
    /// Print player scores
    /// </summary>
    private void PrintScores()
    {
        gameMessage.PrintMessageAt(new Vector2(TopLeft.X, TopLeft.Y + BoardSize.Y + 20),
                                   $"{Logic.player1.Name()}: {Logic.player1.Score()}");
        gameMessage.PrintMessageAt(new Vector2(TopLeft.X, TopLeft.Y + BoardSize.Y + 70),
                                   $"{Logic.player2.Name()}: {Logic.player2.Score()}");
    }

    private void DeclareWinner()
    {
        gameMessage.PrintMessageAt(new Vector2(TopLeft.X, TopLeft.Y + BoardSize.Y + 120),
                                  $"The winner is {Logic.Winner}");
    }

    private void RestartMessage()
    {
        gameMessage.PrintMessageAt(new Vector2(TopLeft.X, TopLeft.Y + BoardSize.Y + 170),
                                   "Press space to continue");
    }

}

GameLogic.cs - responsible for handling the game state - determining the winner and processing the user input.

/// <summary>
///  A class respsonsible to the logic of the game. Used to determine winner and control the turns.
/// </summary>
public class GameLogic
{
    /// <summary>
    /// The state of the game - whether it is over or not.
    /// </summary>
    public enum GameState { Continue, Over};
    private GameManager gameManager;
    GameBoard board => gameManager.Board;
    public IPlayer player1 { get; private set; }
    public IPlayer player2 { get; private set; }
    public IPlayer CurrentPlayer { get;  private set; }
    public CrossesOrNoughts Winner { get; private set; }
    public GameState State { get; private set; }

    /// <summary>
    /// Creates a new game logic object and associates it with the gameManager object
    /// </summary>
    /// <param name="gameManager">Game manager object to associate with</param>
    public GameLogic(GameManager gameManager)
    {
        this.gameManager = gameManager;
        this.State = GameState.Continue;
        player1 = null;
        player2 = null;
        CurrentPlayer = player1;
    }

    /// <summary>
    /// Adds a player to the game
    /// </summary>
    /// <param name="player1"></param>
    /// <param name="player2"></param>
    public void AddPlayers(IPlayer player1, IPlayer player2)
    {
        if (this.player1 == null) this.player1 = player1;
        if (this.player2 == null) this.player2 = player2;
        CurrentPlayer = player1;
    }

    /// <summary>
    /// Determines result of the game state determined by internal board object
    /// </summary>
    /// <returns>Whehter the game is over and the winner symbol</returns>
    private (GameState, CrossesOrNoughts) DetermineResult() => DetermineResult(this.board);

    /// <summary>
    /// Calculates the state and the result of the game at the moment represented by the board.
    /// </summary>
    /// <param name="board"></param>
    /// <returns>Wheher the game is over and who is the winner in case it i. I
    /// f it is not over - Niether is retunred.</returns>
    public static (GameState state, CrossesOrNoughts winner) DetermineResult(GameBoard board)
    {
        // go over rows colums and diagonals to che k whether the game is over and we have a winner.
        // After that - check if the board is full
        for(int i = 0; i < 3; ++i)
        {
            if (board.IsRowTaken(i))
                return (GameState.Over, board[0, i]);
            if (board.IsColumnTaken(i))
                return (GameState.Over, board[i, 0]);
        }

        if (board.IsMainDiagonalTaken())
            return (GameState.Over, board[0, 0]);
        if (board.IsSecondaryDiagonalTaken())
            return (GameState.Over, board[2, 0]);
        if (board.IsFull())
            return (GameState.Over, CrossesOrNoughts.Neither);

        return (GameState.Continue, CrossesOrNoughts.Neither);
    }

    /// <summary>
    /// Change the player
    /// </summary>
    void UpdatePlayer()
    {
        CurrentPlayer = (CurrentPlayer == player1) ? player2 : player1;
    }

    /// <summary>
    /// Checks whether position is legal or if it is taken and puts appropriate player sign on it if the game is not over.
    /// After performing player move, updates the player if it the game is not over.
    /// </summary>
    /// <param name="row"></param>
    /// <param name="column"></param>
    public void Update(int row, int column)
    {
        if (board.ShouldUpdate(row, column) && State == GameState.Continue)
        {
            board[row, column] = CurrentPlayer.Symbol();
            (State, Winner) = DetermineResult();

            if (State == GameState.Continue) UpdatePlayer();
            else
            {
                player1.UpdateScore(Winner);
                player2.UpdateScore(Winner);
            }
        }
    }

    /// <summary>
    /// Calculates the symbol used by opponent player
    /// </summary>
    /// <param name="playerSymbol">Returns the symbol used by the opponent</param>
    /// <returns></returns>
    public static CrossesOrNoughts OpponentSymbol(CrossesOrNoughts playerSymbol)
    {
        if (playerSymbol == CrossesOrNoughts.Crosses) return CrossesOrNoughts.Naughts;
        if (playerSymbol == CrossesOrNoughts.Naughts) return CrossesOrNoughts.Crosses;
        else return CrossesOrNoughts.Neither;
    }
}

Board.cs - represents the board in the game and handles "geometric" and "physical" parts - namely can we put a piece on the board, or do we have lines or diagonals of same type, etc...

/// <summary>
/// Board to represent board in Noughts and crosses game
/// </summary>
public class GameBoard
{    
    private CrossesOrNoughts[,]  entries;

    /// <summary>
    /// Checks whether the position is within bounds
    /// </summary>
    /// <param name="row">Position row</param>
    /// <param name="column">Position column</param>
    /// <returns>True if position is within bounds, false otherwise</returns>
    public bool IsValidPosition(int row, int column) => !(row < 0 || row >= 3 || column < 0 || column >= 3);
    public bool IsFree(int row, int column) => !(entries[row, column] == CrossesOrNoughts.Neither);
    public bool ShouldUpdate(int row, int column) => 
        IsValidPosition(row, column) && (entries[row, column] == CrossesOrNoughts.Neither);

    /// <summary>
    /// The construtor - accepts no arguments and creates 3 on 3 empty board
    /// </summary>
    public GameBoard()
    {
        entries = new CrossesOrNoughts[3, 3];
    }

    /// <summary>
    /// Indexer - returns the square at the given position
    /// </summary>
    /// <param name="row">Position row</param>
    /// <param name="column">Position column</param>
    /// <returns>Position entry</returns>
    public CrossesOrNoughts this[int row, int column]
    {
        get
        {
            if (IsValidPosition(row, column))
                return entries[row, column];
            else throw new IndexOutOfRangeException();
        }
        set
        {
            if (IsValidPosition(row, column))
                entries[row, column] = value;
            else throw new IndexOutOfRangeException();
        }
    }

    /// <summary>
    /// Returns whether the entries in the array are same are either noughts or crosses
    /// </summary>
    /// <param name="crossesOrNoughts">The array os crosses or nouhgts</param>
    /// <returns>True if they are all same or false otherwise</returns>
    private bool HaveSameSign(params CrossesOrNoughts[] crossesOrNoughts)
    {
        for (int i = 0; i < crossesOrNoughts.Length - 1; ++i)
        {
            if (crossesOrNoughts[i] == CrossesOrNoughts.Neither) return false;
            if (crossesOrNoughts[i] != crossesOrNoughts[i + 1]) return false;
        }
        return true;
    }

    /// <summary>
    /// Returns the entries in the given row.
    /// </summary>
    /// <param name="row">Row numbers</param>
    /// <returns>The row entries in array form</returns>
    private CrossesOrNoughts[] TableRow(int row)
    {
        CrossesOrNoughts[] result = new CrossesOrNoughts[entries.GetLength(1)];
        for(int i = 0; i < result.Length; ++i)
        {
            result[i] = entries[i, row];
        }

        return result;
    }

    /// <summary>
    /// Returns the entries in the given column
    /// </summary>
    /// <param name="column">Column number</param>
    /// <returns>The column entries in array form</returns>
    private CrossesOrNoughts[] TableColumn(int column)
    {
        CrossesOrNoughts[] result = new CrossesOrNoughts[entries.GetLength(0)];

        for(int i = 0; i < result.Length; ++i)
        {
            result[i] = entries[column, i];
        }

        return result;
    }

    /// <summary>
    /// Returns the entries in diagonal from top left corner to buttom right corner
    /// </summary>
    /// <returns>Entries of the main diagonal in array form</returns>
    private CrossesOrNoughts[] MainDiagonal()
    {
        CrossesOrNoughts[] result = new CrossesOrNoughts[entries.GetLength(0)];
        for (int i = 0; i < result.Length; ++i)
            result[i] = entries[i, i];

        return result;
    }

    /// <summary>
    /// Return  the entries in the diagonal from buttom left to upper right corner
    /// </summary>
    /// <returns>The entries of the secondary diagonal in array form</returns>
    private CrossesOrNoughts[] SecondaryDiagonal()
    {
        CrossesOrNoughts[] result = new CrossesOrNoughts[entries.GetLength(0)];
        for (int i = 0; i < result.Length; ++i)
            result[i] = entries[result.Length - 1 - i, i];

        return result;
    }

    /// <summary>
    /// Checks whether the board is full
    /// </summary>
    /// <returns></returns>
    public bool IsFull()
    {
        for(int i = 0; i < 3; ++i)
        {
            for(int j = 0; j < 3; ++j)
            {
                if (entries[i, j] == CrossesOrNoughts.Neither) return false;
            }
        }

        return true;
    }

    /// <summary>
    /// Checks whether the given row is full and contains same signs
    /// </summary>
    /// <param name="row"></param>
    /// <returns>True if the column entries are either all noughts or all rosses, false otherwise</returns>
    public bool IsRowTaken(int row) => HaveSameSign(TableRow(row));

    /// <summary>
    /// Checks whether the given column is marked and contains same signs
    /// </summary>
    /// <param name="column"></param>
    /// <returns>True if the column entries are either all noughts or all crosses, false otherwise</returns>
    public bool IsColumnTaken(int column) => HaveSameSign(TableColumn(column));

    /// <summary>
    /// Checks whether the main diagonal is marked and contains same signs
    /// </summary>
    /// <returns>True if all the entries in the main diagonal are either all noughts or all crosses, false otherwise</returns>
    public bool IsMainDiagonalTaken() => HaveSameSign(MainDiagonal());

    /// <summary>
    /// Checks whther the secondary diagonal is marked and contains same signs
    /// </summary>
    /// <returns>True if all the entries in the main diagonal are either all noughts or all crosses, false otherwise</returns>
    public bool IsSecondaryDiagonalTaken() => HaveSameSign(SecondaryDiagonal());
}

An IPlayer interface that represents players in the game

/// <summary>
/// An interface representing the player in the game.
/// </summary>
public interface IPlayer
{
    CrossesOrNoughts Symbol();
    void SetSymbol(CrossesOrNoughts symbol);
    void MakeMove();
    string Name();
    int Score();
    void UpdateScore(CrossesOrNoughts crossesOrNoughts);
}

The interface is implemented by two classes - HumanPlayer and ComputerPlayer

HumanPlayer.cs

/// <summary>
/// A class to represent human controlled player in the game
/// </summary>
public class HumanPlayer : IPlayer
{
    private string name;
    private CrossesOrNoughts symbol;
    private GameManager gameManager;
    private int score;
    private ScoreCalculator scoreCalculator;

    /// <summary>
    /// Creats an instance of player
    /// </summary>
    /// <param name="name">Player's name</param>
    /// <param name="symbol">What player puts on the board - crosses or nauughts</param>
    /// <param name="gameManager">Interface to the game</param>
    public HumanPlayer(String name, CrossesOrNoughts symbol, GameManager gameManager, ScoreCalculator scoreCalculator) 
    {
        this.name = name;
        this.symbol = symbol;
        this.gameManager = gameManager;
        this.score = 0;
        this.scoreCalculator = scoreCalculator;
    }

    /// <summary>
    /// make a Move in the game
    /// </summary>
    public void MakeMove() => gameManager.IO.ProcessMouseInput();

    /// <summary>
    /// The symbol used by player
    /// </summary>
    /// <returns>Returns whether the player puts crosses or naughts</returns>
    public CrossesOrNoughts Symbol() => symbol;

    /// <summary>
    ///  Player's name
    /// </summary>
    /// <returns>Player's name</returns>
    public string Name() => name;

    /// <summary>
    /// Score of the player
    /// </summary>
    /// <returns>The score of the player</returns>
    public int Score() => score;

    public void SetSymbol(CrossesOrNoughts symbol)
    {
        this.symbol = symbol;
    }

    /// <summary>
    /// Update the player's score
    /// </summary>
    /// <param name="winner">Current winner of the game</param>
    public void UpdateScore(CrossesOrNoughts winner)
    {
        if (winner == symbol) score += scoreCalculator.WinScore;
        if (winner == GameLogic.OpponentSymbol(symbol)) score += scoreCalculator.LoseScore;
        else score += scoreCalculator.DrawScore;
    }

}

ComputerPlayer.cs

/// <summary>
/// Represents computer controlled player - designed to always pick the best move
/// </summary>
public class ComputerPlayer : IPlayer
{
    private string name;
    private CrossesOrNoughts symbol;
    private GameManager gameManager;
    private int score;
    private ScoreCalculator scoreCalculator;

    public ComputerPlayer(string name, CrossesOrNoughts symbol, GameManager gameManager, ScoreCalculator scoreCalculator)
    {
        this.name = name;
        this.symbol = symbol;
        this.gameManager = gameManager;
        this.score = 0;
        this.scoreCalculator = scoreCalculator;
    }

    /// <summary>
    /// Symbol used by the player  - crosses or noughts
    /// </summary>
    /// <returns>The symbol used by the player</returns>
    public CrossesOrNoughts Symbol() => symbol;
    public string Name() => name;

    /// <summary>
    /// Calculates the best possible move and passes it to the IO
    /// </summary>
    public void MakeMove()
    {
        MoveAnalysis move = MoveCalculator.BestMove(symbol, gameManager.Board);
        gameManager.IO.ProcessDigitalInput(move.Row, move.Column);
    }

    /// <summary>
    /// The score of the player
    /// </summary>
    /// <returns></returns>
    public int Score() => score;

    public void SetSymbol(CrossesOrNoughts symbol)
    {
        this.symbol = symbol;
    }

    /// <summary>
    /// Update the player's score
    /// </summary>
    /// <param name="winner">Current winner of the game</param>
    public void UpdateScore(CrossesOrNoughts winner)
    {
        if (winner == symbol) score += scoreCalculator.WinScore;
        if (winner == GameLogic.OpponentSymbol(symbol)) score += scoreCalculator.LoseScore;
        else score += scoreCalculator.DrawScore;
    }

}

A helper class MoveCalculator for ComputerPlayer that allows to calculate the best move with helper class MoveAnalysis to keep the track of move statistics.

/// <summary>
/// Statistaics for the move - position and who's the winner if we make it
/// </summary>
class MoveAnalysis
{
    public int Row { get; set; }
    public int Column { get; set; }
    public CrossesOrNoughts Winner { get; set; }

    public MoveAnalysis(int row, int column, CrossesOrNoughts winner)
    {
        Row = row;
        Column = column;
        Winner = winner;
    }
}

/// <summary>
/// Static class used to calculate the optimal move
/// </summary>
static class MoveCalculator
{
    private static Random random = new Random();
    /// <summary>
    /// Calculate the result to which leads outting playerSymbol at gven position on a board
    /// assuming that both players play optimally. 
    /// </summary>
    /// <param name="row">Row position</param>
    /// <param name="column">Column position</param>
    /// <param name="board">The game board</param>
    /// <param name="playerSymbol">Symbol - either naughts or crosses.</param>
    /// <returns></returns>
    private static CrossesOrNoughts EvaluateMove(int row, int column, GameBoard board, CrossesOrNoughts playerSymbol)
    {
        // Sanity check - checks whether the the position is legal
        if (playerSymbol == CrossesOrNoughts.Neither || !board.IsValidPosition(row, column))
            throw new ArgumentOutOfRangeException("Player can be either Crosses or Naughts.");
        if (board[row, column] != CrossesOrNoughts.Neither)
            throw new IndexOutOfRangeException("Square already occupied.");

        /* Calculates the score recursively. 
         * We put the current player's sign on the board and check the result.
         * If the game is over we return the result of the game.
         * Otherwise, we go over all available positions and pick return the best score 
         * achivable by the opponent
         */
        board[row, column] = playerSymbol;
        (GameLogic.GameState state, CrossesOrNoughts winner) = GameLogic.DetermineResult(board);

        if (state == GameLogic.GameState.Over)
        {
            board[row, column] = CrossesOrNoughts.Neither;
            return winner;
        }

        CrossesOrNoughts bestOponentResult = playerSymbol;
        CrossesOrNoughts opponentSymbol = GameLogic.OpponentSymbol(playerSymbol);

        for (int i = 0; i < 3; ++i)
        {
            for (int j = 0; j < 3; ++j)
            {
                if (board[i, j] == CrossesOrNoughts.Neither)
                {
                    CrossesOrNoughts result = EvaluateMove(i, j, board, opponentSymbol);
                    if (result == opponentSymbol)
                        bestOponentResult = opponentSymbol;
                    else if (result == CrossesOrNoughts.Neither && bestOponentResult == playerSymbol)
                        bestOponentResult = CrossesOrNoughts.Neither;
                }
            }
        }

        board[row, column] = CrossesOrNoughts.Neither;
        return bestOponentResult;
    }

    /// <summary>
    /// Calculates the best move that can be maid by the player
    /// </summary>
    /// <param name="playerSymbol">Players symbol - either crosses or noughtss</param>
    /// <param name="board">the game board</param>
    /// <returns>Best move that player can make</returns>
    public static MoveAnalysis BestMove(CrossesOrNoughts playerSymbol, GameBoard board)
    {
        // list of possible moves
        List<MoveAnalysis> moves = new List<MoveAnalysis>();

        // go over all empty positions and them as possible moves
        for (int i = 0; i < 3; ++i)
        {
            for (int j = 0; j < 3; ++j)
            {
                if (board[i, j] == CrossesOrNoughts.Neither)
                {
                    CrossesOrNoughts winner = EvaluateMove(i, j, board, playerSymbol);
                    moves.Add(new MoveAnalysis(i, j, winner));
                }
            }
        }

        // determine the best possible move and result
        MoveAnalysis bestMove = moves[0];

        for (int i = 1; i < moves.Count; ++i)
        {
            if (moves[i].Winner == playerSymbol)
            {
                bestMove = moves[i];
            }

            else if (moves[i].Winner == CrossesOrNoughts.Neither && bestMove.Winner == GameLogic.OpponentSymbol(playerSymbol))
            {
                bestMove = moves[i];
            }
        }

        // randomize - make a list of best moves and chose the best one
        List<MoveAnalysis> bestMoves = new List<MoveAnalysis>();
        for(int i = 0; i < moves.Count; ++i)
        {
            if (moves[i].Winner == bestMove.Winner)
                bestMoves.Add(moves[i]);
        }

        return bestMoves[random.Next(bestMoves.Count)];
    }
}




/// <summary>
/// Statistaics for the move - position and who's the winner if we make it
/// </summary>
class MoveAnalysis
{
    public int Row { get; set; }
    public int Column { get; set; }
    public CrossesOrNoughts Winner { get; set; }

    public MoveAnalysis(int row, int column, CrossesOrNoughts winner)
    {
        Row = row;
        Column = column;
        Winner = winner;
    }
}

/// <summary>
/// Static class used to calculate the optimal move
/// </summary>
static class MoveCalculator
{
    private static Random random = new Random();
    /// <summary>
    /// Calculate the result to which leads outting playerSymbol at gven position on a board
    /// assuming that both players play optimally. 
    /// </summary>
    /// <param name="row">Row position</param>
    /// <param name="column">Column position</param>
    /// <param name="board">The game board</param>
    /// <param name="playerSymbol">Symbol - either naughts or crosses.</param>
    /// <returns></returns>
    private static CrossesOrNoughts EvaluateMove(int row, int column, GameBoard board, CrossesOrNoughts playerSymbol)
    {
        // Sanity check - checks whether the the position is legal
        if (playerSymbol == CrossesOrNoughts.Neither || !board.IsValidPosition(row, column))
            throw new ArgumentOutOfRangeException("Player can be either Crosses or Naughts.");
        if (board[row, column] != CrossesOrNoughts.Neither)
            throw new IndexOutOfRangeException("Square already occupied.");

        /* Calculates the score recursively. 
         * We put the current player's sign on the board and check the result.
         * If the game is over we return the result of the game.
         * Otherwise, we go over all available positions and pick return the best score 
         * achivable by the opponent
         */
        board[row, column] = playerSymbol;
        (GameLogic.GameState state, CrossesOrNoughts winner) = GameLogic.DetermineResult(board);

        if (state == GameLogic.GameState.Over)
        {
            board[row, column] = CrossesOrNoughts.Neither;
            return winner;
        }

        CrossesOrNoughts bestOponentResult = playerSymbol;
        CrossesOrNoughts opponentSymbol = GameLogic.OpponentSymbol(playerSymbol);

        for (int i = 0; i < 3; ++i)
        {
            for (int j = 0; j < 3; ++j)
            {
                if (board[i, j] == CrossesOrNoughts.Neither)
                {
                    CrossesOrNoughts result = EvaluateMove(i, j, board, opponentSymbol);
                    if (result == opponentSymbol)
                        bestOponentResult = opponentSymbol;
                    else if (result == CrossesOrNoughts.Neither && bestOponentResult == playerSymbol)
                        bestOponentResult = CrossesOrNoughts.Neither;
                }
            }
        }

        board[row, column] = CrossesOrNoughts.Neither;
        return bestOponentResult;
    }

    /// <summary>
    /// Calculates the best move that can be maid by the player
    /// </summary>
    /// <param name="playerSymbol">Players symbol - either crosses or noughtss</param>
    /// <param name="board">the game board</param>
    /// <returns>Best move that player can make</returns>
    public static MoveAnalysis BestMove(CrossesOrNoughts playerSymbol, GameBoard board)
    {
        // list of possible moves
        List<MoveAnalysis> moves = new List<MoveAnalysis>();

        // go over all empty positions and them as possible moves
        for (int i = 0; i < 3; ++i)
        {
            for (int j = 0; j < 3; ++j)
            {
                if (board[i, j] == CrossesOrNoughts.Neither)
                {
                    CrossesOrNoughts winner = EvaluateMove(i, j, board, playerSymbol);
                    moves.Add(new MoveAnalysis(i, j, winner));
                }
            }
        }

        // determine the best possible move and result
        MoveAnalysis bestMove = moves[0];

        for (int i = 1; i < moves.Count; ++i)
        {
            if (moves[i].Winner == playerSymbol)
            {
                bestMove = moves[i];
            }

            else if (moves[i].Winner == CrossesOrNoughts.Neither && bestMove.Winner == GameLogic.OpponentSymbol(playerSymbol))
            {
                bestMove = moves[i];
            }
        }

        // randomize - make a list of best moves and chose the best one
        List<MoveAnalysis> bestMoves = new List<MoveAnalysis>();
        for(int i = 0; i < moves.Count; ++i)
        {
            if (moves[i].Winner == bestMove.Winner)
                bestMoves.Add(moves[i]);
        }

        return bestMoves[random.Next(bestMoves.Count)];
    }
}

An class that is supposed to help to calculate the scores in the game ScoreCalculator.cs

/// <summary>
/// A class to help to determine the score obtained by the player as a result of the game
/// </summary>
public class ScoreCalculator
{
    public int WinScore { get; private set; }
    public int DrawScore { get; private set; }
    public int LoseScore { get; private set; }

    /// <summary>
    /// Constructor
    /// </summary>
    /// <param name="win">Score for the win</param>
    /// <param name="draw">Score for the draw</param>
    /// <param name="lose">Score for the loss</param>
    public ScoreCalculator(int win, int draw, int lose)
    {
        WinScore = win;
        DrawScore = draw;
        LoseScore = lose;
    }
}
\$\endgroup\$
  • \$\begingroup\$ A screenshot of your game would be a great addition to this question ;-) \$\endgroup\$ – t3chb0t Apr 29 '18 at 16:15
  • \$\begingroup\$ Did that. Don't be too hard on the graphics. \$\endgroup\$ – Yuval Khachatryan Apr 29 '18 at 22:03
3
\$\begingroup\$

First of all, let me just say I think you're on the right track. You're starting with a relatively small project and you're asking all of the right questions (for the purpose of getting better at programming).

Also, you're code is pretty good quality overall. There's a few things here than there that is a little odd but I've seen much worse.

The whole thing seems too big and bloated, although I did try not to repeat myself too much. Is there an idiomatic way to reduce its size?

This tends to come with experience. Some of the other things I'm about to say should be helpful with the overall size, but as a rule of thumb try to start with the simplest thing that works and build it out from there.

It's quite a common trap to overengineer the solution. A lot of the time you ain't gonna need it. Adding things you don't need is a two fold problem, first it takes time to write and hopefully test the code / documentation the first time around, and usually it takes more time later when you need to update / fix it. If you never actually use it, that's twice the waste.

I did try to provide XML documentation for all of the functions - Is it considered good practice? Or it is should be done only for the functions I expect to use in future?

In my experience, XML documentation is pretty rare if you work on a team of programmers in the real world. Most of the time well named methods and variables are preferred over lots of comments. Sometimes comments are helpful on complicated methods, but for the most part, leave them off.

Remember, it takes quite a bit of time to write good documentation and even more time to maintain it (out of date comments are worse than no comments). If it's not actually making the code easier to understand it's not adding value. So you're paying a cost for no benefit.

One exception to this rule might be when you're writing a library for other users to consume. Although, I personally still prefer documentation by example over XML comments.

The absolute worst kind of XML comments are those that state the obvious. For example:

/// <summary>
/// Constructor
/// </summary>
/// <param name="win">Score for the win</param>
/// <param name="draw">Score for the draw</param>
/// <param name="lose">Score for the loss</param>
public ScoreCalculator(int win, int draw, int lose)

Can be simplified to:

public ScoreCalculator(int winScore, int drawScore, int loseScore)

And it still makes perfect sense. We don't need to be told it's a constructor and by renaming the variables slightly we get all of the important information out of the comments into the code.

About the access level - When should I make a class public and where should I leave its visibility as default? If I've got it right - public classes will be available outside of the project - however mixing public classes with non-public sometimes would not compile.

Paying attention to the access level at the method, property and field level is pretty important. Code is a lot easier to reason about when you know how it might be used.

For example, a private _field can only be used inside the class it's declared. A protected Property { get; private set; } is only accessable in derived classes and only in a read-only way.

When you know this stuff, it makes it a lot easier to make changes and be confident you haven't broken anything outside your scope.

The same thing applies to a public Class but I wouldn't panic about it too much. Again, this is something that matters more when you're writing libraries and not so much when you're on a team. It's not uncommon to see almost all classes made public in a project.

Static functions/classes - should I have them at all?

When you're dealing with static the thing you need to be super careful with is global static state. Avoid it unless you really, really, can't.

However, it's important you understand the difference between static state and a pure function. In the case of a pure function is perfectly okay (and correct) to use static.

For example, you've created a pure function here and it's easy to reason about what the code does and know that it won't have any side effects. Input goes in and output comes out. Simple.

public static CrossesOrNoughts OpponentSymbol(CrossesOrNoughts playerSymbol)
{
    if (playerSymbol == CrossesOrNoughts.Crosses) return CrossesOrNoughts.Naughts;
    if (playerSymbol == CrossesOrNoughts.Naughts) return CrossesOrNoughts.Crosses;
    else return CrossesOrNoughts.Neither;
}

On the other hand, the BestMove method is not a pure function because it internally uses random which is an example of static state. In other words, if you call BestMove twice in a row with the same inputs you'll likely get difference outputs.

Properties with setters/getters, attributes vs properties - when should each one be used? When I had an object exposed by objects I've tried to expose it as a property - is it right way to do it?

I'm not sure I fully understand the question here.

Although, when I looked at the github repo I did notice some odd code when it comes to properties. For example:

private CrossesOrNoughts symbol;

public CrossesOrNoughts Symbol() => symbol;

public void SetSymbol(CrossesOrNoughts symbol)
{
    this.symbol = symbol;
}

This is really just a property disguised as a couple of methods and a field. Shown here it's reasonably easy to see what's going on but mixed in with other code it can be quite difficult to understand.

All of the above code can be reduced to:

public CrossesOrNoughts Symbol { get; set; }

Clean up all this kind of code and you might be surprised how much bloat you can get rid of.

public CrossesOrNoughts Symbol() => symbol; Is actually required I think, because Symbol() is a part of an interface, and interfaces do not allow properties, if I've got it right.

Interfaces certainly do allow properties. It says so in the docs:

An interface contains only the signatures of methods, properties, events or indexers. A class or struct that implements the interface must implement the members of the interface that are specified in the interface definition.

You can of course try this yourself.

As for properties vs fields, let me explain what I mean. Suppose that few objects have an access to the same object, which would normally be a field. In my code, for example, a few classes access the same board. Normally, I think the board should be a private field, because the users of the class need not to know that there is a "board" somewhere. So the board needs to be made public at least in one class. On the other hand, because logically it is the same board I've used GameBoard board => gameManager.Board instead of initializing it in the constroctor. Which method is preferable?

There isn't really a "preferable" way to do this. The best design will vary from project to project and you'll learn what works well and what doesn't with exprience.

That said, there are many well established design patterns that can be helpful to know. Learning the SOLID principles is a good place to start. I also read a book a while back called Game Programming Patterns which I thought was quite a good reference (if you look closely he has a free web version available).

Of course, these are called design patterns for a reason. They are a useful tool to have in your toolbox but they won't always fit perfectly in every situation. Sometimes you'll make up your own varition of a pattern that works for you.

Another question - I use XML comments, because the IDE can display information about the function later, by just pointing a mouse at it, and I don't need to look at the code if I forget something. What's the alternative?

As with anything, if you find it useful, run with it. My point about XML comments is that they are often not adding any value to the code itself. If you mouse over the DrawSquare method and the comment says "Draws a square image on the screen" then is it really that useful?

Of course, all of my answers are just my opinion. I'm definitely not right about everything. I'd like to think that 26 years of coding experience has taught me a few things, but if there's one thing I do know for sure, it's that this time next year some of these opinions may have changed.

\$\endgroup\$
  • \$\begingroup\$ public CrossesOrNoughts Symbol() => symbol; Is actually required I think, because Symbol() is a part of an interface, and interfaces do not allow properties, if I've got it right. \$\endgroup\$ – Yuval Khachatryan May 1 '18 at 10:47
  • \$\begingroup\$ As for properties vs fields, let me explain what I mean. Suppose that few objects have an access to the same object, which would normally be a field. In my code, for example, a few classes access the same board. Normally, I think the board should be a private field, because the users of the class need not to know that there is a "board" somewhere. So the board needs to be made public at least in one class. On the other hand, because logically it is the same board I've used GameBoard board => gameManager.Board instead of initializing it in the constroctor. Which method is preferable? \$\endgroup\$ – Yuval Khachatryan May 1 '18 at 10:56
  • \$\begingroup\$ Another question - I use XML comments, because the IDE can display information about the function later, by just pointing a mouse at it, and I don't need to look at the code if I forget something. What's the alternative? \$\endgroup\$ – Yuval Khachatryan May 1 '18 at 14:31
  • \$\begingroup\$ @YuvalKhachatryan I've added to my answer to answer your additional questions. \$\endgroup\$ – craftworkgames May 2 '18 at 4:23

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.