Tic-Tac-Toe in C++11 - follow-up 2

Question

Previous question:

Tic-Tac-Toe in C++11 - follow-up

Is there any way to improve this code?

#include <iostream>
#include <cctype>
#include <algorithm>
#include <functional>
#include <array>

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

std::ostream& operator<<(std::ostream& os, Player p)
{
    return os << static_cast<char>(p);
}

enum struct Type : int
{
    row = 0,
    column = 1,
    diagonal = 2
};

enum struct Lines : int
{
    first   = 0,
    second  = 1,
    third   = 2
};

class TicTacToe
{
public:
    TicTacToe(); 
    bool isFull() const;
    void draw() const;
    void turn(Player player);
    bool check(Player player) const;

private:
    bool applyMove(Player player, int position);

    static const std::size_t mDim = 3;
    std::array<Player, mDim * mDim> mGrid;
};

// utility functor to compute matching condition
template<int dim>
struct Match 
{
    Match(Type t, Lines i) : mCategory(t), mNumber(i){}
    bool operator() (int number) const
    {
        switch (mCategory)
        {
        case Type::row:
            return (std::abs(number / dim) == static_cast<int>(mNumber));
        case Type::column:
            return (number % dim == static_cast<int>(mNumber));
        case Type::diagonal:
            if (mNumber == Lines::first)
                return ((std::abs(number / dim) - number % dim) == static_cast<int>(mNumber));
            else
                return ((std::abs(number / dim) + number % dim) == static_cast<int>(mNumber));
        }

        return false; 
    }
    Type mCategory;
    Lines mNumber;
};

TicTacToe::TicTacToe() 
{ 
    mGrid.fill(Player::none);
}

bool TicTacToe::applyMove(Player player, int position)
{
    if (mGrid[position] != Player::none)
        return false;
    mGrid[position] = player;
    return true;
}

bool TicTacToe::isFull() const
{
    return 0 == std::count_if(mGrid.begin(), mGrid.end(),
        [](Player i)
    {
        return i == Player::none;
    });
}

bool TicTacToe::check(Player player) const
{
    // check for row or column wins
    std::array<bool, 8> win;
    win.fill(true);

    int j = 0;

    // checking condition loop
    std::for_each(mGrid.begin(), mGrid.end(),
        [&](Player i)
    {
        int x = j++;

        // columns
        if (Match<mDim>(Type::column, Lines::first)(x))
            win[0] &= i == player;;
        if (Match<mDim>(Type::column, Lines::second)(x))
            win[1] &= i == player;
        if (Match<mDim>(Type::column, Lines::third)(x)) 
            win[2] &= i == player;

        // rows
        if (Match<mDim>(Type::row, Lines::first)(x))
            win[3] &= i == player;
        if (Match<mDim>(Type::row, Lines::second)(x))
            win[4] &= i == player;
        if (Match<mDim>(Type::row, Lines::third)(x))
            win[5] &= i == player;

        // diagonals
        if (Match<mDim>(Type::diagonal, Lines::first)(x))
            win[6] &= i == player;
        if (Match<mDim>(Type::diagonal, Lines::third)(x))
            win[7] &= i == player;
    });

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

    return false;
}

void TicTacToe::draw() const
{
    //Creating a onscreen grid
    std::cout << ' ';
    for (auto i = 1; i <= mDim; ++i)
        std::cout << "  " << i;

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

    for (auto i : mGrid)
    {
        if (Match<mDim>(Type::column, Lines::first)(j++))
            std::cout << "\n " << A++;

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

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

void TicTacToe::turn(Player player)
{
    char row = 0;
    char column = 0;
    std::size_t position = 0;
    bool applied = false;

    std::cout << "\n" << player << ": Please play. \n";

    while (!applied)
    {
        std::cout << "Row(1,2,3,...): ";
        std::cin >> row;
        std::cout << player << ": Column(A,B,C,...): ";
        std::cin >> column;

        position = mDim * (std::toupper(column) - 'A') + (row - '1');

        if (position < mGrid.size())
        {
            applied = applyMove(player, position);

            if (!applied)
                std::cout << "Already Used. Try Again. \n";
        }
        else
        {
            std::cout << "Invalid position.  Try again.\n";
        }
    }

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

class Game
{
public:
    Game() = default;
    void run();

private:
    TicTacToe mTicTacToe;
    std::array<Player, 2> mPlayers{ { Player::first, Player::second } };
    int mPlayer = 1;

    void resultScreen(bool winner);
    std::function<void()>       display = std::bind(&TicTacToe::draw, &mTicTacToe);
    std::function<void(Player)> turn    = std::bind(&TicTacToe::turn, &mTicTacToe, std::placeholders::_1);
    std::function<bool(Player)> win     = std::bind(&TicTacToe::check, &mTicTacToe, std::placeholders::_1);
    std::function<bool()>       full    = std::bind(&TicTacToe::isFull, &mTicTacToe);
};

void Game::run()
{
    while (!win(mPlayers[mPlayer]) && !full())
    {
        mPlayer ^= 1;
        display();
        turn(mPlayers[mPlayer]);
    }

    resultScreen(win(mPlayers[mPlayer]));
}

void Game::resultScreen(bool winner)
{
    display();

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

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

Answer 1

Here are some things that may allow you to improve your code:

Separate responsibilities

The Model-View-Controller design pattern is often useful for programs like this. Because the view in this case is essentially just printing the board to std::cout, we can simplify a bit and just have a model, the TicTacToe class, and a controller, the Game class. Here's what the TicTacToe class looks like:

class TicTacToe
{
public:
    TicTacToe() = delete;
    TicTacToe(const TicTacToe &t) = delete;
    TicTacToe(const TicTacToe &&t) = delete;
    TicTacToe(char ch, std::size_t dim) 
        : mDim(dim), emptychar(ch), remaining(mDim*mDim), grid(remaining, emptychar) 
    { }
    bool isNotFull() const { return remaining; }
    bool isWinner(char player) const;
    bool applyMove(char player, unsigned row, unsigned column);
    friend std::ostream &operator<<(std::ostream &out, const TicTacToe &t) {
        out << ' ';
        for (std::size_t i = 1; i <= t.mDim; ++i)
            out << "  " << i;
        std::size_t j = 0;
        char A = 'A';
        for (auto& i : t.grid)
        {
            if (j == 0) {
                out << "\n " << A++;
                j = mDim;
            }
            --j;
            out << ' ' << i << ' ';
        }
        return out << "\n\n";
    }
private:
    const std::size_t mDim;
    const char emptychar;
    unsigned remaining;
    std::vector<char> grid;
};

There are some differences in this class compared to yours, so I'll point out the salient features.

Delete automatic functions which are not wanted

The way I've defined the TicTacToe class requires values to be passed to the constructor. For that reason, I've deleted the default constructor, the copy constructor and the move constructor. This prevents the class from being misused and alerts the user of the class that some things are not supported.

Isolate the internal representation from the interface

The game is played on a square grid and not a linear array (even though that may be the internal representation), so the applyMove function in the revised version takes row and column arguments rather than a linear position value.

Allow for dynamic sizing

The dimension of the board in the revised version of the TicTacToe class is a const value that is initialized with a value passed to the constructor. This allows for more than one size game to be played without recompiling. Also, this required changing from a std::array to a std::vector.

Allow for any character representations

This version does not specify the representations for an empty square, or any of the player tokens. In particular, the emptychar member function is initialized by the constructor. Perhaps more interesting is the fact that this class allows for more than two players. This can be seen most easily in the applyMove member function:

// Returns `false` if requested move was applied, otherwise true
bool TicTacToe::applyMove(char player, unsigned row, unsigned column)
{
    unsigned position = row + mDim * column;
   if ((position > grid.size()) || (grid[position] != emptychar))
        return true;
    grid[position] = player;
    --remaining;
    return false;
}

Define logical functions in a way that makes them most useful

If we look at the original isFull routine, it was always being used as !isFull() so it seems that what's actualy more useful is a method to check if the grid is not full. For this reason, the function is now isNotFull() in the redefined version.

Avoid inefficient algorithms

The original isFull routine counts empty squares each time it is called, but a more efficient (and simpler) way to do this is to simply keep a running count as the game is played.

Use clear function names

The original code has a function named check but it's not clear what it checks. I've renamed it to isWinner so that it's very clear now that it's checking to see if a particular player is a winner or not. I've also reimplemented it to work simply and efficiently no matter what size the array happens to be:

// returns true if the player is a winner
bool TicTacToe::isWinner(char player) const
{
    // check for row or column wins
    for(unsigned i = 0; i < mDim; ++i){
        bool rowwin = true;
        bool colwin = true;
        for (unsigned j=0; j < mDim; ++j) {
            rowwin &= grid[i*mDim+j] == player;
            colwin &= grid[j*mDim+i] == player;
        }
        if (colwin || rowwin) 
            return true;
    }
    // check for diagonal wins
    bool diagwin = true;
    for (unsigned i=0; i < mDim; ++i) 
        diagwin &= grid[i*mDim+i] == player;
    if (diagwin) 
        return true;
    diagwin = true;
    for (unsigned i=0; i < mDim; ++i) 
        diagwin &= grid[i*mDim+(mDim-i-1)] == player;
    return diagwin; 
}

Revise the Game class to be a controller

In the interest in clearly separating responsibilities of the classes, here is the revised Game class:

class Game
{
public:
    Game(std::size_t dim=3) : ttt(players[2], dim), player(1) {}
    void run();
    void run(const char *move);
    void turn();
    void showResult() const;
private:
    const char players[3] = { 'X', 'O', '-' };
    TicTacToe ttt;
    int player;
};

The most significant change here is that the turn method is a method of Game rather than of TicTacToe. This is important because the controller actually controls the game; the model simply reacts to the applied controls. This makes some alternatives much easier to implement as I'll describe.

Put the player character representations within the Game class

The character representations for each player and an empty space are all solely concerns of the Game class. They don't need to be in global space as originally defined.

Validate user input carefully

The current code accepts such inputs as (0,B) which should be rejected. The revised code fixes this:

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

    std::cout << "\n" << players[player] << ": Please play. \n";

    for (bool pending = true; pending; ) 
    {
        std::cout << "Row(1,2,3,...): ";
        std::cin >> row;
        std::cout << players[player] << ": Column(A,B,C,...): ";
        std::cin >> column;

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

        pending = column < 0 || row < 0 || ttt.applyMove(players[player], row, column);
        if (pending)
            std::cout << "Invalid position.  Try again.\n"; 
    }
    std::cout << "\n\n";
}

Note that it also changes the sense of the boolean variable from applied to pending which somewhat simplifies the code and requires no negations.

Eliminate pointless obfuscation

The use of std::bind is really not needed in this program and makes the program that much harder to read and understand. The revised version of run doesn't need them and is easy to read and understand:

void Game::run()
{
    while (!ttt.isWinner(players[player]) && ttt.isNotFull())
    {
        player ^= 1;
        std::cout << ttt;
        turn();
    }
    showResult();
}

Use const where possible

The resultScreen function doesn't and shouldn't modify the underlying Game class, and so it should be declared const. Also, I've changed the name of the function to a more desscriptive showResult and eliminated the need to pass a variable.

void Game::showResult() const
{
    std::cout << ttt;
    if (ttt.isWinner(players[player]))
        std::cout << "\n" << players[player] << " is the Winner!\n";
    else 
        std::cout << "\nTie game!\n";
}

Note that this code only yields correct results when the game has already ended; one could add a check ttt.isNotFull() to handle any mid-game requests for results.

Consider having the computer play by itself

By separating the turn function in Game from the applyMove function in TicTacToe, we have the first step toward having the potential for the computer to play against a human player. I haven't implemented that, but I did implement a means by which a game can be run automatically, given a fixed series of moves. That function looks like this:

void Game::run(const char *move)
{
    unsigned row, column;
    while (!ttt.isWinner(players[player]) && ttt.isNotFull() && *move)
    {
        player ^= 1;
        std::cout << ttt;
        row = *move++ - '1';
        column = std::toupper(*move++) - 'A';
        std::cout << "Applying " << players[player] << " to " 
            << row+1 << static_cast<char>('A'+column) << "\n";
        ttt.applyMove(players[player], row, column);
    }
    showResult();
}

Note that this part of the code lacks much in the way of error handling, but it's meant solely as illustration.

Putting it all together

Here's a sample main function that plays one 3x3 tie game automatically, and then allows for two humans to play a 4x4 game against each other:

#include <iostream>
#include <cctype>
#include <vector>

// TicTacToe and Game classes go here

int main()
{
    Game game1;
    game1.run("2B1A2A2C1C3A3B1B3C");
    Game game2(4);
    game2.run();
}

Answer 2

I have deleted enum struct Lines : int because it would be impossible to handle nth line numbers. Then I made the code into a generic form by templating Tictactoe.

#include <iostream>
#include <cctype>
#include <algorithm>
#include <functional>
#include <array>

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

std::ostream& operator<<(std::ostream& os, Player p)
{
    return os << static_cast<char>(p);
}

enum struct Type : int
{
    row         = 0,
    column      = 1,
    diagonal    = 2
};

template<int mesh>
class TicTacToe
{
public:
    TicTacToe(); 
    bool isFull() const;
    void draw() const;
    void turn(Player player);
    bool check(Player player) const;

private:
    bool applyMove(Player player, int position);

    std::array<Player, mesh * mesh> mGrid;
};

// utility functor to compute matching condition
template<int mesh>
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 / mesh) == mNumber);
        case Type::column:
            return (number % mesh == mNumber);
        case Type::diagonal:
            if (mNumber == 0)
                return ((std::abs(number / mesh) - number % mesh) == mNumber);
             if (mNumber == 1)
                 return ((std::abs(number / mesh) + number % mesh) == mNumber + mesh - 2);
        }

        return false; 
    }
    Type mCategory;
    int mNumber;
};

template<int mesh>
TicTacToe<mesh>::TicTacToe()
{ 
    mGrid.fill(Player::none);
}

template<int mesh>
bool TicTacToe<mesh>::applyMove(Player player, int position)
{
    if (mGrid[position] != Player::none)
        return false;
    mGrid[position] = player;
    return true;
}

template<int mesh>
bool TicTacToe<mesh>::isFull() const
{
    return 0 == std::count_if(mGrid.begin(), mGrid.end(),
        [](Player i)
    {
        return i == Player::none;
    });
}

template<int mesh>
bool TicTacToe<mesh>::check(Player player) const
{
    // check for row or column wins
    std::array<bool, 2 * (mesh + 1)> win;
    win.fill(true);

    int j = 0;

    // checking condition loop
    std::for_each(mGrid.begin(), mGrid.end(),
        [&](Player i)
    {
        int x = j++;

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

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

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

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

    return false;
}

template<int mesh>
void TicTacToe<mesh>::draw() const
{
    //Creating a onscreen grid
    std::cout << ' ';
    for (auto i = 1; i <= mesh; ++i)
        std::cout << "  " << i;

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

    for (auto i : mGrid)
    {
        if (Match<mesh>(Type::column, 0)(j++))
            std::cout << "\n " << A++;

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

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

template<int mesh>
void TicTacToe<mesh>::turn(Player player)
{
    char row = 0;
    char column = 0;
    std::size_t position = 0;
    bool applied = false;

    std::cout << "\n" << player << ": Please play. \n";

    while (!applied)
    {
        std::cout << "Row(1,2,3,...): ";
        std::cin >> row;
        std::cout << player << ": Column(A,B,C,...): ";
        std::cin >> column;

        position = mesh * (std::toupper(column) - 'A') + (row - '1');

        if (position < mGrid.size())
        {
            applied = applyMove(player, position);

            if (!applied)
                std::cout << "Already Used. Try Again. \n";
        }
        else
        {
            std::cout << "Invalid position.  Try again.\n";
        }
    }

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

class Game
{
public:
    Game() = default;
    void run();

private:
    static const std::size_t mMesh = 4;
    TicTacToe<mMesh> mTicTacToe;
    std::array<Player, 2> mPlayers{ { Player::first, Player::second } };
    int mPlayer = 1;

    void resultScreen(bool winner);
    std::function<void()>       display = std::bind(&TicTacToe<mMesh>::draw, &mTicTacToe);
    std::function<void(Player)> turn    = std::bind(&TicTacToe<mMesh>::turn, &mTicTacToe, std::placeholders::_1);
    std::function<bool(Player)> win     = std::bind(&TicTacToe<mMesh>::check, &mTicTacToe, std::placeholders::_1);
    std::function<bool()>       full    = std::bind(&TicTacToe<mMesh>::isFull, &mTicTacToe);
};

void Game::run()
{
    while (!win(mPlayers[mPlayer]) && !full())
    {
        mPlayer ^= 1;
        display();
        turn(mPlayers[mPlayer]);
    }

    resultScreen(win(mPlayers[mPlayer]));
}

void Game::resultScreen(bool winner)
{
    display();

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

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