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

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 < 8; i++)
    {
        int a = winningIndex[i, 0], b = winningIndex[i, 1], c = winningIndex[i, 2];

        if (board.ContainsKey(a) && board.ContainsKey(b) && board.ContainsKey(c))
        {
            Func<KeyValuePair<int, string>, bool> containsO = input => (input.Key == a || input.Key == b || input.Key == c) && input.Value == "O";

            Func<KeyValuePair<int, string>, 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;
    }
}
share|improve this question
    
You have an approach to checking TicTacToe winners that I don't think I have seen before. Because of this, I'm not quite sure how GameBoard.Content and winningIndex are structured. Could you provide some additional information about that? What values exists in winningIndex and how do you add those values there? What kind of dictionary is your board? (I got a feeling it's <int, String>, right?) Do you fill it as each tile is played? (I assume yes?) –  Simon André Forsberg Jun 9 at 23:47
    
@SimonAndréForsberg The type of containsO confirms your guess about the dictionary. My guess is winningIndex would look like new int[,] { { 0, 1, 2 }, { 3, 4, 5 }, { 6, 7, 8 }, { 0, 3, 6 }, { 1, 4, 7 }, { 2, 5, 8 }, { 0, 4, 8 }, { 6, 4, 2 } }. –  mjolka Jun 10 at 2:13
add comment

2 Answers 2

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;
}
share|improve this answer
add comment

These variable initializations are not very readable nor maintainable as a single line:

int a = winningIndex[i, 0], b = winningIndex[i, 1], c = winningIndex[i, 2];

Have each one on a separate line:

int a = winningIndex[i, 0];
int b = winningIndex[i, 1];
int c = winningIndex[i, 2];

Moreover, you should use descriptive variable names instead of using single letters. You may know what they mean (for some time at least), but anyone else reading your code may not.

share|improve this answer
add comment

Your Answer

 
discard

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

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