6
\$\begingroup\$

This code checks winning conditions in Tic-Tac-Toe by checking if there is any row, column or diagonal with the same symbols.

The board is a 2-dimensional array of chars. The character ' ' means that a field is empty.

How can I refactor/simplify this code?

 public static bool SomeoneWins(char[][] board)
        {
            // Check columns
            for (var x = 0; x < board.Length; x++)
            {
                var firstField = board[x][0];
                if (firstField == ' ') continue;
                bool allFieldsTheSame = true;

                for (var y = 1; y < board[x].Length; y++)
                {
                    if (board[x][y] != firstField)
                    {
                        allFieldsTheSame = false;
                        break;
                    }
                }

                if (allFieldsTheSame) return true;
            }

            // Check rows
            for (var y = 0; y < board.Length; y++)
            {
                var firstField = board[0][y];
                if (firstField == ' ') continue;
                var allFieldsTheSame = true;

                for (var x = 1; y < board.Length; x++)
                {
                    if (board[x][y] != firstField)
                    {
                        allFieldsTheSame = false;
                        break;
                    }
                }

                if (allFieldsTheSame) return true;
            }

            // first diagonal
            if (board[0][0] != ' ')
            {
                var allFieldsTheSame = true;

                for (var d = 0; d < board.Length; d++)
                {
                    if (board[d][d] != board[0][0])
                    {
                        allFieldsTheSame = false;
                        break;
                    }
                }
                if (allFieldsTheSame) return true;
            }

            // second diagonal
            if ( board[board.Length - 1][0] != ' ')
            {
                var allFieldsTheSame = true;
                for (var d = 0; d < board.Length; d++)
                {
                    if (board[d][board.Length-d-1] !=  board[board.Length - 1][0])
                    {
                        allFieldsTheSame = false;
                        break;
                    }
                }
                if (allFieldsTheSame) return true;
            }

            return false;
        }
\$\endgroup\$
  • 1
    \$\begingroup\$ Have you actually tried this code? There seem to be at least one obvious mistake in for (var x = 1; y < board.Length; x++) \$\endgroup\$ – barak manos Jul 18 '15 at 14:56
  • \$\begingroup\$ @barakmanos Yes and this is not a mistake. Note that zeroth element is checked before the loop. \$\endgroup\$ – Kao Jul 20 '15 at 7:57
  • 1
    \$\begingroup\$ I am not referring to the fact that you start from the first element, but to the fact that you increment x while checking y < length. \$\endgroup\$ – barak manos Jul 20 '15 at 8:11
  • \$\begingroup\$ Oh, now I see. You are right, of course. \$\endgroup\$ – Kao Jul 20 '15 at 8:54
5
\$\begingroup\$

What does all winning conditions in Tic Tac Toe have in common? They are all straight lines!

The idea of having one method that can be called multiple times is a good one, to do that we need to input the starting position, and how much we should change x and y with every time.

We also need a way to stop the loop, we can either stop when we notice that we will go out of bounds, or we can stop after a specific number of checks. In this implementation, I chose to hard-code 3 as the limit for how many tiles to check.

public static bool AllFieldsTheSame(int startX, int startY, char[][] board, int dx, int dy)
{
    char firstField = board[startY][startX]
    if (firstField == ' ')
    {
        return false;
    }

    for (var i = 0; i < 3; i++)
    {
        int y = startY + dy * i;
        int x = startX + dx * i;
        if (board[y][x] != firstField)
        {
            return false;
        }
    }

    return true;
}

Then this method can be called repeatedly as follows:

public static bool SomeoneWins(char[][] board)
{
    // Check columns
    for (var x = 0; x < board.Length; x++)
    {
        if (AllFieldsTheSame(x, 0, board, 0, 1))
            return true;
    }

    // Check rows
    for (var y = 0; y < board.Length; y++)
        if (AllFieldsTheSame(0, y, board, 1, 0))
            return true;

    // Check diagonals
    if (AllFieldsTheSame(0, 0, board, 1, 1))
        return true;

    if (AllFieldsTheSame(2, 0, board, -1, 1))
        return true;
}

However, I suspect you are also interested in who wins, in which case you could have both AllFieldsTheSame and SomeoneWins return a char instead of a bool.

And by the way, I'd prefer to use an enum for the possible values of each tile. A char can have the value of Q, but I don't believe you want to place a Q tile in your game. Using an enum reduces the eliminates any possible risk of invalid characters.

\$\endgroup\$
  • \$\begingroup\$ Mixing bracing style (with/without braces) should be avoided. \$\endgroup\$ – Heslacher Jul 20 '15 at 6:07
  • \$\begingroup\$ @Heslacher Yes, it should. I hope the OP does that when using this code. \$\endgroup\$ – Simon Forsberg Jul 20 '15 at 10:19
2
\$\begingroup\$

There seem to be at least one obvious mistake in for (var x = 1; y < board.Length; x++).

In addition to that, you may as well generalize the code for checking rows and columns:

public static bool AllFieldsTheSame(char[][] board, char firstField, int m)
{
    if (firstField == ' ')
        return false;

    for (var n = 1; n < board[m].Length; n++)
        if (board[m][n] != firstField)
            return false;

    return true;
}

public static bool SomeoneWins(char[][] board)
{
    // Check columns
    for (var x = 0; x < board.Length; x++)
        if (AllFieldsTheSame(board, board[x][0], x)
            return true;

    // Check rows
    for (var y = 0; y < board.Length; y++)
        if (AllFieldsTheSame(board, board[0][y], y);
            return true;

    // The rest of your code below...
}
\$\endgroup\$
  • \$\begingroup\$ I don't believe this refactoring will work. Are you sure about if (board[m][n] != firstField) ? \$\endgroup\$ – Simon Forsberg Jul 19 '15 at 12:39
  • \$\begingroup\$ It is impossible for your AllFieldsTheSame method to check the win conditions on both columns and rows. \$\endgroup\$ – Simon Forsberg Jul 20 '15 at 10:20

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.