3
\$\begingroup\$

Here is a simple Tic Tac Toe game. I would like to know how I can improve this code further.

#include <iostream>
#include <cctype>
#include <array>
#include <random>

enum struct Player : char
{
    none    = '-',
    first   = 'X',
    second  = 'O'
};

std::ostream& operator<<(std::ostream& os, Player const& p)
{
    return os << std::underlying_type<Player>::type(p);
}

enum struct Type : int
{
    row,
    column,
    diagonal
};

enum struct Diagonals : int
{
    leftTopRightBottom,
    rightTopleftBottom
};

template<std::size_t DIM>
class TicTacToe 
{
public:
    TicTacToe();

    bool isFull() const;
    void draw() const;
    bool isWinner(Player player) const;
    bool applyMove(Player player, std::size_t row, std::size_t column);

private:
    std::size_t mRemain = DIM * DIM;
    std::array<Player, DIM * DIM> mGrid;
};

template<int DIM>
struct Match 
{
    Match(Type t, int i) 
        : mCategory(t)
        , mNumber(i)
    {}

    bool operator() (int number) const
    {
        switch (mCategory)
        {
        case Type::row:
            return (std::abs(number / DIM) == mNumber);

        case Type::column:
            return (number % DIM == mNumber);

        case Type::diagonal:
            if (mNumber == static_cast<int>(Diagonals::leftTopRightBottom))
            {
                return ((std::abs(number / DIM) - number % DIM) == mNumber);
            }

            if (mNumber == static_cast<int>(Diagonals::rightTopleftBottom))
            {
                return ((std::abs(number / DIM) + number % DIM) == DIM - mNumber);
            }

        default:
            return false;
        }
    }

    Type mCategory;
    int mNumber;
};

template<std::size_t DIM>
TicTacToe<DIM>::TicTacToe()
{
    mGrid.fill(Player::none);
}

template<std::size_t DIM>
bool TicTacToe<DIM>::applyMove(Player player, std::size_t row, std::size_t column)
{
    std::size_t position = row + DIM * column;

    if ((position > mGrid.size()) || (mGrid[position] != Player::none))
    {
        return true;
    }

    --mRemain;

    mGrid[position] = player;

    return false;
}

template<std::size_t DIM>
bool TicTacToe<DIM>::isFull() const
{
    return (mRemain == 0);
}

template<std::size_t DIM>
bool TicTacToe<DIM>::isWinner(Player player) const
{
    std::array<bool, 2 * (DIM + 1)> win;

    win.fill(true);

    int j = 0;

    for (auto i : mGrid)
    {
        int x = j++;

        for (auto k = 0; k < DIM; ++k)
        {
            if (Match<DIM>(Type::column, k)(x))
            {
                win[k] &= i == player;
            }

            if (Match<DIM>(Type::row, k)(x))
            {
                win[DIM + k] &= i == player;
            }

            if (Match<DIM>(Type::diagonal, k)(x))
            {
                win[2 * DIM + k] &= i == player;
            }
        }
    }

    for (auto i : win)
    {
        if (i)
        {
            return true;
        }
    }

    return false;
}

template<std::size_t DIM>
void TicTacToe<DIM>::draw() const
{
    std::cout << ' ';
    for (auto i = 1; i <= DIM; ++i)
    {
        std::cout << "  " << i;
    }

    int j = 0;
    char A = 'A';

    for (auto i : mGrid)
    {
        if (j == 0)
        {
            std::cout << "\n " << A++;
            j = DIM;
        }
        --j;

        std::cout << ' ' << i << ' ';
    }

    std::cout << "\n\n";
}

struct Random 
{
    Random(int min, int max)
        : mUniformDistribution(min, max)
    {}

    int operator()()
    {
        return mUniformDistribution(mEngine);
    }

    std::default_random_engine mEngine{ std::random_device()() };
    std::uniform_int_distribution<int> mUniformDistribution;
};

class Game 
{
public:
    void run();

private:
    void showResult() const;
    void turn();

    static const std::size_t mDim = 4;
    int mNumberOfPlayers = 2;
    TicTacToe<mDim> mGame;
    std::array<Player, mNumberOfPlayers> mPlayers{ { Player::first, Player::second } };
    int mPlayer = 1;
    Random getRandom{ 0, mDim - 1 };
};

void Game::run()
{
    while (!mGame.isWinner(mPlayers[mPlayer]) && !mGame.isFull())
    {
        mPlayer ^= 1;
        mGame.draw();
        turn();
    }

    showResult();
}

void Game::showResult() const
{
    mGame.draw();

    if (mGame.isWinner(mPlayers[mPlayer]))
    {
        std::cout << "\n" << mPlayers[mPlayer] << " is the Winner!\n";
    }
    else
    {
        std::cout << "\nTie game!\n";
    }
}

void Game::turn()
{
    char row = 0;
    char column = 0;

    for (bool pending = true; pending;)
    {
        switch (mPlayers[mPlayer])
        {
        case Player::first:
            std::cout << "\n" << mPlayers[mPlayer] << ": Please play. \n";
            std::cout << "Row(1,2,3,...): ";
            std::cin >> row;
            std::cout << mPlayers[mPlayer] << ": Column(a,b,c,...): ";
            std::cin >> column;

            column = std::toupper(column) - 'A';
            row -= '1';

            pending = column < 0 || row < 0 || mGame.applyMove(mPlayers[mPlayer], row, column);

            if (pending)
            {
                std::cout << "Invalid position.  Try again.\n";
            }
            break;
        case Player::second:
            row = getRandom();
            column = getRandom();

            pending = mGame.applyMove(mPlayers[mPlayer], row, column);
            break;
        }
    }

    std::cout << "\n\n";
}

int main()
{
    Game game;
    game.run();
}
\$\endgroup\$
3
\$\begingroup\$

Here are some things that may help you improve your code.

Avoid non-standard extensions

While your compiler may allow them, the use of non-standard extensions makes your code non-portable and less readable to others. In particular, we have this:

int mNumberOfPlayers = 2;
// ...
std::array<Player, mNumberOfPlayers> mPlayers{ { Player::first, Player::second } };

The problem is that mNumberOfPlayers is neither const nor static, so the std::array size is indeterminate.

Reconsider the class roles

The division of the game in to Game and TicTacToe classes seems odd to me. I did not discern a logical reason for them not to be a single class. Maybe the idea was to separate the playing field from the game logic? If so, then it seems that perhaps applyMove and isWinner would more logically belong to the Game rather than the board. Match also seems to be superfluous.

Consider an array rather than an enum

The use of the enum for Player has some good features, such as type safety, but it doesn't lend itself to allowing more than two players, which seems to have been the idea behind having the mNumberOfPlayers instance variable. An array of player tokens might have been a better choice, with some tie between the number of players and the rest of the game. Similarly, the only places that Diagonals are used, they're used with a static_cast<int>. That would seem to be a sign that they shouldn't have been declared as enum.

Understand what auto does

In some instances, the use of auto is perfectly clear and well suited to the job, as when using a "range for":

for (auto i : mGrid)

However, other places it doesn't make as much sense:

for (auto i = 1; i <= DIM; ++i)

The variable i in the latter case will always be an int because 1 is an int. Some compilers will complain about comparing signed and unsigned values here.

Be consistent with template parameters

In some cases, DIM is an int, but in others it's a std::size_t. It's hard to imagine that was intentional and I can't think of a use, so it's probably better to declare them all the same way. Alternatively, see the next suggestion.

Use a constructor argument instead of a template parameter

Having the Match and TicTacToe use DIM as a template parameter instead of as a constructor argument means that the code must be recompiled to accomodate a different size board. With a bit of redesign, it could easily instead be a constructor argument, allowing flexible use without recompiling.

Reconsider member functions

The applyMove() member function does two things -- it checks to see if the move would be valid and then applies it if it's valid. I'd suggest that those two steps might be different functions since a function that checks but does not alter the board might be very handy for a smarter automatic player.

Consider allowing a std::ostream parameter for output

As it stands, the draw() function is only capable of sending its output to std::cout, but it could easily be made more flexible by allowing it to take an std::ostream as a parameter. In fact, I'd be inclined to refactor it as a friend like this:

std::ostream& operator<<(std::ostream& out, const TicTacToe& ttt) { //... }

Consider making Random a templated class

The use of the Random class is good and appropriate. One suggestion I have is that it might be a candidate for a template to allow a range of either int or unsigned values. It doesn't make much practical difference, but might be handy to quiet signed/unsigned type mismatch warnings if an unsigned were needed.

Be cautious with object construction/destruction

In the shortest possible 4x4 game (4 moves by each of 2 players) there are 1728 calls each to the Match constructor and destructor due to the way that class is used in isWinner. That is neither particularly efficient for the computer, nor does it particularly simplify the code for a human reader, so I would strongly advise rewriting the isWinner code.

Consider separating I/O from program logic

The showResult() and draw() functions are both clearly primarily related to I/O function and do nothing else. That's good design. The turn() function could similarly be refactored into the logic portion and the I/O portion, which would make it easier to see how to adapt the program to, say, a non-text GUI version, without altering the underlying game logic.

Allow rational reuse of objects

If we want to play the game twice, there is currently no provision to do so because the game state can't be reset using the existing interface. Adding a Game.reset() function would be simple to do and add to the usability of the object.

\$\endgroup\$
3
\$\begingroup\$

Several things worth pointing out here I think.

Match

There is zero reason for Match to take a category or really anything else. You don't need the Type or Diagonals enums. Given a move, you sohuld be able to determine if that move completed a row, column, or major diagonal.

// return true if given player won with the given move,
// false otherwise
bool won(Player, size_t row, size_t column);

TicTacToe

You may find it easier to just use a 2-d array to represent a 2-d object. That way:

bool applyMove(Player player, size_t row, size_t col)
{
    Player& square = mGrid.at(row).at(col);
    if (square == Player::none) {
        square = player;
        return true;
    }
    else {
        return false;
    }
}

Also you'll note I flipped the return type of applyMove. Returning true on success makes way more sense. Also this will throw std::out_of_range instead of silently doing nothing - that way you can catch that in Game and log an appropriate message. This differentiates an invalid move from attempting to play on a bad square.

Game

It is confusing to have getRandom be a member variable rather than a member function. I would change it. It took me a while to find the first time around. Now, turn() should just defer to the appropriate user:

void turn() {
    if (mPlayers[mPlayer] == Player::first) {
        userTurn();
    }
    else {
        compTurn();
    }
}

The pending loop is weird. Just have each sub-case return when they're done.

Also for the compTurn part, picking a random square each time is hugely inefficient. If there's only 1 open square left, you're spending on average N^2 loops to try to find it. Instead, first find the empty squares then pick a random one:

std::vector<std::pair<size_t, size_t>> open;
for (size_t r = 0; r < mDim; ++r) {
    for (size_t c = 0; c < mDim; ++c) {
        if (mGame.isOpen(r, c)) {
            open.emplace_back(r, c);
        }
    }
}

auto choice = chooseRandom(open);
applyMove(Player::second, choice.first, choice.second);

Also, the TicTacToe member object should really be named board. It's not the Game after all. The Game is already the Game.

Lastly, why is TicTacToe a class template? If you made the dimension a variable and used a vector<vector<Player>> instead, you could ask for the dimension as input up front.

\$\endgroup\$

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.