Noughts & Crosses (Tic Tac Toe) OOP Design

Question

I was wondering if someone could review my object oriented design for my Noughts & Crosses program. Yesterday I posted a procedural version and now I've done an OOP version.

Can someone tell me what I've done well, what I haven't done well, and provide any suggestions on how I can improve this program? I included inheritance to practice with dynamic polymorphism. I haven't done error checking so treat my program how it's supposed to be entered.

#include <iostream>
#include <vector>
#include <map>
#include <utility>
#include <algorithm>


bool ClaimSquare(std::vector<std::map<int, unsigned char>>& grid, int move, const unsigned char player)
{
    for (auto& vec : grid)
    {
        auto validSquare = std::find_if(vec.begin(), vec.end(), [&](auto pair) {
            return pair.first == move;
            });
        if (validSquare != vec.end())
        {
            if (validSquare->second == '-')
            {
                validSquare->second = player;
                return true;
            }
            else
            {
                std::cout << "This square has already been claimed. Choose a different square!" << std::endl;
                return false;
            }
        }
    }
}
void Displaygrid(const std::vector<std::map<int, unsigned char>>& grid)
{
    for (auto const& row : grid)
    {
        for (auto const& cell : row)
        {
            if (cell.second != '-') {
                std::cout << cell.second << "        ";
            }
            else
            {
                std::cout << cell.first << "         ";
            }
        }
        std::cout << "\n\n";
    }
}
class Player
{
private:
protected:
     std::string m_type;
    unsigned char m_name;
public:
    std::string GetType() { return m_type; }
    Player(unsigned char name, std::string&& type = "Player") :m_name(name),m_type(type) {}
    virtual ~Player() = default;
    virtual unsigned char GetName() { return m_name; }
};

class Human : public Player
{

public:
    Human(unsigned char name) :Player(name, "Human") {}
    std::string GetType() { return m_type; }
};

class Robot : public Player
{

public:
    Robot(unsigned char name) :Player(name, "Robot"){}
    std::string GetType() { return m_type; }

    
};

class Game
{
private:

    std::vector<Player*> m_p;
public:
    Game(Player *p1, Player*p2)
    {
        m_p.push_back(p1);
        m_p.push_back(p2);
    }
//Does anyone know if there's a better way of checking for winner?
    bool CheckForAWinner(std::vector<std::map<int, unsigned char>>& m_board, const unsigned char key)
    {
        if (m_board.at(0).at(1) == key && m_board.at(0).at(2) == key && m_board.at(0).at(3) == key) {
            std::cout << key << " is the winner" << std::endl;
            return true;
        }
        else if (m_board.at(1).at(4) == key && m_board.at(1).at(5) == key && m_board.at(1).at(6) == key) {
            std::cout << key << " is the winner" << std::endl;
            return true;
        }
        else if (m_board.at(2).at(7) == key && m_board.at(2).at(8) == key && m_board.at(2).at(9) == key) {
            std::cout << key << " is the winner" << std::endl;
            return true;
        }

        else if (m_board.at(0).at(1) == key && m_board.at(1).at(4) == key && m_board.at(2).at(7) == key) {
            std::cout << key << " is the winner" << std::endl;
            return true;
        }
        else if (m_board.at(0).at(2) == key && m_board.at(1).at(5) == key && m_board.at(2).at(8) == key) {
            std::cout << key << " is the winner" << std::endl;
            return true;
        }
        else if (m_board.at(0).at(3) == key && m_board.at(1).at(6) == key && m_board.at(2).at(9) == key) {
            std::cout << key << " is the winner" << std::endl;
            return true;
        }

        else if (m_board.at(0).at(1) == key && m_board.at(1).at(5) == key && m_board.at(2).at(9) == key) {
            std::cout << key << " is the winner" << std::endl;
            return true;
        }
        else if (m_board.at(2).at(7) == key && m_board.at(1).at(5) == key && m_board.at(0).at(3) == key) {
            std::cout << key << " is the winner" << std::endl;
            return true;
        }
        else
        {
            return false;
        }
    }
    void play(std::vector<std::map<int, unsigned char>> &board)
    {
        int currentPlayer = 1;
        Player *ref = nullptr;
        do
        {
            int move = 0;
            currentPlayer = (currentPlayer + 1) % 2;
            do
            {
                Displaygrid(board);
                std::cout << m_p.at(currentPlayer)->GetType() << ": " << m_p.at(currentPlayer)->GetName() << " turn: " << std::endl;
                std::cout << "Enter a number on the grid (e.g. 1): ";
                std::cin >> move;
            }
            while (ClaimSquare(board, move, m_p.at(currentPlayer)->GetName()) == false);
        } 
        while (CheckForAWinner(board,m_p.at(currentPlayer)->GetName()) == false);
    }
};

int main() {

    std::vector<std::map<int, unsigned char>>grid =
    {
            {std::make_pair(1, '-'), std::make_pair(2, '-'), std::make_pair(3, '-') },
            { std::make_pair(4,'-'), std::make_pair(5,'-'), std::make_pair(6,'-') },
            { std::make_pair(7,'-'), std::make_pair(8,'-'), std::make_pair(9,'-') }
    };
    
    
    Player *player = new Human('O');
    Player *player2 = new Robot('X');

    Player* robot = new Robot('O');
    Player* robot2 = new Robot('X');

    //Game 1: Human vs Human
    Game HumanVsHuman(player, player2);
    HumanVsHuman.play(grid);

    //The robot doesn't work yet. Just treat it as a human. Point is to demonstrate polymorphisim.
    Game RobotvsHumanGame(robot, robot2);
    RobotvsHumanGame.play(grid);

    //deleting memory used
    delete player;
    delete player2;
    //deleting memory used
    delete robot;
    delete robot2;
}

Answer 1

I would enable all your warnings (and treat them like errors). Warnings in C++ show logic flaw in your reasoning usally and fixing them will save you some time.

The warnings show one fatal flaw (missing return) and one oversight (unused variable).

---

You don't need a copy of GetType() in Human and Robot it already exists in Player. Then you will not need the protected region in Player it can all be private (protected is a code smell).

class Human : public Player
{
    public:
        Human(unsigned char name) :Player(name, "Human") {}
        std::string GetType() { return m_type; }
};

class Robot : public Player
{
    public:
        Robot(unsigned char name) :Player(name, "Robot"){}
        std::string GetType() { return m_type; }
};

---

You have not defined how a player makes a move. The Player object should define a virtual (abstract) method for getMove() that for player asks for user input. Then you can call this method in the Game play() method.

class Player
{
    public:
        virtual int getMove() = 0;
};
class Human
{
    public:
        virtual int getMove() override
        {
            int move;
            std::cout << "Enter a number on the grid (e.g. 1): ";
            std::cin >> move;
            return move;
        }
};

void Game::play(std::vector<std::map<int, unsigned char>> &board)
{
    int currentPlayer = 1;
    //Player *ref = nullptr;
    do
    {
        int move = 0;
        currentPlayer = (currentPlayer + 1) % 2;
        do
        {
            Displaygrid(board);
            std::cout << m_p.at(currentPlayer)->GetType() << ": " << m_p.at(currentPlayer)->GetName() << " turn: " << std::endl;

            // Then call getMove in the loop.
            move = m_p.at(currentPlayer)->getMove();
            
        }
        while (ClaimSquare(board, move, m_p.at(currentPlayer)->GetName()) == false);
    }
    while (CheckForAWinner(board,m_p.at(currentPlayer)->GetName()) == false);    
}

---

Not sure this is a good idea.

std::vector<std::map<int, unsigned char>>grid =
{
    {std::make_pair(1, '-'), std::make_pair(2, '-'), std::make_pair(3, '-') },
    { std::make_pair(4,'-'), std::make_pair(5,'-'), std::make_pair(6,'-') },
    { std::make_pair(7,'-'), std::make_pair(8,'-'), std::make_pair(9,'-') }
};

Go back to your original design or one of the choices (this seems to be a hybrid of the two styles).

---

Don't use RAW pointers.

Player *player = new Human('O');
Player *player2 = new Robot('X');

This is how it was done in the old days. When we manually managed memory. Nowadays you practically never create things with new (unless you are deep in the bowls of something). Normally you can stick with normal objects.

Player player{'O'};
Player player2{'X'};

Game HumanVsHuman(&player, &player2);  // as you are using pointers internally
                                       // in Game you can simply pass the address
                                       // of the players here.
                                       //
                                       // Though in the long run I would pass references.

======

Alternatively, if you do want to keep pointers, you should be using smart pointers to make sure the objects are managed.

std::shared_ptr<Player> player = std::make_shared<Player>('O');

This will make sure the memory is deallocated correctly (even when exceptions are thrown).

Answer 2

You have already received an excellent review. I would suggest using aliases with using here.

Make an alias for std::vector<std::map<int, unsigned char>>

using Board = std::vector<std::map<int, unsigned char>>;

std::cout<<std::is_same<Board, std::vector<std::map<int, unsigned char>>>::value;
// 1

Now you can re-write functions like

bool ClaimSquare(Board& grid, int move, const unsigned char player){
 // ... 
}

void Displaygrid(const Board& grid){
// ...
}

If you want Board to be parameterized, then use template with using(type alias, alias template)

template<typename T, typename U>
using Board = std::vector<std::map<T, U>>;

// Define Board with any types
Board<std::int64_t, std::string> grid; //valid 
// equivalent to std::vector<std::map<std::int64_t, std::string>>


Board<std::string, std::uint8_t> grid; //valid
// equivalent to std::vector<std::map<std::string, std::uint8_t>>

Now, re-written functions would be

bool ClaimSquare(Board<int, unsigned char>& grid, int move, const unsigned char player){
 // ... 
}

void Displaygrid(const Board<int, unsigned char>& grid){
// ...
}

---

std::endl does much more than just adding a newline character. It flushes the stream too. Since you are just using it for adding a newline use '\n' or "\n". Related answer from SO