HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Check winner in a Tic Tac Toe game

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
toetictacgamewinnercheck

Problem

I have the below code to check for a winner in a Tic Tac Toe game. I'm wondering if this is a good approach, and if there is a better way of doing this (maybe by monitoring the state of the board).

public void CheckWinner()
{
    var board = GameBoard.Content;//stores data as dictionary
    for (int i = 0; i , bool> containsO = input => (input.Key == a || input.Key == b || input.Key == c) && input.Value == "O";

            Func, bool> containsX = input => (input.Key == a || input.Key == b || input.Key == c) && input.Value == "X";

            var oResult = board.Where(containsO).ToDictionary();
            if (oResult.Count == 3)
            {
                Winner = Player.Computer;
                WinnerIndex = oResult.Keys.ToArray();
                _gameOver = true;
            }

            var xResult = board.Where(containsX).ToDictionary();
            if (xResult.Count == 3)
            {
                Winner = Player.Human;
                WinnerIndex = xResult.Keys.ToArray();
                _gameOver = true;
            }
        }
    }
    if (board.Count == 9)
    {
        _gameOver = true;
        Winner = null;
    }
}

Solution

General advice:

-
Use enums instead of strings for board squares. This will protect you against mistakes like board[i] = "x".

-
A dictionary is overkill for a 3x3 board, you can just use an array. One benefit is that a bad index will throw an IndexOutOfRangeException, while a dictionary will not complain.

-
Board logic should be encapsulated in a class, so that you can make changes to your implementation (like switching from a dictionary to an array) without the rest of your code having to change. It will also allow you to guard against errors such as overwriting a non-empty board square.

To make the function easier to test:

-
Pass the board to the function, or make this a method of your Board class, instead of referring to GameBoard.Content.

-
Return an object representing the game state, instead of modifying private fields.

Together, it might look like this:

public static GameState GetGameState(Board board)
{
    foreach (var winningLine in WinningLines)
    {
        var line = new BoardState[]
        {
            board[winningLine[0]],
            board[winningLine[1]],
            board[winningLine[2]]
        };

        if (line.All(state => state == BoardState.Human))
        {
            return GameState.HumanWon(winningLine);
        }

        if (line.All(state => state == BoardState.Computer))
        {
            return GameState.ComputerWon(winningLine);
        }
    }

    return board.Any(state => state == BoardState.Empty)
        ? GameState.Unfinished
        : GameState.Tie;
}

Code Snippets

public static GameState GetGameState(Board board)
{
    foreach (var winningLine in WinningLines)
    {
        var line = new BoardState[]
        {
            board[winningLine[0]],
            board[winningLine[1]],
            board[winningLine[2]]
        };

        if (line.All(state => state == BoardState.Human))
        {
            return GameState.HumanWon(winningLine);
        }

        if (line.All(state => state == BoardState.Computer))
        {
            return GameState.ComputerWon(winningLine);
        }
    }

    return board.Any(state => state == BoardState.Empty)
        ? GameState.Unfinished
        : GameState.Tie;
}

Context

StackExchange Code Review Q#53841, answer score: 8

Revisions (0)

No revisions yet.