Tic-Tac-Toe Console Game

Question

After providing an answer on this question, I decided to try writing my own version of a console based tic-tac-toe game that will later be converted into an ultimate tic-tac-toe game. My goals for this code are to keep it simple, readable, and well-organized. Hopefully it's not too bad. I'm not completely familiar with all of the new C++11/14 lingo, so any tips to modernize the code would be great.

tic_tac_toe.h

#ifndef MY_TIC_TAC_TOE_H
#define MY_TIC_TAC_TOE_H
#include <array>
#include <iosfwd>

namespace tic_tac_toe
{
    class Grid
    {
    public:
        static const int COLUMNS = 3;
        static const int ROWS = 3;

        bool hasWinner() const;
        bool isTie() const;
        bool setTile(bool isX, int tileNumber);
        std::ostream& printRow(std::ostream& os, int index) const;

    private:
        int filled = 0;

        std::array<std::array<char, COLUMNS>, ROWS> cells = {{
            {{'1', '2', '3'}},
            {{'4', '5', '6'}},
            {{'7', '8', '9'}},
        }};

        bool checkRows() const;
        bool checkColumns() const;
        bool checkDiagonals() const;

        bool checkRow(int index) const;
        bool checkColumn(int index) const;

        bool isFreeCell(int row, int column) const;
    };

    std::ostream& operator<< (std::ostream& os, const Grid& grid);

    class Game
    {
    public:
        void play(bool xStarts = true);
        bool step(bool isX);

    private:
        Grid grid;

        int getTileNumber(char player) const;
        bool hasWinner() const;
        bool isTie() const;
    };
}

#endif

tic_tac_toe.cpp

#include "tic_tac_toe.h"
#include <limits>
#include <iostream>

// We're in a cpp file and I'm not worried about any
// name collisions.
using namespace tic_tac_toe;

namespace
{
    bool same(char a, char b, char c)
    {
        return a == b && b == c;
    }

    bool between(int value, int low, int high)
    {
        return (value >= low) && (value <= high);
    }
}

bool Grid::hasWinner() const
{
    return checkRows()
        || checkColumns()
        || checkDiagonals();
}

bool Grid::isTie() const
{
    return (!hasWinner()) && (filled >= 9);
}

bool Grid::setTile(bool isX, int tileNumber)
{
    if (!between(tileNumber, 1, 9)) {
        return false;
    }

    --tileNumber;
    int row = tileNumber / ROWS;
    int column = tileNumber % COLUMNS;

    if (isFreeCell(row, column)) {
        cells[row][column] = isX ? 'X' : 'O';
        ++filled;
        return true;
    }

    return false;
}

std::ostream& Grid::printRow(std::ostream& os, int index) const
{
    for (auto&& cell : cells[index]) {
        os << '[' << cell << ']';
    }

    return os;
}

bool Grid::checkRows() const
{
    return checkRow(0)
        || checkRow(1)
        || checkRow(2);
}

bool Grid::checkColumns() const
{
    return checkColumn(0)
        || checkColumn(1)
        || checkColumn(2);
}

bool Grid::checkDiagonals() const
{
    bool d1 = same(cells[0][0],
                   cells[1][1],
                   cells[2][2]);

    bool d2 = same(cells[0][2],
                   cells[1][1],
                   cells[2][0]);

    return d1 || d2;
}

bool Grid::checkRow(int index) const
{
    return same(cells[index][0],
                cells[index][1],
                cells[index][2]);
}

bool Grid::checkColumn(int index) const
{
    return same(cells[0][index],
                cells[1][index],
                cells[2][index]);
}

bool Grid::isFreeCell(int row, int column) const
{
    char value = cells[row][column];
    return (value != 'X') && (value != 'O');
}

std::ostream& tic_tac_toe::operator<< (std::ostream& os, const Grid& grid)
{
    grid.printRow(os, 0) << '\n';
    grid.printRow(os, 1) << '\n';
    grid.printRow(os, 2) << '\n';

    return os;
}

void Game::play(bool xStarts)
{
    bool isX = xStarts;
    while(step(isX)) {
        isX = !isX;
    }
}

bool Game::step(bool isX)
{
    std::cout << grid << '\n';
    const char player = isX ? 'X' : 'O';

    while (true) {
        int tileNumber = getTileNumber(player);
        if (grid.setTile(isX, tileNumber)) {
            break;
        }

        std::cout << "Error, tile already covered. "
                     "Please choose another tile." "\n";
    }

    if (hasWinner()) {
        std::cout << grid << '\n';
        std::cout << player << " wins!" "\n";
        return false;
    }

    else if (isTie()) {
        std::cout << grid << '\n';
        std::cout << "Draw game!" "\n";
        return false;
    }

    return true;
}

int Game::getTileNumber(char player) const
{
    while (true) {
        std::cout << "Please select a tile (1-9):\n";
        std::cout << player << " > ";
        int tileNumber = 0;
        std::cin >> tileNumber;
        if (between(tileNumber, 1, 9)) {
            return tileNumber;
        }

        std::cin.clear();
        std::cin.ignore((std::numeric_limits<std::streamsize>::max)(), '\n');
        std::cout << "Error, invalid value!" "\n";
    }
}

bool Game::hasWinner() const
{
    return grid.hasWinner();
}

bool Game::isTie() const
{
    return grid.isTie();
}

driver.cpp

#include "tic_tac_toe.h"

int main()
{
    auto game = tic_tac_toe::Game();
    game.play();
}

sample output

[1][2][3]
[4][5][6]
[7][8][9]

Please select a tile (1-9):
X > 5
[1][2][3]
[4][X][6]
[7][8][9]

Please select a tile (1-9):
O > 9
[1][2][3]
[4][X][6]
[7][8][O]

Please select a tile (1-9):
X > 8
[1][2][3]
[4][X][6]
[7][X][O]

Please select a tile (1-9):
O > 2
[1][O][3]
[4][X][6]
[7][X][O]

Please select a tile (1-9):
X > 7
[1][O][3]
[4][X][6]
[X][X][O]

Please select a tile (1-9):
O > 3
[1][O][O]
[4][X][6]
[X][X][O]

Please select a tile (1-9):
X > 1
[X][O][O]
[4][X][6]
[X][X][O]

Please select a tile (1-9):
O > 6
[X][O][O]
[4][X][O]
[X][X][O]

O wins!

Answer 1

I see a number of things which may help you improve your program. First, though, I think it's important to mention that this code is generally well-constructed and easy to understand, so you're well on your way already. In particular, the proper use of namespaces and const and the separation of I/O in Game versus game state in Grid is good.

Prefer constexpr to const

The COLUMNS and ROWS can both be constexpr and there is sometimes an optimization value in doing so.

Don't use all uppercase for constants

Using all caps for macros is a good and very common practice. Using them for other purposes is not because it violates that understanding and misleads the reader.

Consider simplifying the data structure

Much of the code would likely be simpler if the same structure as presented to the user (a sequence of cells) were also used internally. Lookup would be simplfied and no translation (other than adjusting for zero-based array) would be required between external and internal views.

Use more named constants

The values 'X' and 'O' and the value 9 would all be good candidates for named constants which, if well-named, would enhance readability and clarity.

Rename ambiguous functions

I find step, and the various check... functions to be less than helpful names. First, it's not clear whether step is being used as a noun or a verb here, and one doesn't usually speak of "steps" but rather "turns" when playing a game. Although "turn" still has the noun/verb duality problem, at least the term is more obviously germane to the problem domain. As for the check functions, what are they checking? What does a return value of true mean? I'd suggest maybe isDiagonalWin and isRowWin.

Reduce work

Only the last value played could possibly be part of a winning game, so one could reduce work by only checking combinations relevant to that move, or at least by only checking for the values of the last player. Also, it's not really necessary to check isWinner first and then again within isTie.

Be careful with move semantics

This line in printRow is a little strange to me:

for (auto&& cell : cells[index]) {

I don't really think we need to move the value into cell, do we? I'd suggest just using auto & or const auto & instead. In this case, since a cell is just a char it's probably actually smaller code to just use auto.

Allow the user to specify an iostream

Rather than tying the game solely to std::cout and std::cin, it would be a very simple change to allow, as perhaps a pair of default arguments to a constructor for Game, which streams are used. This makes some things, such as automatic testing, slightly easier.

Answer 2

Thanks for sharing this. Looks fine for me. If you have free time think about possibility to make this game more flexible, maybe not only 3x3 but 5x5, 7x7? That would a be great exercise.

I thought about it when I saw this hard-coded values. You should avoid any kind of hard-coding values.

if (!between(tileNumber, 1, 9)) {
       return false;
}

Next, you probably check the same thing 2x. In you getTileNumber function you check number by using function between (I would think about better name for this), and in setTile you are checking this one more time.

    int tileNumber = getTileNumber(player);
    if (grid.setTile(isX, tileNumber)) {
        break;
    }

You have multiple occurrences of std::cout etc. You could think of some special class that would be responsible for such task, GameLogger?

I personally use and, or instead of &&, || but it is a matter of taste.