Tic-Tac-Toe program

Question

My code can play the tic-tac-toe game with two human players. How can I can make it better ? I will be making a new version that plays tic-tac-toe against a computer later.

/*  Tic Tac Toe - Player 1 vs Player 2*/
#include <iostream>
using namespace std;

#define SIZE 9

//------------------------------------------------------
void printCurrentBoard(char board[])
{
    cout << "\n";
    cout << "\t  " << board[0] << "  | " << board[1] << "  | " << board[2] << "\n";
    cout << "\t---------------\n";
    cout << "\t  " << board[3] << "  | " << board[4] << "  | " << board[5] << "\n";
    cout << "\t---------------\n";
    cout << "\t  " << board[6] << "  | " << board[7] << "  | " << board[8] << "\n\n";

}


//------------------------------------------------------
void player1(char board[])
{
    int move;

    cout << "\nPlayer 1's turn: ";
    cin >> move;

    while(1)
        if (board[move] == 'O')
        {
            cout << "Invalid move, try again: ";
            cin >> move;
        }
        else
        {
            break;
        }

    board[move] = 'X';
}


//------------------------------------------------------
void player2(char board[])
{
    int move;

    cout << "\nPlayer 2's turn: ";
    cin >> move;

    while(1)
        if (board[move] == 'X')
        {
            cout << "Invalid move, try again: ";
            cin >> move;
        }
        else
        {
            break;
        }

    board[move] = 'O';
}


//------------------------------------------------------
int winnerCheck(char board[])
{

    /*  check the diagonals */

    if( ((board[0] == board[4]) && (board[4] == board[8])) ||
        ((board[2] == board[4]) && (board[4] == board[6])))
    {
        if(board[4] == 'X')
        {
            cout << "\nGAME OVER!! Player 1 [X] won!\n";
            return 1;
        }
        else
        {
            cout << "\nGAME OVER!! Player 2 [O] won!\n";
            return 2;
        }
    }

    /* check the rows   */
    if( ((board[0] == board[1]) && (board[1] == board[2])) ||
        ((board[3] == board[4]) && (board[4] == board[5])) ||
        ((board[6] == board[7]) && (board[7] == board[8])) )
    {
        if(board[1] == 'X' || board[4] == 'X' || board[7] == 'X')
        {
            cout << "\nGAME OVER!! Player 1 [X] won!\n";
            return 1;
        }
        else
        {
            cout << "\nGAME OVER!! Player 2 [O] won!\n";
            return 2;
        }
    }

    /* check the columns    */
    if( ((board[0] == board[3]) && (board[3] == board[6])) ||
        ((board[1] == board[4]) && (board[4] == board[7])) ||
        ((board[6] == board[7]) && (board[7] == board[8])) )
    {
        if(board[1] == 'X' || board[4] == 'X' || board[7] == 'X')
        {
            cout << "\nGAME OVER!! Player 1 [X] won!\n";
            return 1;
        }
        else
        {
            cout << "\nGAME OVER!! Player 2 [O] won!\n";
            return 2;
        }
    }
}


//------------------------------------------------------
int main()
{
    char board[SIZE] = {'0', '1', '2', '3', '4', '5', '6', '7', '8'};

    cout << "\t  Tic Tac Toe\n";
    printCurrentBoard(board);
    cout << "\nEnter a number to make a move.\n";

    while(1)
    {
        player1(board);
        printCurrentBoard(board);
        if(winnerCheck(board) == 1)
        {
            return (0);
        }

        player2(board);
        printCurrentBoard(board);
        if(winnerCheck(board) == 2)
        {
            return (0);
        }
    }
}

Answer 1

Don’t write using namespace std;.

Read through and bookmark the C++ Standard Guidelines.

Numbers I note later are citations from this.

---

#define SIZE 9

Don't use #define for constants or “functions” (⧺ES.31)

I see this is only used in one place, in main, so why do you even need a name for it? You can have the array automatically determine the size based on the initializer:

char board[] = {'0', '1', '2', '3', '4', '5', '6', '7', '8'};

---

void printCurrentBoard(char board[])

You realize that this is actually (char* board) because arrays are not passed to functions. Generally, don’t pass a pointer to a sequence like this, whether length is passed separately or implicitly known. See ⧺I.13.

You should make the board an object. Then this can be a member function that doesn’t need a parameter.

You should have made it const char* since the function does not modify the data.

The body is very repetitive! Use a loop and don’t repeat code en masse.

---

player1 and player2 have the same parameter issue.

The functions are identical except for one data item!! Don’t Do That! Factor out the thing that changed and make it a parameter.

Since “player 1” and “player 2” are not very descriptive anyway, I’ll use the X and O for the prompt, too.

void player (const char Mark, char* board)
{
    int move;

    cout << '\n' << Mark << " player's turn: ";
    cin >> move;

    while(1)
        if (!free_space(board[move]))
        {
            cout << "Invalid move, try again: ";
            cin >> move;
        }
        else
        {
            break;
        }

    board[move] = Mark;
}

Your test allowed a player to skip a turn by playing on a square he already had. It doesn’t matter which player is moving; only unoccupied squares are available. So I broke the test out into another function so as not to clutter the code. It’s readable with the name free_space, without worrying too much about how it actually makes that determination, right?

Your loop is structured awkwardly. Use a post test loop:

do {
    cout << "Invalid move, try again: ";
    cin >> move;
} while (!free_space(board[move]));

Note that your input function still doesn’t verify that a valid number was entered. Type 22 for example and it will trash the stack — very bad!

Make getting a legal input from 0 to 8 a separate function that is concerned only with that, and doesn’t return until it gets a good value.

---

Function winnerCheck is another example of repeated code. You can see that the win/lose messages and return values are stated three times, with a pair of nearly identical checks in each repetition.

Instead, call a separate function that just returns a code indicating the winner (or lack of) and then print the message based on the result. And only have one copy of the print code, using the variable to state which player it was.

The actual checking can be done more algorithmically, not expanded out into individual tests like that.

I see you did manage to not repeat the checking for X and O, but test for a row/col/diag of the same mark. That is better than most of the other code here.

---

The main is two copies of the same block inside the loop. After making a single player(param) function rather than player1 and player2, can you see how to make one loop that alternates players on each iteration?

---

Keep at it!

Please post your next version after making a Player class and not repeating code, and I’ll be happy to look at it again.

Answer 2

The function winnerCheck has a bug.

Imagine at position 1 there is an X, and at positions 3, 4 and 5 there are Os. The row check be true, because there are 3 identical markers in a row. Next you look at element 1 and see that there is an X there, so proclaim player 1 as the winner. But it was player 2 that had the 3 markets in a row.

The correct way to implement this is to test each row and each column independently. You can do this using a loop. The check for the two diagonals is correct.

On the other hand, only the current player can be the winner, so this function could instead simply return true/false if someone won (there are three identical markers in a row), and let the calling function print out who won.

—-

The functions player1 and 2 also are flawed: each player is not allowed to put a marker on top of an opposite marker, but they can put one on top of their own. You should also check that the number entered is one of the valid values. A player could enter 10, or -46753, and you would write outside the array bounds (and likely crash the program).

Answer 3

• Don't use using namespace std

• Prefer using a constant for board size over a global #define

• Your player functions do the same and can be merged with an argument indicating who is to play

• WinnerCheck misses a final return

• The //------- type comments are not needed

• The board can be printed with a loop

• As πάντα ῥεῖ pointed out this could be rewritten in an OOP way if you so choose