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 == '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;
std::cout << "That square has already been played.\n";
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 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)
Less confusing use of data
Separation of concerns will help with this in the case of the
But let's also consider the variable
We start out simply enough with
int player = 1;,
but then we have
player = (player%2) ? 1 : 2;,
player++, and yet elsewhere
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
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';
You could use a few more functions to perform certain tasks within the program.
I already suggested the function
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)
status = checkWin(squares);
if (status == CONTINUE_PLAYING)
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.)
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
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.