C++ OOP Tic Tac Toe

Question

This is a follow up to my question here. Well, it ain't exactly a follow-up, but more like my next project after the last one

I created a tic tac toe game using object-oriented programming

You all already know how the tic tac toe works so I won't be wasting your time by explaining to you how it works

I'm looking for feedback on absolutely everything that could make me a better programmer, especially a better C++ programmer, also how to use class, function better, and how to use OOP correctly, and these:

• Optimization
• Bad practice and good practice
• Code structure
• Functions and variable naming
• Bugs
• Improving class and function usage
• How to correctly use OOP
• Oh, also how to add comment properly
• etc

Thank you very much!

I'm using Visual Studio Community 2019 ver 16.7.7

Globals.h

#ifndef GUARD_GLOBALS_H
#define GUARD_GLOBALS_H

namespace
{
    enum class Players : char
    {
        PLAYER_X = 'X',
        PLAYER_O = 'O'
    };
}

#endif // !GUARD_GLOBALS_H

board.h

#ifndef GUARD_BOARD_H
#define GUARD_BOARD_H

#include "player.h"

class Board
{
private:
    char board[9];
    // This is suppose to be a place to put the score
    // But I don't know how to implement it yet
    int scoreX{};
    int scoreO{};
public:
    Board();

    void printBoard() const;
    void markBoard(const size_t& choseNum, const char& player, bool& inputPass);
    char checkWin(bool& isDone, int& countTurn);
    void printWinner(bool& isDone, int& countTurn);
};

#endif // !GUARD_BOARD_H

board.cpp

#include "board.h"

#include <iostream>

// To set the board with numbers
Board::Board()
{
    int j{ 1 };
    for (int i = 0; i < 9; i++)
    {
        board[i] = '0' + j++;
    }
}

void Board::printBoard() const
{
    system("cls");

    std::cout << "   |   |  " << "\n";
    std::cout << " " << board[0] << " | " << board[1] << " | " << board[2] <<   "\tPlayer X: " << scoreX << "\n";
    std::cout << "___|___|__" <<                                                "\tPlayer O: " << scoreO << "\n";
    std::cout << "   |   |  " << "\n";
    std::cout << " " << board[3] << " | " << board[4] << " | " << board[5] << "\n";
    std::cout << "___|___|__" << "\n";
    std::cout << "   |   |  " << "\n";
    std::cout << " " << board[6] << " | " << board[7] << " | " << board[8] << "\n";
    std::cout << "   |   |  " << "\n\n";

}

// To change the board to which the player choose the number
void Board::markBoard(const size_t& choseNum, const char& player, bool& inputPass)
{
    char checkNum = board[choseNum - 1];
    // To check if the number that the player choose is available or not 
    if (checkNum != (char)Players::PLAYER_X && checkNum != (char)Players::PLAYER_O)
    {
        // To check if the number that the player input
        if (choseNum >= 1 && choseNum <= 9)
        {
            board[choseNum - 1] = player;
            inputPass = true;
        }
        else
        {
            std::cout << "CHOOSE THE AVAILABLE NUMBER!\nTRY AGAIN: ";
        }
    }
    else
    {
        std::cout << "SPACE HAS ALREADY BEEN OCCUPIED\nTry again: ";
    }
}

/*
There is probably a better way to do this. But, I don't know how tho
Maybe someday I could improve the checking for win but right now 
this is good enough

Also, there are a lot of magic number here such as 8, 2, 6 and 7.
I've tried to remove the magic number but I don't know how.
*/

// Check the board if there is player with parallel set or not
char Board::checkWin(bool &isDone, int &countTurn)
{
    /*
    I use middleboard and initialize it to board[4] because in order 
    for a player to win diagonally they have to acquire the 
    middle board first. So, I initialize middleboard to board[4]
    hoping it could remove the magic number

    and I initialize i to 0 and j to 8 because the checking is 
    begin from the top left corner-middle-bottom right corner 
    if it false then I add add 2 to i and substract 2 from j
    because now the checking is top right corner-middle-bottom left corner
    */

    // Check diagonal win
    size_t middleBoard = board[4];
    for (size_t i = 0, j = 8; i <= 2 && j >= 6; i+=2, j-=2)
    {
        // If all the board is occupied by the same player then the same player win
        if (middleBoard == board[i] && board[i] == board[j])
        {
            //This is suppose to add score, but I don't know how to implement it yet
            board[middleBoard] == (char)Players::PLAYER_X ? scoreX++ : scoreO++;
            isDone = true;
            return middleBoard; // To return the character of the player who won
        }
    }

    /*
    I initialize initialNum to 0 as a starting point for the checking. 
    Initilialized i to 1 and j to 2
    The checking is like this, top left corner-middle top-top right corner
    If it false then the I add 3 to initialNum to make middle left as the
    starting point, then add 3 to i and j so it the next checking is 
    middle left-middle-middle right, and so on
    */

    // Check horizontal win
    size_t initialNum = 0;
    for (size_t i = 1, j = 2; i <= 7 && j <= 8; i += 3, j += 3)
    {
        if (board[initialNum] == board[i] && board[i] == board[j])
        {
            board[initialNum] == (char)Players::PLAYER_X ? scoreX++ : scoreO++;
            isDone = true;
            return board[initialNum];
        }
        else
        {
            initialNum += 3;
        }
        
    }
    
    /*
    I reset the initialNum to 0 and initialized i to 3 and j 6 so 
    the first check will be like this: top left corner-middle left-bottom left corner
    if it fails then i add 1 to initialNum, i, and j, so the next check will be
    middle top-middle-middle bottom and so on
    */

    // Check vertical win
    initialNum = 0;
    for (size_t i = 3, j = 6; i <= 5 && j <= 8; i++, j++)
    {
        if (board[initialNum] == board[i] && board[i] == board[j])
        {
            board[initialNum] == (char)Players::PLAYER_X ? scoreX++ : scoreO++;
            isDone = true;
            return board[initialNum];
        }
        else
        {
            initialNum++;
        }
        
    }
    // If the countTurn is 8 then there're no place to occupy anymore, thus a draw
    if (countTurn == 8)
    {
        isDone = true;
        return 'D'; // As a check for printWinner() function
    }

    countTurn++;
}

// To print who's the winner or draw
void Board::printWinner(bool& isDone, int& countTurn)
{
    if (checkWin(isDone, countTurn) == 'D')
    {
        std::cout << "It's a Draw!\n";
    }
    else
    {
        std::cout << "Congratulations!\nPlayer " << checkWin(isDone, countTurn) << " won the game!\n";
    }
    
}

player.h

#ifndef GUARD_PLAYER_H
#define GUARD_PLAYER_H

#include "Globals.h"
#include "board.h"

class Board;

class Player
{
private:
    char mainPlayer;
    char secondPlayer;
    char turnPlayer = mainPlayer;

public:
    void choosePlayer(bool &choosePass);
    void movePlayer(Board& myBoard);
    void switchPlayer();
};

#endif // !GUARD_PLAYER_H

player.cpp

#include "player.h"
#include "board.h"

#include <iostream>
#include <random>

// To give a choice for the player if they want to be X or O
void Player::choosePlayer(bool& choosePass)
{
    char chosePlayer;
    std::cout << "Do you want to be player X or O? ";

    while (!choosePass)
    {
        std::cin >> chosePlayer;
        // If the player type X uppercase or lowercase then they will be
        // X and the computer will be O, vice versa
        if (chosePlayer == 'x' || chosePlayer == 'X')
        {
            mainPlayer = (char)Players::PLAYER_X;
            secondPlayer = (char)Players::PLAYER_O;
            choosePass = true;
        }
        else if (chosePlayer == 'o' || chosePlayer == 'O')
        {
            mainPlayer = (char)Players::PLAYER_O;
            secondPlayer = (char)Players::PLAYER_X;
            choosePass = true;
        }
        else
        {
            std::cout << "Invalid choice\n Try again: ";
        }
    }
}

// To make a player choose a number to which they want to occupy
void Player::movePlayer(Board &myBoard)
{
    size_t choseNum;
    bool inputPass = false;

    /*
    I make it turnPlayer != mainPlayer because if I make it
    turnPlayer == mainPlayer then the computer will make the first move
    I don't know why. Probably should find out the why. But it'll do for now
    */

    // If turnPlayer is not mainPlayer then it's the player's move
    if (turnPlayer != mainPlayer)
    {
        std::cout << "Player " << mainPlayer << " choose a number: ";

        while (!inputPass)
        {
            if (std::cin >> choseNum)
            {
                myBoard.markBoard(choseNum, mainPlayer, inputPass); //Go to markBoard function in board.cpp
            }
            else
            {
                std::cout << "Invalid input type (Type only number)\nTry again: ";
                std::cin.clear();                                                   // To clear the input so 
                std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n'); // the player can input again
            }
        }
    }
    // If the turnPlayer is mainPlayer then it's the computer's move
    else
     {
         while (!inputPass)
         {
             // To make a random move for the computer
             std::random_device rd;
             std::mt19937 gen(rd());
             std::uniform_int_distribution<> distrib(1, 9);
             choseNum = distrib(gen);

             myBoard.markBoard(choseNum, secondPlayer, inputPass);
         }
     }
}

// To change turn, if the player finishes then the computer will make the move
void Player::switchPlayer()
{
    turnPlayer = (turnPlayer == mainPlayer) ? secondPlayer : mainPlayer;
}

main.cpp

#include "board.h"
#include "player.h"

int main()
{
    Board myBoard;
    Player mainPlayer;
    
    int countTurn{ 0 };
    bool choosePass = false;
    bool isDone = false;
    

    myBoard.printBoard(); // To print the initial board with numbered spaces
    
    while (!isDone)
    {
        if (!choosePass)
        {
            mainPlayer.choosePlayer(choosePass);
        }

        mainPlayer.movePlayer(myBoard);
        myBoard.printBoard();
        mainPlayer.switchPlayer();
        myBoard.checkWin(isDone, countTurn);
    }
    myBoard.printWinner(isDone, countTurn);
}

Answer 1

Should there be Globals.h?

I disagree. Globals.h has a single enum that is only meaningful to your Player class. So why create a new Header? Why can't enum class Players just be in Player.cpp? That is the only file that ever accesses the contents of Players. I believe the best thing to do here is to create an anonymous namespace in Player.cpp and let it remain there.

// Player.cpp
namespace {
    enum class Players { ... };
}

Also, be careful while using an unnamed namespace in a header file

---

Use std::tolower

instead of comparing with both cases of a character, use std::tolower to directly convert a character into lowercase. This would convert

std::cin >> chosePlayer;

if (chosePlayer == 'x' || chosePlayer == 'X') {...}
else if (chosePlayer == 'o' || chosePlayer == 'O') {...}
else {...}

Into

std::cin >> chosePlayer;
chosePlayer = std::tolower(chosePlayer, std::locale());

if (chosePlayer == 'x' ) {...}
else if (chosePlayer == 'o') {...}
else {...}

^{#include <locale>}

• Note that on entering anything > 1 character, the code will accept the first one. For example, if the user enters cplusplus, chosePlayer is now set to c.

---

Use the enum class you created

You created an enum removing the magic x and o. Why are you still using them here?

if (chosePlayer == 'x' ) 
else if (chosePlayer == 'o')

Use the values of enum class Players here.

---

Use an enum here

While some might disagree, I think an enum is better compared to enum class here. The reason being you don't have to constantly cast the values to char whenever you want to compare an enum and char type.
If it's only going to be visible in a single .cpp file like I earlier mentioned, you're most probably not going to have name conflicts.

enum Player : char { PLAYER_1 = 'x', PLAYER_2 = 'o' };

From Player::chosePlayer()

void Player::choosePlayer(bool& choosePass)
{
    char chosePlayer;
    std::cout << "Do you want to be player X or O? ";

    while (!choosePass)
    {
        std::cin >> chosePlayer;
        // If the player type X uppercase or lowercase then they will be
        // X and the computer will be O, vice versa
        if (chosePlayer == 'x' || chosePlayer == 'X')
        {
            mainPlayer = (char)Players::PLAYER_X;
            secondPlayer = (char)Players::PLAYER_O;
            choosePass = true;
        }
        else if (chosePlayer == 'o' || chosePlayer == 'O')
        {
            mainPlayer = (char)Players::PLAYER_O;
            secondPlayer = (char)Players::PLAYER_X;
            choosePass = true;
        }
        else
        {
            std::cout << "Invalid choice\n Try again: ";
        }
    }
}

If you want to indicate whether the inputted values are good or bad, why are you passing a reference to a bool variable? Why not return true if the input is good, and false if the input isn't? Passing by reference implicitly passes a pointer, so you are actually passing a pointer to a bool variable in the function. You will have to pass by reference if you go with your current logic, but the thing is

sizeof(bool) == 2
sizeof(bool*) == 8

For that reason, and for simplicity, I believe simply returning True or False will be better

---

Checking for a winner

Your current algorithm of checking for a winner is very long and hard to read. There are better ways. This thread will provide a lot of useful information about them. The simplest of all

constexpr int NB_WIN_DIR = 8;
constexpr int N = 3; // please think of a better name 

constexpr int wins[NB_WIN_DIR][N] {
    {0, 1, 2}, // first row
    {3, 4, 5}, // second row
    {6, 7, 8}, // third row
    {0, 3, 6}, // first col
    {1, 4, 7}, // second col
    {2, 5, 8}, // third col
    {2, 4, 6}, // diagonal
    {0, 4, 8}, // antidiagonal
};

for (int i = 0; i < NB_WIN_DIR ;i++)
{
    if (board[wins[0]] == board[wins[1]] and board[wins[1]] == board[wins[2]]) 
        return board[wins[0]];
}

---

When should you pass by const&?

I see a const bool& and const size_t& function arguments.
When you should pass as a constant reference

• When you want to avoid copies for large objects

As I said earlier, passing by reference implicitly passes a pointer. But the problem is

sizeof(bool) == 2
sizeof(bool*) == 8

sizeof(size_t) == 8 // depending on your machine, sometimes 4
sizeof(size_t*) == 8 

So at best, it's doing you no good at all, and possibly is doing more bad. A simple rule of thumb, you don't have to pass primitive types like int, char, double, float by const&, however, do pass by reference if you have something like std::vector.

Don't get me wrong, you should pass by reference if a function should be modifying the original value of an object. But if this isn't the intent, only use it for large objects.

---

Re-think your code structure

I really dislike this class

class Player
{
private:
    char mainPlayer;
    char secondPlayer;
    char turnPlayer = mainPlayer;

public:
    void choosePlayer(bool &choosePass);
    void movePlayer(Board& myBoard);
    void switchPlayer();
};

Your Player class doesn't hold any information about a single player. All your member functions modify the values of your board. All of this actually belongs to your Board class. A player is actually just a char, either o or x. It literally holds no other information than that. What you should do is simply represent a player using an enum like you already do

enum Player { ... };

class Board{ 
      Player human;
      Player bot;  
};

the bot would be the computer who is playing against you, and human would be the actual user.

What think should be represented using a class is a simple move. A move has two things.

• The square
• The player

Everywhere in your program, you have passed around these two separately, why not create a simple struct that would hold it?

struct Move {
    int square;
    Player player;
}

I wrote out a very basic example of how this game can be re-structured.

class Game
{
    private:
        struct Move {
            Player player;
            int square;

            Move(const int square, const Player player)
                : square(square), player(player)
            {}
        };

        enum Player {
            PLAYER_1, PLAYER_2, NONE 
        };

        template < typename T, size_t N > using array = std::array < T, N  >;


        array < char, NB_SQ > board;
        Player human;
        Player bot;

        short int turns; // number of total moves played
    

    
        void computer_move();
        Move input_move() const;
        void make_move(const Move& move);
        bool validate_move(const Move& move);

        Player check_win() const;
        bool check_draw() const;

        void print_board() const;
        void new_game(); // choose whether the player plays 'x' or 'o' here
        
    public:
        void mainloop(){
            for (;;) {
                const Move& move = input_move();
                make_move(move);
                computer_move();

                if (check_win()) // ...
                if (check_draw()) // ...

            }
        }
        
        Game() { new_game(); }

};

int main() {
    Game game;
    game.mainloop();
}

---

About system("cls")

Your current program will not work on operating systems that aren't windows. On most other systems, the word is clear. To make this more portable you can use an #ifdef statement to check for the operating system

void clear_screen()
{
#ifdef _WIN32
    system("cls");
#else 
    system("clear");
#endif
}

Read more

Answer 2

Overall Observations

The code in main() is well sized, nice and tight, very readable. The only down side to main() is the comment which really isn't necessary.

There seems to be mutual dependencies between Board and Player, in software design this is known as a tight coupling and it generally indicates a bad design.

I only see one instance of the Player class and I would expect to see 2 instances, one for each player.

Keep working on your object designs to remove tight coupling, and try to follow the SOLID programming principles. Learn some object oriented design patterns such as composition.

SOLID is a mnemonic acronym for five design principles intended to make software designs more understandable, flexible and maintainable. This will help you design your objects and classes better.

1. The Single Responsibility Principle - A class should only have a single responsibility, that is, only changes to one part of the software's specification should be able to affect the specification of the class.
2. The Open–closed Principle - states software entities (classes, modules, functions, etc.) should be open for extension, but closed for modification.
3. The Liskov Substitution Principle - Objects in a program should be replaceable with instances of their subtypes without altering the correctness of that program.
4. The Interface segregation principle - states that no client should be forced to depend on methods it does not use.
5. The Dependency Inversion Principle - is a specific form of decoupling software modules. When following this principle, the conventional dependency relationships established from high-level, policy-setting modules to low-level, dependency modules are reversed, thus rendering high-level modules independent of the low-level module implementation details.

Turn on a High Level of Warning, Don't Ignore Warnings

There are 2 warnings when I compile and both warnings indicate possible logic problems in the code.

One warning is possible loss of data on this line:

            return middleBoard; // To return the character of the player who won  

in Board::checkwin(). This warning is because the code is returning a variable declared as size_t as a char.

The second warning is also about Board::checkwin(), the warning is not all control paths return a value which is issued on the last line of the function. This may be the more serious of the 2 warning since it definitely indicates possible logic problems in the code.

Prefer C++ Style Casts Over Old C Style Casts

The following line of code is using an old C style cast:

            board[initialNum] == (char)Players::PLAYER_X ? scoreX++ : scoreO++;

C++ has it's own casts that provide better warnings and compiler errors, these are static casts and dynamic casts. Static casts occur at compile time and provide possible errors or warnings if the cast is not type safe. On the line of code above a static cast is more appropriate.

            board[initialNum] == (static_cast<char>(Players::PLAYER_X)) ? scoreX++ : scoreO++;

Prefer Self Documenting Code Over Comments

There are too many comments in the code. One of the things that new programmers are not aware of is the maintenance of code, the code you write may be in use for 20 or more years and it is quite possible that you won't be working for the company that long. If there are a lot of comments in the code the comments must be maintained as well as the code itself, and this can double the amount of work to be done. It is better to write self documenting code using clear variable, class and function names. Use comments for design decisions or high level abstractions. If a function requires a special flow state it in a comment block preceding the function.

DRY Code

There is a programming principle called the Don't Repeat Yourself Principle sometimes referred to as DRY code. If you find yourself repeating the same code multiple times it is better to encapsulate it in a function. If it is possible to loop through the code that can reduce repetition as well. The function Board::checkWin() contains redundant code in the 3 loops that check for wins. There are multiple ways to fix this and a good one has been suggested in another answer.

Complexity

The function Board::checkWin() is too complex (does too much). Rather than returning a character Board::checkWin() should return a boolean value indicating if it is a win or not. Other functions should implement updating the board with the proper characters. The complexity of this function has lead to the warning not all control paths return a value.

Magic Numbers

There are Magic Numbers in the Board::checkWin() function in each of the loops that check if there is a win, it might be better to create symbolic constants for them to make the code more readble and easier to maintain. These numbers may be used in many places and being able to change them by editing only one line makes maintainence easier.

Numeric constants in code are sometimes referred to as Magic Numbers, because there is no obvious meaning for them. There is a discussion of this on stackoverflow.