8
\$\begingroup\$

I have found Tic-Tac-Toe source code: Tic Tac Toe in C++

I have rewritten the source code in C++11 as shown below. How can I minimize hardcoding in the game logic?

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

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

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

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

bool TicTacToe::check(char player) const
{
    column<mDim>::argument_type input;

    // check for row or column wins
    bool row1 = true, row2 = true, row3 = true,
        col1 = true, col2 = true, col3 = true,
        diag1 = true, diag2 = true;
    int j = 0;

    // columns
    std::for_each(grid.begin(), grid.end(), 
        [&](char i)
    {
        int x = j++;

        if (column<mDim>(0)(input = x)) 
            col1 &= i == player;

        if (column<mDim>(1)(input = x)) 
            col2 &= i == player;

        if (column<mDim>(2)( input = x)) 
            col3 &= i == player;
    });

    // diagonals
    j = 0;
    for (const auto& i : grid)
    {
        int x = j++;
        if (x == 0 || x == 4 || x == 8)
            diag1 &= i == player;
        if (x == 2 || x == 4 || x == 6)
            diag2 &= i == player;
    }

    if (col1 || col2 || col3 || diag1 || diag2)
        return true;

    // rows
    return std::search_n(grid.begin(), grid.end(), 3, player) != grid.end();
}

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)
    {
        int x = j++;
        if (column<mDim>(0)(input = x ))
            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();
    void run();

private:
    TicTacToe ttt;
    std::array<char, 2> players{ { 'X', 'O' } };
    int player = 0;

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

Game::Game()
    :ttt()
{

}

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

    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\$
5
\$\begingroup\$

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

Eliminate unused variables

In the check routine, the variables row1, row2 and row3 are initialized but unused. It would be best to eliminate them.

Fix check code

The code does not correctly evaluate when a player has won. For example, if I redirect this file to the game:

2
B
1
A
2
A
2
C
1
C
3
A
3
B

The result is this:

   1  2  3
 A X  O  X 
 B -  O  O 
 C O  X  - 


O is the Winner!

Clearly that's not right.

I'd write it like this:

bool TicTacToe::check(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; 
}

Fix Game::run

The code currently includes the following code in Game::run():

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

However, that's an error because win is std::function<bool(char)> and this is not an invocation of that function. In fact, what it's doing is checking to see if the address of the function is nullptr. It never is, so this code will always claim there's a winner, even if the game was actually a tie. Either invoke the function again as win(players[player]) or, better, save the result from the previous invocation a few lines above and test that.

Avoid unnecessary obfuscation

The code for Game::run uses std::bind to essentially redefine four functions of the TicTacToe class. It would be much simpler to simply call those functions directly. For example, instead of writing !full() you could simply call !ttt.isFull().

Eliminate column

The column function makes the code more complex rather than less and is created and destroyed many many times during the course of a regular game. (On a sample run with a tie game here, 90 column objects were created and destroyed.)

For example, the code for TicTacToe::draw() contains this code:

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

It can be rewritten in much simpler form without column:

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

Consider function names carefully

The function named isFull() is well-named, but others are not. For instance, the fill function would probably make more sense named apply or applyMove. The word fill implies that the entire array is filled, which is not actually the purpose for this function.

Use a constructor for TicTacToe

Most of the code carefuly uses mDim as a potentially changeable parameter denoting the dimension of the board. However, the grid member is statically initialized with a hand-created array of - characters. Better would be to simply define grid like this:

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

and then create a constructor:

TicTacToe() { grid.fill('-'); }

Avoid "magic numbers"

A few places within the code use '-' to signify an empty square. However, this should instead be a static const member of the TicTacToe class instead.

Simplify isFull()

Rather than check each square in the grid each turn, it would probably be simpler just to maintain a number of empty squares.

Consider revising the class responsibilities

Right now, the Game class keeps track of the turns, does some of the I/O and contains a TicTacToe object. Better might be to make it responsible for all of the I/O and have TicTacToe incorporate only the logic of the game. This is a step toward what is called the Model-View-Controller pattern. The use is that if, at some future point, you wished to convert this game into, say, a graphical program with a touch-screen interface, you would only have to rework Game and not the TicTacToe class.

Thoroughly validate user input

Right now, the code will allow the user to input a position of (0,B) which it interprets as (3,A) and (0,D) as (3,C). Even stranger input, such as (/,B) is also accepted. It would be better to make sure that what the user inputs is actually valid. Most of the logic is already there -- it just needs some improvement.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ unfortunately, you have logic error in check funtion. try this:1a1b2a2b3b3a \$\endgroup\$ – MORTAL Nov 24 '14 at 21:53
  • \$\begingroup\$ @MORTAL: that sequence works just fine here. What do you see when you try it? \$\endgroup\$ – Edward Nov 24 '14 at 21:59
  • \$\begingroup\$ it gave me: O is the Winner!. here my code in gists:gist.github.com/anonymous/8090aad86a7a4861f168 \$\endgroup\$ – MORTAL Nov 24 '14 at 22:02
3
\$\begingroup\$

Instead of defining the winning conditions by using a bunch of for-loops in your code, you can define the winning conditions by using an actual array for them.

Let's say that each win condition is an integer array. The array [ 0, 4, 8 ] is then the win-condition representing the top-left to bottom-right diagonal.

So what you can use is an array of these win conditions.

Then, when checking for a winner, loop through the win conditions and check if there is one win condition that rules them all (i.e., if a player has played on all the positions in the win condition).

To setup the win conditions, I would recommend using a method similar to my loopAdd method. Also take a look at my TTWinCondition class in my Tic-Tac-Toe Ultimate implementation

|improve this answer|||||
\$\endgroup\$
3
\$\begingroup\$

I just have some minor points to address, though I haven't looked at the entire code.

  • You can just combine the private sections in TicTacToe. It doesn't really matter to separate member variables and functions. You can still add whitespace between them.

  • You don't need a blank constructor in C++11:

    Game::Game()
        :ttt()
    {
    
    }
    

    If you need to provide one, then you can use a default constructor instead:

    Game() = default;
    

    This would just go in the class declaration.

  • Consider adding an additional function for determining the winner so that run() doesn't have to handle this other task.

|improve this answer|||||
\$\endgroup\$
1
\$\begingroup\$

There is an error in the code:

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

The return value was missing a parameter. The correct one is:

 bool TicTacToe::isFull() const
{
    return 0 == std::count_if(grid.begin(), grid.end(),
        [](const char& i)
    {
        return i == '-'; 
    });
}
|improve this answer|||||
\$\endgroup\$
1
\$\begingroup\$

Before explain recent change, allow me to explain what was bothering me with my previous code.

Problems in my original source code was the game logic where I have hard-coding game-logic in the function check() . As it seems there are 3 for-loop to compute the logic condition checking for both the player as input, and grid array and punch of bool variables.

The first for-loop is to check any matching between player and columns and assign it col1, col2, col3 via bitwise AND operator:

// columns
std::for_each(grid.begin(), grid.end(), 
    [&](char i)
{
    int x = j++;

    if (column<mDim>(0)(input = x)) 
        col1 &= i == player;

    if (column<mDim>(1)(input = x)) 
        col2 &= i == player;

    if (column<mDim>(2)( input = x)) 
        col3 &= i == player;
});

In my opinion, there is no problem with it. I'm even comfortable with it. However, for the next for-loop, the most ugly part in the check() function. It's checking in diagonal axes by hard-coding the possible position in a grid array:

    for (const auto& i : grid)
    {
        int x = j++;
        if (x == 0 || x == 4 || x == 8)
            diag1 &= i == player;
        if (x == 2 || x == 4 || x == 6)
            diag2 &= i == player;
    }
//ugly right!

The last for-loop is for checking by rows.

Why this approach is better

First of all, I removed both for-loops to be just one for-loop that takes care of checking any matches:

    std::for_each(grid.begin(), grid.end(),
        [&](char i)
    {
        int x = j++;

        // columns
        if (column<mDim>(0)(input = x)) // first column
            win[0] &= i == player;
        if (column<mDim>(1)(input = x))
            win[1] &= i == player;
        if (column<mDim>(2)(input = x))
            win[2] &= i == player;

        // rows
        if (row<mDim>(0)(input = x)) // first row
            win[3] &= i == player;
        if (row<mDim>(1)(input = x))
            win[4] &= i == player;
        if (row<mDim>(2)(input = x))
            win[5] &= i == player;

        // diagonals
        if (diagonal<mDim>(0)(input = x))
            win[6] &= i == player;
        if (diagonal<mDim>(2)(input = x))
            win[7] &= i == player;
    });

Also, I added 2 functors to take condition arguments and compute for matching:

// utility functor to compute matching condition by row
template<int dim>
struct row : public std::unary_function<int, bool>
{
    row(int i) : rowNum(i){}
    bool operator() (int number)
    {
        return (std::abs(number / dim)  == rowNum);
    }
    int rowNum;
};

template<int dim>
struct diagonal : public std::unary_function<int, bool>
{
    diagonal(int i) : diag(i){}
    bool operator() (int number)
    {
        switch (diag)
        {
        case 0:
            return ((std::abs(number / dim) - number % dim) == diag);
        case 2:
            return ((std::abs(number / dim) + number % dim) == diag);
        }

        return false; // for the warning in VC++ 2013
    }
    int diag;
};

I removed the old variables row1, row2, row3, col1, col2, col3, diag1 and diag2 and replaced them with an array that takes bool values.

std::array<bool, 8> win = 
{
    true, true, true,true,
    true, true, true, true
};

Now, it is much easier to minimize the hard-coding by control all constants with mDim

I believe this is a much better approach, in my opinion, to minimize hard-coding in game logic.

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

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

// utility functor to compute matching condition by column
template<int dim>
struct column : public std::unary_function<int, bool>
{
    column(int i) : colNum(i){}
    bool operator() (int number) const
    { 
        return (number % dim == colNum); 
    }
    int colNum;
};

// utility functor to compute matching condition by row
template<int dim>
struct row : public std::unary_function<int, bool>
{
    row(int i) : rowNum(i){}
    bool operator() (int number) const
    {
        return (std::abs(number / dim)  == rowNum);
    }
    int rowNum;
};

// utility functor to compute matching condition by diagonal
template<int dim>
struct diagonal : public std::unary_function<int, bool>
{
    diagonal(int i) : diag(i){}
    bool operator() (int number) const
    {
        switch (diag)
        {
        case 0:
            return ((std::abs(number / dim) - number % dim) == diag);
        case 2:
            return ((std::abs(number / dim) + number % dim) == diag);
        }

        return false; // for the warning in VC++ 2013
    }
    int diag;
};

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

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

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

bool TicTacToe::check(Player player) const
{
    column<mDim>::argument_type input; // one for all and all for one

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

    int dim = mDim;
    int j = 0;

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

        // columns
        if (column<mDim>(0)(input = x))
            win[0] &= i == player;;
        if (column<mDim>(1)(input = x))
            win[1] &= i == player;
        if (column<mDim>(2)(input = x)) // first column
            win[2] &= i == player;

        // rows
        if (row<mDim>(0)(input = x)) // first row
            win[3] &= i == player;
        if (row<mDim>(1)(input = x))
            win[4] &= i == player;
        if (row<mDim>(2)(input = x))
            win[5] &= i == player;

        // diagonals
        if (diagonal<mDim>(0)(input = x))
            win[6] &= i == player;
        if (diagonal<mDim>(2)(input = 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';

    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(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 < grid.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();
}
|improve this answer|||||
\$\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.