5
\$\begingroup\$

This is based on my first Tic-Tac-Toe game:

Tic-Tac-Toe in C++11

I have come up with different game logic for my game. I would like to know which one is more reliable and effective and how I can improve the whole game.

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

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

private:
    bool fill(char player, int position);

    static const std::size_t mDim = 3;
    std::array<char, mDim * mDim> grid
    { 
        {   
            '-', '-', '-',
            '-', '-', '-',
            '-', '-', '-' 
        } 
    };
};

template<std::size_t dim>
struct column : public std::unary_function<int, bool>
{
    column(int i) 
        : colNum(i)
    {}

    bool operator() (int number) 
    { 
        return (number % dim == colNum); 
    }

    int colNum;
};

struct pairCondition
{
    int number;
    char name;

    pairCondition(int number, char name)
        : number(number), name(name)
    {}

    bool operator== (const pairCondition& s) const
    {
        return name == s.name;
    }
};

bool checkCondition(const pairCondition& i, const std::array<int, 3>& a, char player)
{

    if (i.number == a[0] || i.number == a[1] || i.number == a[2])
        return i.name == player;
    else
        return i.name != player;

}

bool TicTacToe::fill(char player, int position)
{
    if (grid[position] != '-')
        return false;
    grid[position] = player;
    return true;
}

bool TicTacToe::isFull() const
{
    return 0 == std::count_if(grid.begin(), grid.end(),
        [](const char& i)
    {
        return i == '-';
    });
}

bool TicTacToe::check(char player) const
{
    typedef std::function<bool(pairCondition)> func;

    // rows
    bool row1 = true, row2 = true, row3 = true;

    std::array<int, 3> rows1 = { 0, 1, 2 };
    std::array<int, 3> rows2 = { 3, 4, 5 };
    std::array<int, 3> rows3 = { 6, 7, 8 };

    func rowf1 = std::bind(&checkCondition, std::placeholders::_1, rows1, player);
    func rowf2 = std::bind(&checkCondition, std::placeholders::_1, rows2, player);
    func rowf3 = std::bind(&checkCondition, std::placeholders::_1, rows3, player);

    // columns
    bool col1 = true, col2 = true, col3 = true;

    std::array<int, 3> columns1 = { 0, 3, 6 };
    std::array<int, 3> columns2 = { 1, 4, 7 };
    std::array<int, 3> columns3 = { 2, 5, 8 };

    func colf1 = std::bind(&checkCondition, std::placeholders::_1, columns1, player);
    func colf2 = std::bind(&checkCondition, std::placeholders::_1, columns2, player);
    func colf3 = std::bind(&checkCondition, std::placeholders::_1, columns3, player);

    // diagonals
    bool diag1 = true, diag2 = true;

    std::array<int, 3> diagonal1 = { 0, 4, 8 };
    std::array<int, 3> diagonal2 = { 2, 4, 6 };

    func diagf1 = std::bind(&checkCondition, std::placeholders::_1, diagonal1, player);
    func diagf2 = std::bind(&checkCondition, std::placeholders::_1, diagonal2, player);

    std::vector<pairCondition> v =
    {
        { 0, grid[0] }, { 1, grid[1] }, { 2, grid[2] },
        { 3, grid[3] }, { 4, grid[4] }, { 5, grid[5] },
        { 6, grid[6] }, { 7, grid[7] }, { 8, grid[8] }
    };

    for (const auto& i : v)
    {
        // rows conditions
        row1 &= rowf1(i);
        row2 &= rowf2(i);
        row3 &= rowf3(i);

        // columns conditions
        col1 &= colf1(i);
        col2 &= colf2(i);
        col3 &= colf3(i);

        // diagonals conditions
        diag1 &= diagf1(i);
        diag2 &= diagf2(i);
    };

    return (row1 || row2 || row3 ||
            col1 || col2 || col3 ||
            diag1 || diag2);
}

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

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

    column<mDim>::argument_type input;

    for (auto& i : grid)
    {
        if (column<mDim>(0)(input = j++))
            std::cout << "\n " << A++;

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

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

void TicTacToe::turn(char 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 < grid.size()) 
        {
            applied = fill(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 ttt;
    std::array<char, 2> players{ { 'X', 'O' } };
    int player = 1;

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


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

    resultScreen();
}

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

    if (win)
    {
        std::cout << "\n" << players[player] << " is the Winner!\n";
    }
    else
    {
        std::cout << "\nTie game!\n";
    }
}

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

4 Answers 4

4
\$\begingroup\$

I have a few tidbits that could help. Nothing big, but things that you can improve as you code without having to overthink them:

  • Use constexpr to represent compile-time constants. static const works but constexpr pops into to head and screams "compile time!" when you read it:

    static constexpr std::size_t mDim = 3;
    
  • column::operator() should be const-qualified since it does not alter the state of column:

    bool operator() (int number) const
    { 
        return (number % dim == colNum); 
    }
    
  • unary_function and binary_function are deprecated since C++11 and will be removed from the standard in C++17. Therefore, you should avoid using them. The easy fix is to define result_type and argument_type by hand (which is rahter quick) or to simply use the types at the call point where you probably know them.

  • You could consider making the types column and pairCondition immutable by making their members const. Beforehand, really think about whether you want them to be immutable. It could make sense for column since functions are often immutable and this is a functor type.

  • I don't think that you want to represent any other cells thant "no player", "player 1" and "player 2". Therefore, you could probably use an enum struct to represent the accepted values in your grid instead of a char:

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

    Using an enum struct would ensure that you don't use any other magic value and would answer the question "what does this char represent?" when reading the code. If you really need to access the underlying type of the enum at different places, then use a simple enum instead of an enum struct.

\$\endgroup\$
7
  • \$\begingroup\$ thanks for your answer. i checked it as answer. but constexpr not recognized in VC++ 2013 which i'm using now. \$\endgroup\$
    – MORTAL
    Nov 25, 2014 at 13:10
  • \$\begingroup\$ @Morwenn: What do you think about const char players[3]{ 'X', 'O', '-' }; as an alternative to the enum struct? That way the code could continue to use player ^= 1; to change players. \$\endgroup\$
    – Edward
    Nov 25, 2014 at 13:27
  • \$\begingroup\$ @Edward .. even with 'enum struct player: char'. still the code can use 'player ^=1' \$\endgroup\$
    – MORTAL
    Nov 25, 2014 at 14:40
  • \$\begingroup\$ @Edward .. please check my code in gist: gist.github.com/anonymous/e04dff4659fa90f364f0 \$\endgroup\$
    – MORTAL
    Nov 25, 2014 at 14:55
  • \$\begingroup\$ @MORTAL That's probably an MSVC extension, using ^= with an enum struct should be a hard error. Anyway, you can still overload operator^= for player or simply use an enum instead of an enum struct even though it lessens type safety. \$\endgroup\$
    – Morwenn
    Nov 25, 2014 at 16:26
5
\$\begingroup\$

Your check() is still completly hardcoded to have a dimension of 3x3. I think it's even worse, as in more hardcoded, than the previous one.

Your class has a constant mDim. Define your rows, columns and diagonals using that constant. The number of rows and collumns is determined by mDim. Also the length of each row, column and diagonal is determined by 'mDim'.

The same goes for checkCondition(). The 3 is hardcoded but should be governed by mDim.

Try making mDim selectable in the constructor and play a standard 3x3 game of Tic-Tac-Toe and a larger 4x4 game of Tic-Tac-Toe-Tuz to see if you have have removed the hardcoded 3 everywhere.


If you want to go one step further you could remove the hardcoding of the board being a square and the board being 2 dimensional. Ever played 3D Tic-Tac-Toe?

\$\endgroup\$
1
  • \$\begingroup\$ you are absolutely right about this example is more worse than previous. i coded it for some one ask to make one for loop and separate arrays for win-condition. i knew this method will lead to more hard coding which why i came here to get help or hint how to do it. \$\endgroup\$
    – MORTAL
    Nov 23, 2014 at 13:43
3
\$\begingroup\$

I'm not sure your new checkCondition is an improvement. Compare the long and complex code you've got to something like this:

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; 
}

Which is easier to understand? Which is shorter?

\$\endgroup\$
1
  • \$\begingroup\$ yes. i'm totally satisfied and agree with your game-logic version. \$\endgroup\$
    – MORTAL
    Nov 25, 2014 at 12:58
1
\$\begingroup\$

Here's my attempt in minimizing hard-coding of this code, as well as improving my code base on what I have learned so far from @Jamal and @Edward in my previous post.

Proper naming for classes and its member functions and variables

I still have difficulty choosing the proper name to the classes and its members. It's a BIG problem for anyone for whom the English language is not his native language. So I have renamed some functions to imply the exact meaning of its name. For instance, fill() in TicTacToe class becomes applyMove. Also, all member variables using prefixes such as mGrid, mDash, mFree and mDim.

Also, I changed struct WinCondition to be struct Pair.

It's even better to use extended prefixes to member variables like s for static and c for constant.

Create and use a constructor for TicTacToe

It's great to initialize std::array in the constructor body rather than initialize it in the declaration part of the class. So, the TicTacToe constructor will be like this:

TicTacToe::TicTacToe() 
{ 
    mGrid.fill(mDash);
}

and it simply defines mGrid like this:

std::array<char, mDim * mDim> mGrid;

Simplifying and optimizing

I changed isFull() to be much simpler by making a new variable mFree:

bool TicTacToe::isFull() const
{
    return mFree == 0;
}

Also, I changed the conditional statement in draw().

column<mDim>::argument_type input;

for (auto& i : grid)
{
    int x = j++;
    if (column<mDim>(0)(input = x ))
        std::cout << "\n " << A++;

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

to become like this:

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

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

I removed struct column as it's no longer be needed in the code.

Minimizing hard-coding

To accomplish this, I need rearrange the check() function body. As it looks, it has huge blocks of variable declarations and definitions, so a struct is needed. I created a struct called WinCondition like this:

template<int dim>
struct WinCondition
{
    typedef std::function<bool(Pair, char)> func;
    typedef std::array<int, dim> Array;
    WinCondition()
    {
        for (auto i = 0; i < dim; ++i)
        {
            // set rows
            const int j = i;
            rows1[j] = i;
            rows2[j] = i + 1 * dim;
            rows3[j] = i + 2 * dim;

            // set columns
            columns1[j] = i * dim;
            columns2[j] = 1 + i * dim;
            columns3[j] = 2 + i * dim;

            // set diagonals
            diagonal1[j] = 4 * i ;
            diagonal2[j] = 2 * (i + 1);

        }

        colf1 = std::bind(&checkCondition<dim>, std::placeholders::_1, columns1, std::placeholders::_2);
        colf2 = std::bind(&checkCondition<dim>, std::placeholders::_1, columns2, std::placeholders::_2);
        colf3 = std::bind(&checkCondition<dim>, std::placeholders::_1, columns3, std::placeholders::_2);

        rowf1 = std::bind(&checkCondition<dim>, std::placeholders::_1, rows1, std::placeholders::_2);
        rowf2 = std::bind(&checkCondition<dim>, std::placeholders::_1, rows2, std::placeholders::_2);
        rowf3 = std::bind(&checkCondition<dim>, std::placeholders::_1, rows3, std::placeholders::_2);


        diagf1 = std::bind(&checkCondition<dim>, std::placeholders::_1, diagonal1, std::placeholders::_2);
        diagf2 = std::bind(&checkCondition<dim>, std::placeholders::_1, diagonal2, std::placeholders::_2);
    }

    Array rows1, rows2, rows3, 
          columns1, columns2, columns3, 
          diagonal1, diagonal2;


    func colf1, colf2, colf3,
         rowf1, rowf2, rowf3,
         diagf1, diagf2;
};

Now, in check() function would be like this

bool TicTacToe::check(char player) const
{
    std::array<bool, 8> winner;
    winner.fill(true);

    std::vector<Pair> v;

    for (auto i = 0; i < 8; ++i)
        v.emplace_back(Pair(i, mGrid[i]));

    for (auto i : v)
    {
        // rows conditions
        winner[0] &= win.rowf1(i, player);
        winner[1] &= win.rowf2(i, player);
        winner[2] &= win.rowf3(i, player);

        // columns conditions
        winner[3] &= win.colf1(i, player);
        winner[4] &= win.colf2(i, player);
        winner[5] &= win.colf3(i, player);

        // diagonals conditions
        winner[6] &= win.diagf1(i, player);
        winner[7] &= win.diagf2(i, player);
    }

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

    return false;
}

Now, it's much easier to minimize hard-coding any further.

Thanks to @Morwenn, I'm now a bit familiar with scoped enumerations, a new kind of enumeration in C++ (documented here).

Thanks to @Goswin, Now check() is ok.

Complete code:

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

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

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

struct Pair
{
    int mNumber;
    Player mName; 
    Pair(int number, Player name)
        : mNumber(number), mName(name)
    {}

    bool operator== (const Pair& s) const
    {
        return mName == s.mName;
    }
};

template<int dim>
bool checkCondition(const Pair& i, const std::array<int, dim>& a, Player player)
{

    if (i.mNumber == a[0] || i.mNumber == a[1] || i.mNumber == a[2])
        return i.mName == player;
    else
        return i.mName != player;

}

template<int dim>
struct WinCondition
{
    typedef std::function<bool(Pair, Player)> func;
    typedef std::array<int, dim> Array;
    WinCondition()
    {
        for (auto i = 0; i < dim; ++i)
        {
            // set rows
            const int j = i;
            rows1[j] = i;
            rows2[j] = i + 1 * dim;
            rows3[j] = i + 2 * dim;

            // set columns
            columns1[j] = i * dim;
            columns2[j] = 1 + i * dim;
            columns3[j] = 2 + i * dim;

            // set diagonals
            diagonal1[j] = 4 * i ;
            diagonal2[j] = 2 * (i + 1);
        }

        colf1 = std::bind(&checkCondition<dim>, std::placeholders::_1, columns1, std::placeholders::_2);
        colf2 = std::bind(&checkCondition<dim>, std::placeholders::_1, columns2, std::placeholders::_2);
        colf3 = std::bind(&checkCondition<dim>, std::placeholders::_1, columns3, std::placeholders::_2);

        rowf1 = std::bind(&checkCondition<dim>, std::placeholders::_1, rows1, std::placeholders::_2);
        rowf2 = std::bind(&checkCondition<dim>, std::placeholders::_1, rows2, std::placeholders::_2);
        rowf3 = std::bind(&checkCondition<dim>, std::placeholders::_1, rows3, std::placeholders::_2);

        diagf1 = std::bind(&checkCondition<dim>, std::placeholders::_1, diagonal1, std::placeholders::_2);
        diagf2 = std::bind(&checkCondition<dim>, std::placeholders::_1, diagonal2, std::placeholders::_2);
    }

    Array rows1, rows2, rows3, 
          columns1, columns2, columns3, 
          diagonal1, diagonal2;

    func colf1, colf2, colf3,
         rowf1, rowf2, rowf3,
         diagf1, diagf2;
};

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 unsigned mDim = 3;
    //const char mDash = '-';
    unsigned mFree = mDim * mDim;
    std::array<Player, mDim * mDim> mGrid;
    WinCondition<mDim> win;
};


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

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

bool TicTacToe::isFull() const
{
    return mFree == 0;
}

bool TicTacToe::check(Player player) const
{
    std::array<bool, 8> winner;
    winner.fill(true);

    std::vector<Pair> v;

    for (auto i = 0; i < 8; ++i)
        v.emplace_back(Pair(i, mGrid[i]));

    int j = 0;
    for (auto i : v)
    {
        // rows conditions
        winner[j++] &= win.rowf1(i, player);
        winner[j++] &= win.rowf2(i, player);
        winner[j++] &= win.rowf3(i, player);

        // columns conditions
        winner[j++] &= win.colf1(i, player);
        winner[j++] &= win.colf2(i, player);
        winner[j++] &= win.colf3(i, player);

        // diagonals conditions
        winner[j++] &= win.diagf1(i, player);
        winner[j++] &= win.diagf2(i, player);

        // correction
        j = 0;
    }

    for (auto i : winner)
    {
        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 (j++ % mDim == 0)
            std::cout << "\n " << A++;

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

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

void TicTacToe::turn(Player player)
{
    char row = 0;
    char column = 0;
    unsigned 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 ttt;
    std::array<Player, 2> players{ { Player::first, Player::second } };
    int player = 1;

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

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

    resultScreen(win(players[player]));

}

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

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

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

}
\$\endgroup\$
2
  • \$\begingroup\$ You have still hardcoded 8 victory conditions. That should be width + height + 2 many. \$\endgroup\$ Nov 25, 2014 at 20:23
  • \$\begingroup\$ @GoswinvonBrederlow ... thanks, i have corrected now \$\endgroup\$
    – MORTAL
    Nov 25, 2014 at 23:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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