Noughts & Crosses (tic-tac-toe) revisited (using C++20 modules)

Question

This is a development of Nought and Crosses (tic-tac-toe) in C++ a learning project in C++. This time using C++20 modules. As before, any suggestions on improvements in style, clarity (or anything else) gratefully received.

A few specifics:

1. The diagram - is it helpful? What do people use? Is there a better way?
2. The partitioning - originally vacant_cells was part of board, but then it seemed reasoanbly independent, so separated it out. Good idea?
3. Naming seems important - e.g. methods that return true/false - any suggestions as to a good pattern for naming them?
4. At the moment getMachineMove() exists to be able to plug strategies in (at the moment a highly simplistic one!)
5. Compiled successfully with MSVC (kept getting errors in the compiler itself with g++).

cl /c /std:c++latest /EHsc /MTd /Wall board.ixx terminal.ixx vacant_cells.ixx
cl /std:c++latest /EHsc /MTd /Wall /reference board=board.ifc /reference terminal=terminal.ifc /reference vacant_cells=vacant_cells.ifc main.cpp

main.cpp

#include <string>
#include <iostream>

import board;
import terminal;
import vacant_cells;

static constexpr std::size_t dimension = 3;
static Vacant_cells vacant_cells(dimension*dimension);

std::size_t getMachineMove(){
        return vacant_cells.getRandomMove();
    }

int main() {  
    static Board gameBoard (dimension);
    static Terminal gameTerminal;
    enum Player : char { HUMAN = 'X', MACHINE = 'O' };
    static char currentPlayer = HUMAN;
    std::size_t position;
    
    while (true) {
        if (currentPlayer == HUMAN) {
            gameTerminal.displayBoard(gameBoard.GetStatusOfCells());
            while (true) {
                position = gameTerminal.getPlayerChoice();
                if (vacant_cells.isCellVacant(position)) {
                    gameBoard.placeSymbol(position,HUMAN);
                    vacant_cells.removeFromVacancies(position);
                    break;    
                }
                gameTerminal.displayMessage("Already taken, try again");
            }
             
        }
        else { 
            position = getMachineMove();
            gameBoard.placeSymbol(position,MACHINE);
            vacant_cells.removeFromVacancies(position);
        }   

        if (gameBoard.win()) {
            gameTerminal.displayBoard(gameBoard.GetStatusOfCells());
            gameTerminal.displayMessage(std::string(1, currentPlayer)+" wins!");
            break;
        }

        if (!vacant_cells.vacancies()) {
            gameTerminal.displayBoard(gameBoard.GetStatusOfCells());
            gameTerminal.displayMessage("It's a tie!");
            break;
        }

        currentPlayer = (currentPlayer == HUMAN) ? MACHINE : HUMAN;
    } // end while

    return 0;
}

board.ixx

module;

#include <vector>

export module board;

export class Board {

private:

    std::vector<std::vector<char>> board;
    std::size_t boardDimension;

public:

    Board(std::size_t dimension) {
        boardDimension=dimension;
        board.resize(dimension, std::vector<char>(dimension, ' '));
      
        char value = '1'; // Start value for the cells
        for (std::size_t row = 0; row < dimension; ++row) {
            for (std::size_t col = 0; col < dimension; ++col) {
                board[row][col] = value++;
            }
        }
    }

    auto GetStatusOfCells() const{
        return board;
    }

    void placeSymbol(std::size_t position, char symbol) {
        std::pair<std::size_t, std::size_t> coordinates;
        coordinates.first = (position-1) / boardDimension;  // x coordinate
        coordinates.second = (position-1) % boardDimension;
        board[coordinates.first][coordinates.second] = symbol;
    }

    bool win() const {
        for (std::size_t n=0; n < boardDimension; n++) {
            //col n
            if (board[n][0] == board[n][1] && board[n][1] == board[n][2]) {
                return true;
            }
            // row n
            if (board[0][n] == board[1][n] && board[1][n] == board[2][n]) {
                return true;
            }
        }    // diagonals
        if (board[0][0] == board[1][1] && board[1][1] == board[2][2]) {
            return true;
        }
        if (board[0][2] == board[1][1] && board[1][1] == board[2][0]) {
            return true;
        }
        return false;
    }

};

vacant_cells.ixx

module;

#include <vector>
#include <random>

export module vacant_cells;

export class Vacant_cells {

    private:
        std::vector<std::size_t> positions;

    public:
        Vacant_cells (std::size_t n) {
            for (std::size_t i = 1; i <= n; ++i) {
                positions.push_back(i);
            }
            std::random_device rd;
            std::mt19937 gen(rd());
            std::shuffle(positions.begin(), positions.end(), gen);
        }

        std::size_t getRandomMove() const {
            return positions.back();
        }   

        bool isCellVacant(std::size_t move) const {
            bool found = false;
            for (const auto& posn : positions) {
                if (posn == move) {
                    found = true;
                    break;
                }
            }
            if (found) {
                return true;
            } else {
                return false;
            }
        }
        
        void removeFromVacancies (std::size_t move) {
            std::erase(positions, move);
        }

        bool vacancies () const {
            return !positions.empty();
        }

};

terminal.ixx

module;

#include <cstddef> 
#include <iostream> 
#include <string> 

export module terminal;

export class Terminal {

private:    

public:

std::size_t getPlayerChoice() {
    int digit {-1};
    std::string input;

    while (true) {
        std::cout << "Enter your move (1 - 9): " << std::flush;
        std::getline(std::cin, input);
        if (std::cin.eof()) {
            exit(1); 
        }
        input.erase(0, input.find_first_not_of(" \t"));
        input.erase(input.find_last_not_of(" \t") + 1);
        if (input.length() == 1 && std::isdigit(input[0])) {
            digit = input[0] - '0'; 
            if (digit >= 1 && digit <= 9) {
                break;
            }
        }
        std::cout << "Oops... must be a single digit between 1 and 9\n";
    }
    return static_cast<std::size_t>(digit);
}

void displayMessage(const std::string& message){
    std::cout << message << '\n';  
}

void displayBoard(auto status){
    std::size_t dim = status.size();
    std::cout << "\n";
    for (uint8_t row = 0; row < dim; ++row) {
        for (uint8_t col = 0; col < dim; ++col) {
            std::cout << status[row][col];
            if (col < dim-1) {
                std::cout << " | ";
            }
        }
        std::cout << "\n";
        if (row < dim-1) {
            std::cout << "---------\n";
        }
    }
    std::cout << "\n";
    std::cout << std::flush;
}

};

Answer 1

Answers to your questions

• The diagram - is it helpful? What do people use? Is there a better way?

I think the drawings of the board, the valid inputs and the mock-up of the output are fine, and help define the goal you are working towards.

However, the diagram showing the relationships between classes and functions is, in my opinion, not very helpful. The problem with that sort of diagram is that it very quickly gets very complicated, to the point that it's easier to just read the source code. Also, keeping that diagram in sync with the actual code is a lot of work, and if you don't keep it in sync it will become useless.

• The partitioning - originally vacant_cells was part of board, but then it seemed reasoanbly independent, so separated it out. Good idea?

First of all, the name is badly chosen; it's not just keeping track of vacant cells, it also is used to produce a random vacant cell.

Second, consider that isCellVacant() has to loop over the vector positions in your code, but if it was part of Board, it could just directly check the cell at the given position. For small boards this doesn't matter, but with your code, the larger the board is, the slower isCellVacant() becomes.

Furthermore, now you have to update both gameBoard and vacant_cells whenever you make a move. Having just one class keep track of the board and its vacant cells avoids potential mistakes.

• Naming seems important - e.g. methods that return true/false - any suggestions as to a good pattern for naming them?

Yes, make sure it's obvious from the name of the method that it returns a boolean. For example, isCellVacant() is fine. However, win() is not. For the latter, hasWinner() might be better. Similarly, vacancies() sounds like it returns a list of vacancies, but it only checks if there is any vacancy left. Maybe hasVacancies() is better, or you could invert it and name it isFull().

Naming

There are more issues related to naming. First of all, be consistent: I see you use both snake_case (vacant_cells) and camelCase (gameBoard) for variables, and some member functions start with a lower case (placeSymbol()), others start with a capital (GetStatusOfCells()). It doesn't really matter that much which style you choose for a given category, just that you apply it consistently.

Avoid hardcoding numbers

On the one hand you have made the size of the board variable (boardDimension), but on the other hand you have hardcoded knowledge of the size in many places. And in some places you even mix that up, for example in win() you use boardDimension in the for-loop, but at the same time you assume each row, column and diagonal has size 3.

Whenever you write a number that's not 0 or 1, check if you don't already have some constant or variable you can use instead. If not, create one.

Store the board as a one-dimensional vector

Nested std::vectors are not efficient. It's often better to use a one-dimensional vector instead. Especially in your code, where you already pass around position as a single std::size_t; if you have a one-dimensional vector you can just use that directly as an index into the vector. Only convert to/from row and column positions as necessary.

C++23 introduces std::mdspan, which allows you to get a multi-dimensional view of a one-dimensional array or vector.

Error checking

While reading input, you only check for std::cin.eof(). However, there might be other errors that do not result in an EOF condition. You ignore those, which will cause your program to go into an infinite loop trying to read a line, and complaining it doesn't contain a valid number.

The right thing to do is to check the result of getline() itself:

if (!std::getline(std::cin, input)) {
    // handle error here
}

Avoid global variables

You made gameBoard a variable inside main(), but why not do the same for vacant_cells? There is not reason to make it a global variable.