5
\$\begingroup\$

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.

diagram of structure of oxo

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

};
\$\endgroup\$
4
  • 1
    \$\begingroup\$ This is not enough to write an actual answer: Why is Terminal a class if it has no data? Those should just be free functions. \$\endgroup\$ Commented Aug 16, 2023 at 13:38
  • \$\begingroup\$ "$currentPlayer wins" may be justified by your loop invariants, but it would be safer for the function checking for wins to return the symbol it found a line of. \$\endgroup\$
    – Ben Voigt
    Commented Aug 16, 2023 at 19:43
  • \$\begingroup\$ @CrisLuengo - my thinking with Terminal was that it grouped together functions that would need to change together - for example, if the i/o changed to a GUI. Isn't a class a useful way of grouping functions that will change together? \$\endgroup\$
    – scripta
    Commented Aug 21, 2023 at 5:55
  • \$\begingroup\$ Functions should be grouped in a namespace. This is C++, not Java. How do you mean “change together”? Do you have plans to support maybe a GUI in the near future? If you want to swap UI at compile time, you’d use conditional compilation or instruct your build system to compile a different source file. If you want to switch at run-time, then yes, one way would be to have a base class that defines the interface, and multiple derived classes that implement the interface. You can then instantiate any of the derived classes and use it as a base class pointer. But you are not doing that here. \$\endgroup\$ Commented Aug 21, 2023 at 13:39

1 Answer 1

5
\$\begingroup\$

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.

\$\endgroup\$

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.