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.

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;

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.