4
\$\begingroup\$

I was hoping someone could review my C++ console based noughts and crosses (Tic Tac Toe) application. I have not looked at any other documentation, this is purely programmed from the top of my head. Can you tell me what I've done well, what I haven't done well, and suggest any improvements I can make to it. If you decide to run this program ensure C++ 17 is specified (not that you need to be told that). Please treat it how it's supposed to be treated. I haven't included validation for inputs.

#include <iostream>
#include <vector>
#include <map>
#include <utility>
#include <algorithm>
void Displaygrid(const std::vector<std::vector<std::pair<int, unsigned char>>>&grid)
{
    std::for_each(grid.begin(), grid.end(), [&](auto pairVec) 
        {
        std::for_each(pairVec.begin(), pairVec.end(), [&](auto pair) 
            {
                if (pair.second != '-') {
                    std::cout << pair.second << "    ";
                }
                else 
                {
                    std::cout << pair.first << "    ";
                }
            }
        );
        std::cout << "\n";
       });
}

bool CheckForAWinner(const std::vector<std::vector<std::pair<int, unsigned char>>>&grid, const unsigned char key) 
{
    if (grid.at(0).at(0).second == key  && grid.at(0).at(1).second == key && grid.at(0).at(2).second == key) {
        std::cout << key << " is the winner" << std::endl;
        return true;
    }
    else if (grid.at(1).at(0).second == key && grid.at(1).at(1).second == key && grid.at(1).at(2).second == key) {
        std::cout << key << " is the winner" << std::endl;
        return true;
    }
    else if (grid.at(2).at(0).second == key && grid.at(2).at(1).second == key && grid.at(2).at(2).second == key) {
        std::cout << key << " is the winner" << std::endl;
        return true;
    }
    else if (grid.at(0).at(0).second == key && grid.at(1).at(0).second == key && grid.at(2).at(0).second == key) {
        std::cout << key << " is the winner" << std::endl;
        return true;
    }
    else if (grid.at(0).at(1).second == key && grid.at(1).at(1).second == key && grid.at(2).at(1).second == key) {
        std::cout << key << " is the winner" << std::endl;
        return true;
    }
    else if (grid.at(0).at(2).second == key && grid.at(1).at(2).second == key && grid.at(2).at(2).second == key) {
        std::cout << key << " is the winner" << std::endl;
        return true;
    }
    else if (grid.at(0).at(0).second == key && grid.at(1).at(1).second == key && grid.at(2).at(2).second == key) {
        std::cout << key << " is the winner" << std::endl;
        return true;
    }
    else if (grid.at(2).at(0).second == key && grid.at(1).at(1).second == key && grid.at(0).at(2).second == key) {
        std::cout << key << " is the winner" << std::endl;
        return true;
    }
    else if (grid.at(2).at(0).second == key && grid.at(1).at(1).second == key && grid.at(0).at(2).second == key) {
        std::cout << key << " is the winner" << std::endl;
        return true;
    }
    else 
    {
        return false;
    }
}
bool ClaimSquare(std::vector<std::vector<std::pair<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 PlayerMove(std::vector<std::vector<std::pair<int, unsigned char>>>& grid, unsigned char player) 
{
    int move = 0;
    do 
    {
        move = 0;
        Displaygrid(grid);
        std::cout << player << " turn: " << std::endl;
        std::cout << "Enter a number on the grid (e.g. 1): ";
        std::cin >> move;
    } 
    while (ClaimSquare(grid, move, player) == false);
}

void PlayNoughtsAndCrosses() 
{
    const unsigned char player1 = 'O';
    const unsigned char player2 = 'X';
    std::vector<std::vector<std::pair<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,'-') }
    };
    do 
    {
        PlayerMove(grid, player1);

        if (CheckForAWinner(grid, player1)) 
        {
            break;
        }

        PlayerMove(grid, player2);
    } 

    while (CheckForAWinner(grid, player2) == false);
}

int main()
{
    PlayNoughtsAndCrosses();
}
\$\endgroup\$
4
\$\begingroup\$

Overview

Not much in the way of OO design.
Not required but unusual for C++ application. With a bit of abstraction you can make multiple types of player all able to play the game (thus you could have a normal user or a computer plater play the game (see blow)).

Design

Your grid seems a bit over complex:

std::vector<std::vector<std::pair<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,'-') }
};

Each time you need to find a square you search the grid. I mean its not that bad as the grid is very small. But you could have made that very easy by simply having an array of 9 squares and indexing directly into it.

char grid[9] = {'-', ....};

It would have made printing different but getting the square would be a lot simpler.

grid[move - 1] returns the square you are looking for.

Even if you had gone for a simple 2D array:

char grid[3][3] = {{'-', ....};

Then access to the square would have been:

grid[(move - 1) / 3][(move - 1) % 3] returns the square you are looking for.

Code Review

Looks like you could have designed the game loop in a simpler way:

const unsigned char player1 = 'O';
const unsigned char player2 = 'X';
do 
{
    PlayerMove(grid, player1);

    if (CheckForAWinner(grid, player1)) 
    {
        break;
    }

    PlayerMove(grid, player2);
} 
while (CheckForAWinner(grid, player2) == false);

Why not use a loop that switch between players?

int currentPlayer = 1;
const unsigned char player[2] = {'O', 'X'};
do 
{
    currentPlayer = (currentPlayer + 1) % 2;
    PlayerMove(grid, player[currentPlayer]);
} 
while (CheckForAWinner(grid, player[currentPlayer]) == false);

You have the same line twice:

else if (grid.at(2).at(0).second == key && grid.at(1).at(1).second == key && grid.at(0).at(2).second == key) {
    std::cout << key << " is the winner" << std::endl;
    return true;
}
else if (grid.at(2).at(0).second == key && grid.at(1).at(1).second == key && grid.at(0).at(2).second == key) {
    std::cout << key << " is the winner" << std::endl;
    return true;
}

Not sure the std::for_each() is best here. Why not use a range based for?

void Displaygrid(const std::vector<std::vector<std::pair<int, unsigned char>>>&grid)
{
    std::for_each(grid.begin(), grid.end(), [&](auto pairVec) 
        {
        std::for_each(pairVec.begin(), pairVec.end(), [&](auto pair) 

Like this:

void Displaygrid(const std::vector<std::vector<std::pair<int, unsigned char>>>&grid)
{
    for(auto const& row: grid) {
        for(auto const& cell: row) { 

OO Design

class Grid
{
    // Interface for board here:
};
class Player
{
    public:
        virtual ~Player() {}
        virtual int getNextMove(Grid const& grid) = 0;
};

class Game
{
    public:
        Game(Grid& board, Player& p1, Player& p2);
        Player& play(); // Play the game return a ref to winner.
};

class Human: public Player
{
    public:
        virtual int getNextMove(Grid const& grid)
        {
             // Ask user for their move (like your app above).
        }
};

class Robot: public Player
{
    public:
        virtual int getNextMove(Grid const& grid)
        {
             // Your AI code here.
        }
};
\$\endgroup\$
5
  • \$\begingroup\$ Thanks very much for your suggestions! The reason I didn’t use a RAW 2D array of chars and used a std::vector of std::vector of std::pairs instead is because I needed a way to access the grid coordinate and store the O or X at the coordinate, using the coordinate as the key to access the value. \$\endgroup\$ – George Austin Bradley Dec 3 '20 at 18:46
  • \$\begingroup\$ @Martin what should Game.play return in the case of a draw? \$\endgroup\$ – Roland Illig Dec 3 '20 at 20:35
  • \$\begingroup\$ For the life of me I am unable to execute your OOP design for my program. I understand virtual functions and abstract classes to achieve dynamic polymorphism, however, I can't code this logically. \$\endgroup\$ – George Austin Bradley Dec 4 '20 at 12:55
  • \$\begingroup\$ @GeorgeAustinBradley Create a gist and show me what you have done so far. \$\endgroup\$ – Martin York Dec 4 '20 at 17:57
  • \$\begingroup\$ Hi Martin York. I've submitted through another code review post here: If you could review it. I based it on your design with a few modifications. codereview.stackexchange.com/questions/253050/… \$\endgroup\$ – George Austin Bradley Dec 4 '20 at 17:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.