A Tic Tac Toe game in C++

Question

I am a beginner programmer and have made a tic tac toe program in c++. I need some help with my memory management can anyone review and help me. This is my code

// This is a trial for tic tac toe in C++
#include <iostream>
using namespace std;

char *squares = new char[9]{'1', '2', '3', '4', '5', '6', '7', '8', '9'};

void play();
void getBoard();
int checkWin();

int main()
{
    char *playAgain = new char;
    do
    {
        play();
        cout << "Do you want to play again(y/n): ";
        cin >> *playAgain;

    } while (tolower(*playAgain) == 'y');
    delete squares, playAgain;
    cin.get();
    return 0;
}

//Play the game
void play()
{
    int *i = new int;
    int player = 1;
    int *choice = new int;
    char *mark = new char;

    do
    {
        getBoard();
        player = (player%2) ? 1 : 2;
        cout << "Enter your choice: ";
        cin >> *choice;
        *mark = (player == 1) ? 'X' : 'O';

        if (squares[0] == '1' && *choice == 1)
        {
            squares[0] = *mark;
        }
        else if (squares[1] == '2' && *choice == 2)
        {
            squares[1] = *mark;
        }
        else if (squares[2] == '3' && *choice == 3)
        {
            squares[2] = *mark;
        }
        else if (squares[3] == '4' && *choice == 4)
        {
            squares[3] = *mark;
        }
        else if (squares[4] == '5' && *choice == 5)
        {
            squares[4] = *mark;
        }
        else if (squares[5] == '6' && *choice == 6)
        {
            squares[5] = *mark;
        }
        else if (squares[6] == '7' && *choice == 7)
        {
            squares[6] = *mark;
        }
        else if (squares[7] == '8' && *choice == 8)
        {
            squares[7] = *mark;
        }
        else if (squares[8] == '9' && *choice == 9)
        {
            squares[8] = *mark;
        }
        else
        {
            cout << "Invalid move ";

            player--;
            cin.ignore();
            cin.get();
        }

        *i = checkWin();
        player++;

    } while (*i == -1);
    getBoard();
    if (*i == 1)
    {
        cout << "\aPlayer " << --player << " Wins" << endl;
        delete mark, choice, i;
    }
    else
    {
        cout << "\aGame Draw" << endl;
        delete mark, choice, i;
    }
}

// Print the board
void getBoard()
{
    cout << "\n\n\tTic Tac Toe\n\n";

    cout << "Player 1 (X)  -  Player 2 (O)" << endl
         << endl;
    cout << endl;

    cout << "     |     |     " << endl;
    cout << "  " << squares[0] << "  |  " << squares[1] << "  |  " << squares[2] << endl;

    cout << "_____|_____|_____" << endl;
    cout << "     |     |     " << endl;

    cout << "  " << squares[3] << "  |  " << squares[4] << "  |  " << squares[5] << endl;

    cout << "_____|_____|_____" << endl;
    cout << "     |     |     " << endl;

    cout << "  " << squares[6] << "  |  " << squares[7] << "  |  " << squares[8] << endl;

    cout << "     |     |     " << endl
         << endl;
}

/**********************************************************************************************************
Return 1 if some one wins
Return 0 if draw
Return -1 if the game is not over
***********************************************************************************************************/

int checkWin()
{
    if (squares[0] == squares[1] && squares[1] == squares[2])
    {
        return 1;
    }
    else if (squares[0] == squares[3] && squares[3] == squares[6])
    {
        return 1;
    }
    else if (squares[0] == squares[4] && squares[4] == squares[8])
    {
        return 1;
    }
    else if (squares[3] == squares[4] && squares[4] == squares[5])
    {
        return 1;
    }
    else if (squares[1] == squares[4] && squares[4] == squares[7])
    {
        return 1;
    }
    else if (squares[6] == squares[4] && squares[4] == squares[2])
    {
        return 1;
    }
    else if (squares[6] == squares[7] && squares[7] == squares[8])
    {
        return 1;
    }
    else if (squares[2] == squares[5] && squares[5] == squares[8])
    {
        return 1;
    }
    else if (squares[0] != '1' && squares[1] != '2' && squares[2] != '3' && squares[3] != '4' && squares[4] != '5' && squares[5] != '6' && squares[6] != '7' && squares[7] != '8' && squares[8] != '9')
    {
        return 0;
    }
    else
    {
        return -1;
    }
}

Answer 1

Try not to using namespace std

It is considered a bad practice. This is because namespaces were introduced as a way to avoid name collisions, i.e Multiple objects with the same name. The std namespace is HUGE and has hundreds of common identifiers that can interfere with yours.

By doing using namespace std you now have no idea what function is a part of the std library, and what isn't.

The use of new

I think you have misunderstood the use of the new operator in C++. What is the use of dynamic memory allocation here? I fail to see a good reason to not simply create your variables on the stack. The problem here is you cannot forget to delete. If you do, you expose your program to memory leaks - Horrible.

You can use new when:

• You don't want an object to be destroyed until you call delete
• When your object is very large

In short, there is no need to use the new operator here. It just introduces complications.

When to use the 'new' operator

---

Use an enum

/**********************************************************************************************************
Return 1 if some one wins
Return 0 if draw
Return -1 if the game is not over
***********************************************************************************************************/

the numbers 1, 0, -1 are known as magic numbers, your comment helps but one would have to keep referring back to know their meaning. Using an enum here will clear a lot

enum Result { Win , Draw , OnGoing };

return Result::Win;

That way you can declare your function as

Result checkWin();

And instead of

return (number which means nothing)

You do

return Result::Win;

The values 0, 1, and 2 are automatically assigned to Win, Draw, Ongoing respectively. Now that we have the names for these numbers, we use them instead of the raw numbers. That means

return Result::Win;

Is the same as

return 0;

---

checkWin()

int checkWin()
{
    if (squares[0] == squares[1] && squares[1] == squares[2])
    {
        return 1;
    }
    else if (squares[0] == squares[3] && squares[3] == squares[6])
    {
        return 1;
    }
    else if (squares[0] == squares[4] && squares[4] == squares[8])
    {
        return 1;
    }
    else if (squares[3] == squares[4] && squares[4] == squares[5])
    {
        return 1;
    }
    else if (squares[1] == squares[4] && squares[4] == squares[7])
    {
        return 1;
    }
    else if (squares[6] == squares[4] && squares[4] == squares[2])
    {
        return 1;
    }
    else if (squares[6] == squares[7] && squares[7] == squares[8])
    {
        return 1;
    }
    else if (squares[2] == squares[5] && squares[5] == squares[8])
    {
        return 1;
    }
    
}

A more readble way to do this would be to store the patterns in an array, and everytime you want to check for a winner, you iterate through the array and use them.

constexpr int winPatterns[8][3]{
    {0, 1, 2}, // first row
    {3, 4, 5},  // second row

    //...
};

Now when you want to check for a winner

for (int i = 0;i < 8;i++){
    auto const& line = winPatterns[i];
    const int a = squares[line[0]];
    const int b = squares[line[1]];
    const int c = squares[line[2]];
    if ( a == b and b == c ) return a;
}

---

Checking for draw

Instead of checking whether each and every cell is occupied, use an int which is initially set to 0. Keep incrementing it after every turn. That way you can get the result like so

bool checkDraw(){
    return (checkWin() != Result::Win and counter == 9 );
}

It is simple. If the counter reaches 9 and no one has won yet, it is a draw because all the cells are occupied without a winner.

---

Printing the board

cout << "\n\n\tTic Tac Toe\n\n";

    cout << "Player 1 (X)  -  Player 2 (O)" << endl
         << endl;
    cout << endl;

    cout << "     |     |     " << endl;
    cout << "  " << squares[0] << "  |  " << squares[1] << "  |  " << squares[2] << endl;

    cout << "_____|_____|_____" << endl;
    cout << "     |     |     " << endl;

    cout << "  " << squares[3] << "  |  " << squares[4] << "  |  " << squares[5] << endl;

    cout << "_____|_____|_____" << endl;
    cout << "     |     |     " << endl;

    cout << "  " << squares[6] << "  |  " << squares[7] << "  |  " << squares[8] << endl;

    cout << "     |     |     " << endl
         << endl;

You can use string literals here.

const char* Heading = R"(
        Tic Tac Toe

Player 1 (X)  -  Player 2 (O)

)";

std::cout << Heading;

Answer 2

Avoid using namespace std

I cannot emphasize enough on how bad this can be, avoid it now. You might not feel its effect now, but it becomes a headache when the size of the codebase increases. Imagine this, you have a class that defines a method cout, this might seem trivia, who would define a method with cout?. But you should know, there are numerous good and reasonable names that are already taken by the standard libary, what good name can you come up for sin, cos or tan. Doing this makes the compiler confused and results to undefined behaviors. Not only that, you pollute everywhere with irrelevant stuff you aren't using. Prefer

std::cout << "Hello World\n"

or

using std::cout
cout << "Hello World\n"

in the latter, the effect is localized to just std::cout

Avoid GLOBALS!!!!

Just one global might seem reasonable to the programmer, but it leads to many subtle bugs and makes the code dependent on those globals.

char *squares = new char[9]{'1', '2', '3', '4', '5', '6', '7', '8', '9'};

could be defined in main() and passed as an argument to other functions.

Builtins are never implicitly defined in a local scope

Builtins like Integral types and floating-point types are never initialized implicitly,

int a;

This above snippet is uninitialized and is a source of hard to track bugs, avoid it, when in doubt, explicitly initialize your variables. This means, the above code snippet becomes

int a{}

Here a is initialized to 0.

Bad delete

delete mark, choice, i;

Ok, you want to free the memory allocated to those variables, but oops, what have you done, you succeeded in deleting just mark. choice and i are never deleted. A good compiler should warn against, this emphasizes that you should make the compiler your friend, do not let warnings pass implicitly, take care of them or they might hurt you later.

delete mark;
delete choice;
delete i;

Now, this is better, we made it explicit what we want to do.

Detour

1. getBoard() is a misleading name, variable naming is the heart of programming and must carry its intent as unambiguous as possible, a first glance at getBoard(), I would be expecting an array of board as a return value, but it suprisingly prints a board to the stdout, a better name would be printBoard or displayBoard, this is so clear to a reader like me.

2. Right now, after a game is over, a user might want to play again, but your game does not reset itself, you might want to fix that subtle bug.

3. Some parts of your code are really messy, I would highlight just a few of them. Right now, you have a lot of checks just to place a mark at a location. You can simplify things by having an isValid() function like this

bool isValid(int loc)
    return loc > 0 && loc <= MAX_SIZE;

Then placing a mark is as simple as saying

squares[*choice-1] = *mark;

[EDIT] isValid() checks if a user's input is within the range of the squares for example 1 is valid because it is greater than 0 and less than 9(The size of the squares). -1 is invalid because it is not greater than 0 though it is less than 9. If isValid() returns true, we can offset it by 1 to give the location in squares, so a value of 1 means an index of 0 in squares.

This greatly reduces the code size, you might want to consider the same approach for checkwin().

Side Note

When you have multiple if-else statements, consider refactoring your code

Answer 3

The other answers have some very good advice. I would just like to add a few opinions.

Separation of concerns

The data structure squares has been given two very different jobs:

• Keep track of what happens in the game.
• Keep track of what to display to the user after each move.

These are two different concerns. It's usually good to separate the display of what happens in a program from the actual data and algorithms of a program.

In this particular case, you have the very wordy and repetitious logic of

if (squares[0] == '1' && *choice == 1)

and the other eight similar checks because you are using the symbol that will be displayed in each square to decide what has actually happened in each square. As far as the game mechanics go, it would be simpler to have one way to indicate that a square is empty, so you could replace the nine different checks for the contents of the square with one. You could have code something like this:

if (1 <= choice && choice <= 9)
{
  if (squares[choice - 1] == EMPTY_SQUARE)
  {
    isValidMove = true;
    squares[choice - 1] = mark;
  }
  else
  {
    std::cout << "That square has already been played.\n";
  }
}
else
{
  std::cout << "Your move must be a number between 1 and 9 inclusive.\n";
}

(Note that I've assumed you also took the advice to declare variables on the stack rather than using pointers.)

Note that you could combine the two if conditions together, but a bonus of stating them separately is that you can display more informative error messages to the user depending on which error they make.

The function that displays the board would then have the sole responsibility for deciding what to show in the empty squares. This would admittedly make its job slightly more complicated, since you cannot just display squares[0] in the upper left square, but you can delegate that job to a subroutine:

char contentsForDisplay(char* squares, int index)
{
  static const char* emptyText = "123456789";
  if (squares[index] == EMPTY_SQUARE)
  {
    return emptyText[index];
  }
  else
  {
    return squares[index];
  }
}

Less confusing use of data

Separation of concerns will help with this in the case of the squares array. But let's also consider the variable player.

We start out simply enough with int player = 1;, but then we have player = (player%2) ? 1 : 2;, later player++, and yet elsewhere player-- and --player. If we were to trace the values the player variable is set to, it would sometimes be 3 (after player 2's turn and before we get to player = (player%2) ? 1 : 2; in player 1's turn) and sometimes 0 (after player 1 attempts an illegal move).

Even an experienced programmer will have difficulty keeping track of this. You'd better hope nobody has to make any modifications to the code later (or that if they do, they have the time to test the living daylights out of it), because this is an open invitation for bugs to be introduced.

I also notice that you never say whose turn it is. The first time that the user interface ever uses the numbers 1 or 2 to identify a player is when it says who wins. Perhaps you might have noticed this if you didn't have to expend so much mental effort to get the correct player number in the first place.

Below, I'll suggest some ways to tame this particular variable.

Now what is this variable named i? That's a typical name for a loop variable, for example in something like this where the only use of i is inside the loop, which is a small chunk of code so that we never have to look very far from where i is used to see how i is defined:

for (int i = 0; i < NUMBER_OF_SQUARES; ++i)
{
  squares[i] = EMPTY_SQUARE;
}

I strongly advise using a more descriptive name such as gameStatus (which also isn't great, but you've overloaded this one poor little variable with having to say whether the game is still going and if it is not still going, whether it's a win or a draw, so it's hard to come up with a short, fully descriptive name).

Declare variables closer to where they are used

This is related to less confusing use of data. You don't have a lot of opportunities to declare variables more locally in the existing design of your program, but you could at least do this rather than declaring mark where you do:

char mark = (player == 1) ? 'X' : 'O';

More functions

You could use a few more functions to perform certain tasks within the program. I already suggested the function contentsForDisplay.

A good candidate for a somewhat larger role is a function to perform one move for one player. Here is a declaration of one that assumes you have also used an enumeration to describe the mark each player makes in a square:

void waitForValidMove(PlayerMarker player, int* squares);

This function would prompt the player for a move and continue running (with an internal loop) until the player made a valid response. This means no trickery is required to keep your outer loop playing the correct player (such as decrementing player from 1 to 0 after a illegal move by player 1 so that ++player doesn't cause player 2's turn to start).

This gives you another option to handle the alternation of players:

GameStatus status = CONTINUE_PLAYING;
while (status == CONTINUE_PLAYING)
{
  waitForValidMove(FIRST_PLAYER, squares);
  status = checkWin(squares);
  if (status == CONTINUE_PLAYING)
  {
    waitForValidMove(SECOND_PLAYER, squares);
  }
}

Alternatively, you could do just one player's turn per iteration of the main loop, but use a function to tell whose turn it is:

enum PlayerMarker getNextPlayer(PlayerMarker currentPlayer)
{
  return (currentPlayer == FIRST_PLAYER ? SECOND_PLAYER : FIRST_PLAYER);
}

This is more wordy than just writing player = (player%2) ? 1 : 2;, but it's clearer, and it's more self-documenting than even just writing player = (player == FIRST_PLAYER ? SECOND_PLAYER : FIRST_PLAYER); in-line.

Another candidate might be a function to decide whether a particular winning set of squares has been taken by one player:

bool squaresAreWon(const char* squares, int index1, int index2, int index3)
{
  return (squares[index1] == squares[index2] && squares[index2] == squares[index3]);
}

This may seem trivial, but it lets you write code that is more compact and self-documenting:

if (squaresAreWon(squares, 0, 1, 2)
    || squaresAreWon(squares, 3, 4, 5)
    || squaresAreWon(squares, 6, 7, 8)
    ... etc. ...

(But note that an alternative to an additional function is to implement an additional data structure such as the winPatterns array recommended in another answer.)

Use objects

The main thing that's supposed to distinguish C++ from C (at least the way I heard it years ago) is that C++ is object-oriented. That's a debatable proposition, but it is a fact that C++ has convenient syntax (the class syntax) for defining types of "objects".

This particular program is so simple that classes/objects might be overkill, but for someone experienced with C++ classes it's tempting to create a Board class to contain the record of what has been played in each square, in which case Board could have a public function bool checkWin() const; so that any subroutines that function uses could be made private functions of Board.

Actually, with a Board class you could have the function of Board that records a player's move also immediately determine whether the game is still going, a win, or a draw, and set member variables of the Board object accordingly, so that you can simply call bool-valued functions of Board to find out whether you need to keep playing (during the loop) and whether someone has won (after the loop). This would solve the problem of what kind of reasonably descriptive name to give to your variable i -- you would no longer need to declare that variable at all.

To be honest, however, it might be better to wait until you have a problem where creating some classes will really help with the implementation, unless you have a particular desire to make this project the one in which you move beyond using C++ as a fancy version of C.