Tic-Tac-Toe game in C++

Question

My friend is kind of new to coding and he decided to write a console tic-tac-toe game. He then showed me his code and I thought it would be cool to optimize it a bit and rewrite in my own way. I tried my best but I'm pretty new to C++ and I'm not super experienced in optimizing things so I'd like to know about ways to improve my code in both performance and/or readability with the preference being on performance.

#include <iostream>
#include <stdlib.h>
#include <sstream>
using namespace std;
char board[3][3] = {{'1', '2', '3'}, {'4', '5', '6'}, {'7', '8', '9'}};
string choice;
int row, rows[3], col, columns[3], diagonal, anti_diagonal, intchoice;
bool turn = true; // True = X turn, False = O turn
void display_board()
{
 cout << "\nPlayer 1[X] Player 2[O]\n";
 cout << " | | \n";
 cout << " " << board[0][0] << " | " << board[0][1] << " | " << board[0][2] << " \n";
 cout << "_____|_____|_____\n";
 cout << " | | \n";
 cout << " " << board[1][0] << " | " << board[1][1] << " | " << board[1][2] << " \n";
 cout << "_____|_____|_____\n";
 cout << " | | \n";
 cout << " " << board[2][0] << " | " << board[2][1] << " | " << board[2][2] << " \n";
 cout << " | | \n";
}
bool isint(string str) // Input check for an int value
{
 for (int i = 0; i < str.length(); i++)
 if (!isdigit(str[i]))
 return false;
 return true;
}
void player_turn()
{
 while (true)
 {
 if (turn)
 cout << "Player - 1 [X] turn : ";
 else if (!turn)
 cout << "Player - 2 [0] turn : ";
 getline(cin, choice); // Prevents multiple turns in one input
 if (!isint(choice))
 continue;
 stringstream(choice) >> intchoice;
 switch (intchoice)
 {
 case 1: row = 0; col = 0;break;
 case 2: row = 0; col = 1; break;
 case 3: row = 0; col = 2; break;
 case 4: row = 1; col = 0; break;
 case 5: row = 1; col = 1; break;
 case 6: row = 1; col = 2; break;
 case 7: row = 2; col = 0; break;
 case 8: row = 2; col = 1; break;
 case 9: row = 2; col = 2; break;
 default: cout << "Invalid Move\n"; continue;
 }
 if (board[row][col] == 'X' || board[row][col] == 'O')
 cout << "Occupied cell\n";
 else
 {
 int change = turn ? 1 : -1; // Increment or Decrement value for win check
 board[row][col] = turn ? 'X' : 'O';
 rows[row] += change;
 columns[col] += change;
 if (row == col)
 diagonal += change;
 if (row == (2 - col))
 anti_diagonal += change;
 turn = !turn; // Switch turns
 return;
 }
 }
}
bool gameover()
{
 for (int i = 0; i < 3; i++)
 if (rows[i] > 2 || rows[i] < -2 || columns[i] > 2 || columns[i] < -2 || diagonal > 2 || diagonal < -2 || anti_diagonal > 2 || anti_diagonal < -2)
 return true;
 return false;
}
int main()
{
 cout << "T I C K -- T A C -- T O E -- G A M E";
 display_board();
 for (int i = 0; i < 9; i++)
 {
 player_turn();
 display_board();
 if (gameover())
 break;
 }
 if (!gameover())
 cout << "Draw";
 else if (turn)
 cout << "Player 'O' has won";
 else if (!turn)
 cout << "Player 'X' has won";
}

Here's a couple changes I myself made:

1. I changed turn to a bool value to make it easy to invert and compare (before it was a char containing either 'X' or 'O').

2. I removed bool draw and instead I check if the game isn't over after there are no turns left (before it was a part of gameover() function and changed values each time the function was called).

3. Instead of checking the whole input to be made out of digits I search for the closest non-digit value and go back to the start of the loop.

4. Instead of comparing all the values from the board array I created 4 extra variables: rows[3], columns[3], diagonal, anti_diagonal. Those variables change their value each time a turn is made by incrementing (if it's X's turn) or decrementing (if O's turn). At the end those values are used to check for win conditions. For example rows[0] would represent the 1st row and the value can only be > 2 if there are 3 X's or < -2 if there are 3 O's.

Those are all the major changes I made alongside some minor cleanup like removing unnecessary repeating checks.

P.S. At the time of writing this question I noticed that in my gameover() function the if statement inside of the for loop also checks for diagonal and anti_diagonal values even tho they only need to be checked once so that might be something worth changing.

• chux: "Tic-Tac-Toe game in C++" and cout << "T I C K -- T A C -- T O E -- G A M E"; spell Tic/Tick differently. Any particular reason for that?
• Noby: @chux-ReinstateMonica This code is a modification of the one that my friend made, so it wasn't exactly my choice. Although now that you say that I do think I'll change it to "Tic"
• thegreatemu: Did your friend give you permission to publish their code? Even if you have made some modifications to it, your statements make it clear that some of their original code remains.
• Noby: @thegreatemu I feel like I've answered this question before, but maybe the comment got deleted or something. The only things that are left from my friend's code are the names of the functions and that one cout at the start of the main func. Everything else was written by me so I doubt that it violates publishing rights.
• Eric Lippert: First: there's no such thing as "improving performance" without a customer focused performance metric. What's your metric? Second, never focus on performance before readability. Make the code correct, then make it elegant, then measure its performance against metrics, and only if your correct, elegant program fails to meet its metrics should you even start to optimize it.

Answer 1

I think the only 'major' change I might make is to replace the switch..case with if...else to remove duplicate code and actually use intchoice

if (intchoice >= 1 && intchoice <= 9) {
 col = (intchoice - 1) % 3;
 row = (intchoice - 1) / 3;
} else {
 cout << "Invalid Move\n";
 continue;
}

In gameover you could check diagonal after row and column check

for (int i = 0; i < 3; i++)
 if (rows[i] > 2 || rows[i] < -2 || columns[i] > 2 || columns[i] < -2)
 return true;
if (diagonal > 2 || diagonal < -2 || anti_diagonal > 2 || anti_diagonal < -2)
 return true;
return false;

• Noby: I've actually been wondering about something. Wouldn't it be slightly better to put diagonal check before row and column? Since that way if it is diagonal, then there would be no need to go through the for loop before the program realises that
• depperm: technically, but the loop is 3 long. In larger applications for sure, but for this app its basically negligible

Answer 2

Avoid using namespace std;

If you are coding professionally you probably should get out of the habit of using the using namespace std; statement. The code will more clearly define where cout and other identifiers are coming from (std::cin, std::cout). As you start using namespaces in your code it is better to identify where each function comes from because there may be function name collisions from different namespaces. The identifiercout you may override within your own classes, and you may override the operator << in your own classes as well. This stack overflow question discusses this in more detail.

Avoid Global Variables

It is very difficult to read, write, debug and maintain programs that use global variables. Global variables can be modified by any function within the program and therefore require each function to be examined before making changes in the code. In C and C++ global variables impact the namespace and they can cause linking errors if they are defined in multiple files. The answers in this stackoverflow question provide a fuller explanation.

Readability and Maintainability

If you are going to use a switch statement, each statement should be on it's own line. This is to make it easier to find code and to modify existing code. Always remember you may not be the one maintaining the code so it needs to be easy to read and modify.

Example:

 switch (intchoice)
 {
 case 1:
 row = 0;
 col = 0;
 break;
 case 2:
 row = 0;
 col = 1;
 break;
 case 3:
 row = 0;
 col = 2;
 break;
 ...
 }

This is also true of any other code such as declarations of variables:

int row;
int rows[3];
int col;
int columns[3];
int diagonal;
int anti_diagonal;

Variable Declarations

Variables should be declared as needed. In the original version of C back in the 1970s and 1980s, which is the language C++ is based on, variables had to be declared at the top of the function. That is no longer the case, and a recommended programming practice to declare the variable as needed. In C++ the language doesn't provide a default initialization of the variable so variables should be initialized as part of the declaration. For readability and maintainability each variable should be declared and initialized on its own line.

void player_turn()
{
 while (true)
 {
 std::string choice;
 int intchoice = -1;
 if (turn)
 cout << "Player - 1 [X] turn : ";
 else if (!turn)
 cout << "Player - 2 [0] turn : ";
 getline(cin, choice); // Prevents multiple turns in one input
 if (!isint(choice))
 continue;
 std::stringstream(choice) >> intchoice;
 int row = 0;
 int col = 0;

Switch Statements

There are multiple problems using switch/case statements in the C++ programming language, one is that they take up a lot of space in the program, a second is that there are faster ways of getting the result if you are worried about the performance of the code. The example that @depperm provided in their answer is one way, another way is to use tables to translate the data.

static int rowConverter[] = { -1, 0, 0, 0, 1, 1, 1, 2, 2, 2};
static int columnConverter[] = {-1, 0, 1, 2, 0, 1, 2, 0, 1, 2};
void player_turn()
{
 ... 
 if (intchoice >= 1 && intchoice <= 9) {
 col = columnConverter[intchoice];
 row = rowConverter[intchoice];
 }
 else {
 cout << "Invalid Move\n";
 continue;
 }

Given the amount of data this method and @depperm solutions are about equal. If you were to enlarge the grid, say make it 8 by 8, the solution by @depperm might be better.

Missing Error Message

When the user input is not an integer there is no error message to the the user to re-enter the data, all the other tests on the user input report an error.

• Noby: A couple of questions. Regarding variable declaration. When is it fine to declare multiple variables at once and does it affect the performance in any way? Same with int intchoice = -1;, is there any reason you're immediately assigning a value to it and how does it affect performance compared to using it as a global variable? I know there's always a tradeoff between Readability/Maintainability and Performance, so I wanna find out more about why certain decisions are made. Sorry if I'm asking too many questions and thanks for the help
• pacmaninbw: @Noby There is generally no tradeoff between Readability/Maintainability and performance, especially if you use the optimizing options on the compiler. The optimizer is much better at generating fast code than human programmers these days. The optimizer puts local variables (non-global variables) into registers, which will be faster than global variables which will need to be fetched from memory. From a maintenance point of view it is never okay to declare multiple variables at once. Not initializing variables when they are declared is one of the major causes of bugs in C++ and C.
• AndyB: While I agree with you about generally putting one statement on a line, I might not do that in the original switch (intchoice) statement. With these short case blocks, putting the entire case on a line shows the parallel structure and makes it obvious if there's a typo. Of course, the fact that it's so parallel suggests that another solution (your tables, or @depperm's division/remainder) would be better than using a switch in the first place.
• Peter Cordes: @Noby: Local variables are usually faster than globals since they can be fully optimized away, or their usage transformed more; the compiler knows that global functions like std::cout operator<< can't read local vars (if their address hasn't been taken and stored somewhere globally reachable, i.e. escape analysis). But global vars have to be in sync (memory matching the C++ abstract machine) at every non-inline function call. See How to remove "noise" from GCC/clang assembly output? and especially Matt Godbolt's video on compilers, linked from there.