Tic-Tac-Toe in C++11

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
#include
#include
#include
#include

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 grid
{
{
'-', '-', '-',
'-', '-', '-',
'-', '-', '-'
}
};
};

template
struct column : public std::unary_function
{
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::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(),
&
{
int x = j++;

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

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

if (column(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

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 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::argument_type input;

for (auto& i : grid)
{
    int x = j++;
    if (column(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 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.

StackExchange Code Review Q#70593, answer score: 5