2
\$\begingroup\$

Othello is a two-player strategy game typically played over the board.

I implemented Othello in C++17 with CMake as the build system. I'm looking for feedback on cleanness, readability, and extensibility, in addition to the quality of the C++ (are there certain features I could have taken advantage of / are there features that I used inappropriately?).

Here is the project structure:

Othello
│
├── cmake-build-debug
│
├── src
│   ├── Controller
│   │   ├── Controller.cpp
│   │   └── Controller.h
│   ├── Model
│   │   ├── Model.cpp
│   │   ├── Model.h
│   │   ├── Player.cpp
│   │   ├── Player.h
│   │   └── Utility.h
│   └── View
│       ├── StandardView.cpp
│       └── StandardView.h
│
├── main.cpp
├── CMakeLists.txt
└── test

Here are the source files:

main.cpp

#include <iostream>
#include "Controller/Controller.h"

int main() {
    Othello::Controller othello;
    othello.SetUpGame();
    othello.PlayGame();
    return 0;
}

Controller

Controller.h

#ifndef OTHELLO_CONTROLLER_H
#define OTHELLO_CONTROLLER_H

#include "../View/StandardView.h"
#include "../Model/Model.h"

namespace Othello {
    class Controller {
    public:
        Controller();
        void SetUpGame();
        void PlayGame();
    private:
        Model* model_;
        StandardView* view_;

        void EnactMove(Move move);
    };
}

#endif //OTHELLO_CONTROLLER_H

Controller.cpp

#include "Controller.h"


void Othello::Controller::SetUpGame() {
    // Get size of the board and player names
    int board_size = Othello::StandardView::GetBoardSize();
    auto [black_name, white_name] = Othello::StandardView::GetPlayerNames();

    // Create the board and players
    model_->Init(board_size, black_name, white_name);

    // Set up the board with the usual configuration
    int center_row = board_size / 2 - 1;
    int center_col = center_row;

    Coordinate top_left = {center_row, center_col};
    Coordinate top_right = {center_row, center_col + 1};
    Coordinate bottom_left = {center_row + 1, center_col};
    Coordinate bottom_right = {center_row + 1, center_col + 1};

    model_->AddPiece(top_left, WHITE);
    model_->AddPiece(top_right, BLACK);
    model_->AddPiece(bottom_left, BLACK);
    model_->AddPiece(bottom_right, WHITE);
    view_->PrintBoard(model_);
}

void Othello::Controller::PlayGame() {
    // Game ends when neither player can make a move
    while (!model_->IsGameOver()) {
        // Check if active player can make a move
        // If not, skip turn
        if (!model_->TurnValid()) {
            model_->ChangeTurn();
            continue;
        }
        // If active player cannot move, turn is skipped
        auto move = view_->GetMove(model_); // We need to use the model to check if move is valid
        this->EnactMove(move);
        model_->ChangeTurn();
    }
//    view_.DeclareWinner(model_.GetWinner());
}

void Othello::Controller::EnactMove(Move move) {
    auto [coordinate, color] = move;
    model_->AddPiece(coordinate, color);
    auto tiles_flipped = model_->GetTilesFlipped(move);
    for (auto& tile : tiles_flipped) {
        model_->InvertCell(tile);
    }
}

Othello::Controller::Controller() {
    model_ = new Model();
    view_ = new StandardView();
}

Model

Model.h


#ifndef OTHELLO_MODEL_H
#define OTHELLO_MODEL_H


#include "Player.h"

namespace Othello {
    class Model {
    public:
        Model();
        void Init(int board_size, std::string black_name, std::string white_name);

        // Board editing utility
        void AddPiece(Coordinate coordinate, Color color);
        void InvertCell(Coordinate coordinate);


        // Getters & commonly used board state
        Color GetCellColor(Coordinate coordinate) const;
        int GetBoardSize() const;
        bool IsGameOver() const;
        bool CanMove(Player* player) const;
        bool IsValidMove(Move move) const;
        bool TurnValid() const;
        std::vector<Coordinate> GetTilesFlipped(Move move) const;

        // Active Player Methods
        Color GetActiveColor() const;
        std::string GetActiveName() const;
        void ChangeTurn();


    private:
        Player* black_;
        Player* white_;
        std::vector<std::vector<Cell>> board_;
        int board_size_;
        Player* active_player_;
        bool Inbounds(int coordinate) const;
    };
}



#endif //OTHELLO_MODEL_H

Model.cpp

#include "Model.h"

#include <utility>



Othello::Model::Model() : board_(0)
{}

void Othello::Model::AddPiece(Coordinate coordinate, Color color) {
    auto [row, column] = coordinate;
    auto& cell = board_.at(row).at(column);
    if (cell.color != EMPTY) {
        throw std::logic_error("You are trying to add a piece to a square that already has one.");
    }
    cell.color = color;
}

void Othello::Model::InvertCell(Coordinate coordinate) {
    auto [row, column] = coordinate;
    auto& cell = board_.at(row).at(column);
    Color curr_color = cell.color;
    if (curr_color == EMPTY) {
        throw std::logic_error("You are trying to invert a cell that is empty.");
    }
    else {
        Color new_color = (curr_color == BLACK) ? WHITE : BLACK;
        cell.color = new_color;
    }
}

void Othello::Model::Init(int board_size, std::string black_name, std::string white_name) {
    black_ = new Player(std::move(black_name), BLACK);
    white_ = new Player(std::move(white_name), WHITE);
    board_ = std::vector<std::vector<Cell>>(board_size, std::vector<Cell>(board_size));
    board_size_ = board_size;
    active_player_ = black_; // BLACK goes first by default
}

Color Othello::Model::GetCellColor(Coordinate coordinate) const {
    auto [row, column] = coordinate;
    auto cell = board_.at(row).at(column);
    return cell.color;
}

int Othello::Model::GetBoardSize() const {
    return board_size_;
}

bool Othello::Model::IsGameOver() const {
    return !CanMove(black_) && !CanMove(white_);
}

bool Othello::Model::CanMove(Othello::Player *player) const {
    int size = (int)board_.size();
    for (int row = 0; row < size; row++) {
        for (int column = 0; column < size; column++) {
            // If the cell is occupied, skip
            if (GetCellColor({row, column}) != EMPTY) continue;
            // From this cell, check in all directions
            Move move = {row, column, player->GetPlayerColor()};
            if (!GetTilesFlipped(move).empty()) {
                return true;
            }
        }
    }
    return false;
}

bool Othello::Model::TurnValid() const{
    return CanMove(active_player_);
}

bool Othello::Model::IsValidMove(Move move) const {
    return !GetTilesFlipped(move).empty();
}

std::vector<Coordinate> Othello::Model::GetTilesFlipped(Move move) const {
    auto [coordinate, same_color] = move;
    auto [start_row, start_column] = coordinate;
    Color alt_color = (same_color == BLACK) ? WHITE : BLACK;
    std::vector<Coordinate> directions = {
            {0, 1}, // Horizontal
            {1, 0}, // Vertical
            {1, 1}, // Right Diagonal
            {1, -1} // Left Diagonal
    };

    // offset for forward and backwards pass
    std::vector<int> offsets = {
            -1,                 // Backward Pass
             1                  // Forward Pass
    };

    std::vector<Coordinate> tiles_flipped;
    for (auto& direction : directions) {
        // for each direction, we perform a forward and backward pass
        for (auto offset : offsets) {
            auto [row_activated, column_activated] = direction;

            // We collect all alternate-color pieces
            std::vector<Coordinate> potential_tiles_flipped;

            // neat trick for combining forward and backward passes into one loop
            int row_offset = offset * row_activated;
            int column_offset = offset * column_activated;
            int row = start_row + row_offset;
            int column = start_column + column_offset;

            // we add the potential tiles flipped once we encounter a tile of the same color
            bool completed_flip = false;

            // flip check loop
            while (Inbounds(row) && Inbounds(column)) {
                auto cell_color = GetCellColor({row, column});
                if (cell_color == alt_color) {
                    potential_tiles_flipped.push_back({row, column});
                } else if (cell_color == same_color) {
                    completed_flip = true;
                    break;
                } else {
                    completed_flip = false;
                    break;
                }

                // update
                offset += offset;
                row_offset = offset * row_activated;
                column_offset = offset * column_activated;
                row = start_row + row_offset;
                column = start_column + column_offset;
            }
            if (completed_flip) {
                // flip all intermediate tiles if flip is completed
                tiles_flipped.insert(tiles_flipped.end(),
                                     potential_tiles_flipped.begin(),
                                     potential_tiles_flipped.end());
            }
        }
    }
    return tiles_flipped;
}

void Othello::Model::ChangeTurn() {
    bool is_black = (active_player_ == black_);
    active_player_ = (is_black) ? white_ : black_;
}

bool Othello::Model::Inbounds(int coordinate) const {
    return (coordinate >= 0) && (coordinate < board_size_);
}

Color Othello::Model::GetActiveColor() const {
    return active_player_->GetPlayerColor();
}

std::string Othello::Model::GetActiveName() const {
    return active_player_->GetName();
}

Player.h


#ifndef OTHELLO_PLAYER_H
#define OTHELLO_PLAYER_H
#include <string>
#include "Utility.h"

namespace Othello {
    class Player {
    public:
        Player(std::string name, Color color);
        std::string GetName();
        Color GetPlayerColor();
    private:
        std::string name_;
        Color color_;
    };
}



#endif //OTHELLO_PLAYER_H

Player.cpp

#include "Player.h"

#include <utility>

using std::vector;

Othello::Player::Player(std::string name, Color color) : name_(std::move(name)), color_(color) {
}

std::string Othello::Player::GetName() {
    return name_;
}

Color Othello::Player::GetPlayerColor() {
    return color_;
}

Utility.h

#ifndef OTHELLO_UTILITY_H
#define OTHELLO_UTILITY_H

#include "Player.h"
enum Color { EMPTY, BLACK,  WHITE };

inline std::string ToString(Color color) {
    switch (color) {
        case EMPTY:
            return "empty";
        case BLACK:
            return "black";
        case WHITE:
            return "white";
    }
}

struct Coordinate {
    int row;
    int column;
};

struct Cell {
    Coordinate coordinate;
    Color color;
};

struct Move {
    Coordinate coordinate;
    Color color;
};


#endif //OTHELLO_UTILITY_H

View

StandardView.h


#ifndef OTHELLO_STANDARDVIEW_H
#define OTHELLO_STANDARDVIEW_H


#include <utility>
#include <string>
#include "../Model/Model.h"

namespace Othello {
    class StandardView {
    public:
        StandardView() = default;

        // Input methods
        static int GetBoardSize();
        static std::pair<std::string, std::string> GetPlayerNames();

        // Output methods
        void PrintBoard(Model *model) ;

        Move GetMove(Othello::Model *model);
    };
}


#endif //OTHELLO_STANDARDVIEW_H

StandardView.cpp

#include "StandardView.h"
#include <iostream>

using std::cout, std::cin, std::string, std::pair;

int Othello::StandardView::GetBoardSize() {
    // TODO: Add validation for an even board size
    int board_size;
    std::cout << "Enter an even board size: \n";
    std::cin >> board_size;
    return board_size;
}

pair<std::string, std::string> Othello::StandardView::GetPlayerNames() {
    string black_name, white_name;
    cout << "Enter the name of player with the black pieces: \n";
    cin >> black_name;
    cout << "Enter the name of player with the white pieces: \n";
    cin >> white_name;
    return {black_name, white_name};
}

// Design taken from Code Review Stack Exchange
// https://codereview.stackexchange.com/questions/51716/shortest-possible-way-of-printing-a-specific-board
void Othello::StandardView::PrintBoard(Model *model) {
    int size = model->GetBoardSize();
    string letter_bar, decoration_bar;

    // We want to pad each line with a consistent amount of spaces
    int total_padding = (int)log10(size);
    // Construct letter bar
    for (int _ = 0; _ < total_padding; _++) {
        letter_bar += " ";
    }
    for (char letter = 'a'; (int)letter < 'a' + size; letter++) {
        letter_bar += letter;
        letter_bar += " ";
    }

    // Construct decoration bar
    for (int _ = 0; _ < total_padding; _++) {
        decoration_bar += " ";
    }
    decoration_bar += "+";
    for (int _ = 0; _ < size; _++) {
        decoration_bar += "--";
    }
    decoration_bar += "--+";

    // Print board
    cout << "   " << letter_bar << '\n';
    cout << " " << decoration_bar << '\n';
    for (int row = 0; row < size; row++) {
        int num_digits = (int)log10(row + 1);
        for (int _ = 0; _ < total_padding - num_digits; _++) {
            cout << " ";
        }
        cout << row + 1 << "| ";
        for (int column = 0; column < size; column++) {
            Color color = model->GetCellColor({row, column});
            switch (color) {
                case BLACK:
                    std::cout << 'B';
                    break;
                case WHITE:
                    std::cout << 'W';
                    break;
                case EMPTY:
                    std::cout  << ' ';
                    break;
                default:
                    throw std::logic_error("Cell has not been initialized.");
            }
            cout << " ";
        }
        std::cout << " |\n";
    }
    cout << " " << decoration_bar << '\n';
}

Move Othello::StandardView::GetMove(Othello::Model *model) {
    Move move{0};
    do {
        PrintBoard(model);
        auto color = model->GetActiveColor();
        auto name = model->GetActiveName();
        cout << "It's your move " << name << ".\n";
        cout << "You have the " << ToString(color) << " pieces.\n";
        cout << "Enter move as space separated row number and column character\n";
        cout << "Ex. 1 a\n";
        int raw_row; char raw_col;

        // add some validation here later
        cin >> raw_row >> raw_col;
        int row = raw_row - 1;
        int column = tolower(raw_col)-'a';
        move = {row, column, color};

    } while (!model->IsValidMove(move));
    return move;
}

CMakeLists.txt

cmake_minimum_required(VERSION 3.25)
project(Othello)

set(CMAKE_CXX_STANDARD 17)

add_executable(Othello src/main.cpp src/Controller/Controller.cpp src/Controller/Controller.h src/View/StandardView.cpp src/View/StandardView.h src/Model/Player.cpp src/Model/Player.h src/Model/Model.cpp src/Model/Model.h src/Model/Utility.h)

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Nicely formatted and interesting post. Hopefully you will get feedback \$\endgroup\$ Dec 25, 2023 at 5:40
  • \$\begingroup\$ You seem to be missing Controller.cpp. \$\endgroup\$
    – indi
    Dec 25, 2023 at 22:14
  • \$\begingroup\$ You're right, thanks for pointing that out @indi. Added. \$\endgroup\$
    – aadithyaa
    Dec 26, 2023 at 9:29

3 Answers 3

4
\$\begingroup\$

Project layout

I don’t do CMake if I can avoid it, so I won’t comment on that stuff.

I’ll first suggest that your main.cpp should probably be in src… which is where it seems to be according to CMakeLists.txt, but not according to the illustration. So, maybe the illustration is wrong?

Now, as for tests, you really should have them, and it looks like you’ve at least given that a thought, judging by the existence of the test directory. However:

  1. The directory is typically named tests (note the plural) for technical reasons (test is a standard POSIX utility).
  2. That directory is typically for functional testing, or maybe integration testing, not unit testing.
  3. For unit tests, the best recommendation I’ve seen—and the one I always use—is to include the unit tests alongside the units. So for a single logical module, you’d have Player.hpp (the interface), Player.cpp (the implementation), and Player.test.cpp (the unit tests), all together, side-by-side in the same directory. I don’t know how well this works with CMake.

High-level design

Using a model-view-controller pattern is all fine and good but… you know, you don’t actually need to explicitly use the names of the design pattern elements. This is especially true when the pattern names don’t really make sense in the practical context.

Put another way: You are not making an abstract software “thing”, you are making a game. And games have specific terminology and structure that all game developers understand. Even if a game’s renderer is technically the “view” in the MVC pattern, in the context of games we usually refer to it as the “renderer”. Design patterns are for understanding and communicating software structure; they are not meant to be explicitly implemented, so don’t be a slave to them.

And to make that point even more emphatically: I don’t think you’ve implemented the model-view-controller pattern properly. In a game, the “controller” is not the game… it’s just the input to the game. And your view is actually acting partly as a view, and partly as a controller. It’s all muddled.

In the MVC pattern, the model is the “thing” that holds all the data and business logic; the view is the way the model passes information to the user, and the controller is the way the user passes information to the model. The point of the MVC pattern is the view and controller abstract away that information exchange between the model and user, so you can change the way the user interacts with the model without changing the model.

If you imagine a physical console game system, the model is the game itself, the controller is the controller (the thing you hold and push buttons on to control the game), and the view is the screen. The idea of the MVC system is that you can swap out different controllers—you can change between game pad, keyboard-and-mouse, touchscreen, etc.—to change the way you control the game, but the game itself doesn’t change. Similarly, you can change the display—you can view it via a big-screen TV, a small tablet display, or a VR headset—to change the way you view the game, but again, the game remains unchanged.

So it doesn’t really make sense for the controller to set up a game. And you don’t play a controller, you play a game. So the methods SetUpGame() and PlayGame() don’t really make sense for the controller. For a game like Othello, pretty much the only thing you need to do to control the game is “get move”, to get the \$x\$ and \$y\$ coordinates of the square to place a piece in. You do have this method… but it’s in the view component.

For your specific implementation, you have a couple more things that you need from the controller. You need it to get the board size, and the player name… again, both of which are currently in the view. Basically, any input you need from the player to control the game should come from the controller… not the view, and not the model. But note: it should not be “get player nameS (plural)”, it should be “get player name (singular)”, because each player should have their own controller. Each player should also have their own view.

The standard game loop is, very basically:

while (not done)
{
    input();    // Process the player(s) input(s)
    update();   // Update the game state
    render();   // Display the game state
}

Using MVC terms:

auto game = model{};

while (not game.done)
{
    for (auto&& player : game.players)
        game.process_input(player.controller.get_input());

    game.update();

    for (auto&& player : game.players)
        player.view.render(game.state());
}

That’s more for a real-time game where all players can act at any time. For a turn-based game like Othello, you might want something more like:

// Probably makes more sense to call it "game" rather than "model":
auto model::play()
{
    while (not done)
    {
        // Probably makes more sense to just do stuff like:
        //  current_player.get_move()
        // rather than:
        //  current_player.controller.get_move()
        // but I'm illustrating the pattern.

        auto move = current_player.controller.get_move();

        update(move);

        for (auto&& player : players)
            player.view.render(state);
    }
}

All you need is a brief section before the loop to get the board size and player name(s), and that’s basically your game. Or, you could integrate the game setup into the game loop, by getting general “input” rather than specifically moves.

So the model is the game; the model class should probably be called game or othello.

Each player gets their own controller and view to interact with the model.

For maximum flexibility, you probably want the player to be an abstract class. That way you can support local human players (using standard input and output), online human players (using a network connection to get the game state and submit moves), AI players, and maybe more.

The controller and view should both be abstract base classes as well. That way, even if you only have a local human player, you can support console input and output (std::cin and std::cout), a graphical interface (using a mouse or touch to get moves), etc..

Code review

Generally, the code is nice and well-structured. (Though, I’m not a fan of indenting code at namespace scope. EVERYTHING is (or should be) in a namespace, so if you indent at namespace scope, you just waste 4+ columns of horizontal space for no good reason.) I’m not going to do an ordered, line-by-line review because there is so much code, and I’ll be mixing design suggestions along with actual code structure suggestions. So, let’s dive in.

Let’s start at the highest level: the main function:

int main() {
    Othello::Controller othello;
    othello.SetUpGame();
    othello.PlayGame();
    return 0;
}

So, first, Othello::Controller isn’t really a great name for what you’re making (setting aside the fact that it’s also logically incorrect). You’re supposed to be constructing a game, right? Not a controller (or model). I would think that, in practice, anyone constructing an instance is not interested in the design pattern being used; they just want to make a game. So it would make more sense for the type to be named game or othello or othello::game or something like that.

Now, I have to ask: will there ever be a situation where someone would want to set up a game, and not play it? Or, more interestingly, will there ever be a situation where someone would want to play a game without first setting it up?

No? Then why not just have a single function that does all the set up and plays the game?

Types should be easy to use, and hard—if not impossible—to misuse. Any time your interface has a set of functions that must all be called in a certain sequence… why put that responsibility on the user? Why not just do all the work internally, and only a expose a single function that does the whole sequence?

Consider std::string. When you assign an array of characters to a std::string, it has to free its current data, allocate new memory for the new data, then copy the data into that memory. So basically, this:

auto str = std::string{};
str.free_current_memory();
str.allocate_new_memory(std::strlen("hello"));
str.copy_characters_into_memory("hello");

Now, imagine what a nightmare it would be to have to write the above code every time you want to assign new data to a std::string, instead of just:

auto str = std::string{};
str.assign("hello");

or:

auto str = std::string{};
str = "hello";

or even just:

auto str = std::string{"hello"};

Looking at the code for your class, I see:

    Othello::Controller othello;
    othello.SetUpGame();
    othello.PlayGame();

and I ask, why not:

    // Set up is either done in the constructor, or at the start of PlayGame()
    Othello::Controller othello;
    othello.PlayGame();

Now, regarding std::string you can do:

auto str = std::string{};
str.clear();
str.reserve(std::strlen("hello"));
str.assign("hello");

Which is basically the same as the nightmare code above. std::string does give you that level of control, and you may make the case that you want that level of control for your game class: there may be cases where you want to set up the game separately from playing it, maybe to do something else in between. But here is the really, really important point about that: std::string does gives you that level of control… if you want to use it. And here’s the second really, really important point about that: no matter what you do, no matter what order you call those functions in, no matter if you forget one or two… everything always still works.

What I’m saying is: If your class requires calling certain functions and/or requires calling functions in a certain order… then your class is broken.

So rather than requiring the user to call “setup” then “play”… it should just work with “play”, doing the setup implicitly and automatically. And if you want to have separation between “setup” and “play” (and you shouldn’t; more on that in a moment), then everything should still work if the user forgets to call “setup”, or calls it multiple times, or whatever.

So you just need a “play” function for the game class. You could make a separate “setup” function… but I would suggest not. Because the more crap you add to an interface, the more you have to test. And you should test your code. If you just a “play” function, you just have one function to test. But the moment you add a separate “setup” function… now you have two functions to test… and you need to test the interaction between those two functions (“What happens if you forget to call ‘setup’ before ‘play’?” “What happens if you call ‘setup’ multiple times before ‘play’?” “What happens if you call ‘setup’ but don’t call ‘play’?”). So don’t complicate the interface without a damn good reason. All you need is “play”.

So this is what I’d suggest your main function should look like (roughly; never mind the style choices like trailing return and AAA):

auto main() -> int
{
    auto game = othello::game{};
    game.play();
}

I’m going to skip to the model class… which should be the game class (not controller).

Let’s start by looking at the data members:

        Player* black_;
        Player* white_;
        std::vector<std::vector<Cell>> board_;
        int board_size_;
        Player* active_player_;

You have 3 naked pointers there. 2 of them are owning, 1 is not. All of this is very bad.

Let’s start with the owning pointers. Using naked pointers for ownership is completely unacceptable in modern C++. There is just no excuse for it anymore. You should be using smart pointers. And, if you had used smart pointers—and particularly if you used the correct smart pointer std::unique_ptr—then you would have avoided several rather massive bugs that exist in your code.

Bug #1: Your code leaks. You use new, but there is not a single delete anywhere in your code. At the very least, Model should have a destructor to free the memory you allocated with the new calls.

But even a destructor would not be enough because:

Bug #2: Your Model class automatically gets copy operations, which will just copy those raw pointers. Which means if you just naïvely delete them, you’ll probably end up with a double delete, and a crash. So Model should also have a copy constructor and copy assignment operator… and probably move constructor and move assignment as well.

All of those bugs and all of that extra work magically goes away if you just replace those naked pointers with std::unique_ptr<Player>. This is why smart pointers are not optional in modern C++. If you are not using smart pointers, your code is not acceptable in any modern C++ project.

Now, what about that third pointer, the non-owning one.

This isn’t technically a problem, in that it isn’t actively creating bugs. But it’s still not great. The problem is that you’ve coupled two (or three) data members together. If ever you change the player (either one!), then you must remember to change the active player pointer, too. Right now, you change them all together, which is good. But this is brittle. If your game gets more complex… like, say, maybe you make it possible to change players mid-game (if a player disconnects and reconnects, or maybe they have to quit but what to hand the game over to someone who will finish it for them, or whatever)… then you have to be very careful to remember to keep all the pointers in sync.

Let offer an alternative design.

Right now you are hard-coding two players, and hard-coding them as separate data members. What if, instead, you coded the players like this:

class Model
{
    // ... [snip] ...

    std::array<std::unique_ptr<Player>, 2> players_ = {};
    std::size_t active_player_index_ = {}

    // ... [snip] ...
};

Couple things to note:

  1. It is now trivially possible to add more players. Just change the size of the array, or replace it with a vector to have a dynamically-adjustable number of players.
  2. Even if you say you will never, ever have more than two players, coding them like this makes other places in the code simpler. For example, look at your ChangeTurn() function. With the structure above, that becomes a one-liner: active_player_index_ = (++active_player_index_) % players_.size();.
  3. By using an index to keep track of the active player, you are now free from worry about pointers going stale.

Last thing to note about those pointers… do they need to be pointers? I mean, yes, if you were using polymorphism with an abstract player base class, then sure, pointers would make sense. But… you’re not. There is no reason to hold pointers to players when you could just as well hold the players directly. In other words:

class Model
{
    // ... [snip] ...

    std::array<Player, 2> players_ = {};
    std::size_t active_player_index_ = {}

    // ... [snip] ...
};

Now let’s look at the board itself:

        std::vector<std::vector<Cell>> board_;

This is a bad pattern; you (almost) never want vectors of vectors.

What you want is a 2D array, right? The way to do that is to use a flat array, and calculate the indices.

In other words, to get a 8×10 array, you would do:

auto board = std::vector<Cell>(8 * 10);

// I want to access cell (3, 5):
board[(3 * 8) + 5].color = black;

In C++23, we get std::mdspan to make this easier:

auto board = std::vector<Cell>(8 * 10);
auto board_view = std::mdspan{board.data(), 8, 10};

// I want to access cell (3, 5):
board_view[3, 5].color = black;

Because your board is always square, you can just store a single extent, like you’re doing now:

class Model
{
    // ... [snip] ...

    std::vector<Cell> board_ = {};
    int board_size_ = {};

    // ... [snip] ...
};

auto Model::AddPiece(Coordinate coordinate, Color color)
{
    auto const board_view = std::mdspan{board_.data(), board_size_, board_size_};

    auto const [row, column] = coordinate;

    if (board_view[row, column].color != EMPTY)
        throw std::logic_error{"You are trying to add a piece to a square that already has one."};

    board_view[row, column].color = color;
}

And you can always assert that board_.size() == std::pow(board_size, 2);. (Or you can allow for non-square boards by including an extents object directly. In fact, you could even theoretically allow for 3D or higher dimensional boards. Once you avoid hard-coding assumptions into the code, a lot of amazing things become possible, even if you never take advantage of them.)

The rest of the member functions in Model are okay, but I would ask if they are necessary. The reason you need them now is because you’ve split the model logic across the view and controller objects, but if all the model logic was contained in Model, then you wouldn’t need all those manual operations to be public. The only public function Model would need is a “play game” function (the one that is currently in Controller, basically). The fewer functions in the public interface the better, because then there is no need to test them.

So, the minimum interface for Model (which, again, should probably be called Game or Othello) is:

class Model
{
public:
    // Creates a new Othello game with the given players:
    template <std::ranges::input_range R>
        requires std::convertible_to<std::ranges::range_value_t<R>, Player*>
    explicit Model(R&& players);

    // Or if you want to hard-code it to just two players:
    Model(Player*, Player*);

    // Play the game!
    auto play() -> void;
};

Which you’d use like:

// Player is an abstract base class. StdioPlayer is a player that uses
// std::cin for its controller, and std::cout for its view.
class StdioPlayer : public Player
{
    // ...
};

auto main() -> int
{
    // Set up the players
    auto const player_1 = std::make_unique<StdioPlayer>();
    auto const player_2 = std::make_unique<StdioPlayer>();

    // Play the game!
    auto othello = Othello::Game{player_1.get(), player_2.get()};
    othello.play();
}

Which is an interface that is both simple, and extremely hard to misuse.

Okay, on to the Player class.

Currently, you’re just using the player class to hold information about the player—their name and piece colour. You are not using the player class to be the player.

The player class should have a view (to view the state of the model/game) and a controller (to control the state of the model/game). Anything else is gravy.

In particular, there’s no harm in storing the player’s name in the player class, because if the player changes their own name… meh. Doesn’t really affect the game. But storing any game state information in the player class is probably not wise. For example, storing the piece colour there is dodgy. What happens if the player changes their own piece colour mid-game? They shouldn’t have that control. It would probably be better to store any game state info connected to a player in the model. The game needs to know what each player’s colour is, after all.

Speaking of colour:

enum Color { EMPTY, BLACK,  WHITE };

inline std::string ToString(Color color) {
    switch (color) {
        case EMPTY:
            return "empty";
        case BLACK:
            return "black";
        case WHITE:
            return "white";
    }
}

First, never use SCREAMING_SNAKE_CASE (aka ALL_CAPS) for anything but macros.

Second, you should use modern C++ scoped enums, not C enums. So that should be:

enum class Color
{
    empty,
    black,
    white
};

Third, you have a toString() function to get strings for the enumerators, which is not terrible… but consider that the only place you use it is here:

Move Othello::StandardView::GetMove(Othello::Model *model) {
    // ... [snip] ...
        cout << "You have the " << ToString(color) << " pieces.\n";

So what you are doing is creating a string for the sole purpose of just printing that string. There is no need for that. You could just do:

auto operator<<(std::ostream& out, Color color) -> std::ostream&
{
    switch (color)
    {
    case Color::empty:
        return out << "empty";
    case Color::black:
        return out << "black";
    case Color::empty:
        return out << "white";
    }
}

Move Othello::StandardView::GetMove(Othello::Model *model) {
    // ... [snip] ...
        cout << "You have the " << color << " pieces.\n";

But the biggest issue is that you have an “empty” enumerator. This is not a good thing. There is no such colour as “empty”. You need it because you are using the colour enumeration for more than just colours; you are also using it to mark the absence of colour. But that’s conceptually nonsensical.

A more correct model would have each space on the board holding a std::optional<color>, which logically indicates that each space may be occupied by a colour, or not.

But a better idea might be to encode each space with with the player who owns it, rather than the colour, and use some out-of-bounds value, like std::size_t{-1}, to encode a space that is not occupied. If you have the set of players stored as an array or vector of Player, then you can use the indices (or maybe the index plus one, so you get the more natural “player 1”, “player 2”, etc., rather than “player 0”, “player 1”, etc.… and you can use 0 to encode “no player”, which is natural and efficient). Doing it this way will spare a lot of the repetitive trinary checks ((curr_color == BLACK) ? WHITE : BLACK;).

But the main point is that if you are creating a class to encode colour, just have it encode colour, not “colour + other info”. When you do need “colour + other info”, then you have options. But if you hack “+ other info” in there from the start, you’re stuck with it even when it doesn’t make sense. For example, in Player you have GetPlayerColor(), which returns a Color. If Color really were just for colours, then GetPlayerColor() could never return an invalid value… but Color is “colour + error state”, which means that every single line of code that uses GetPlayerColor() now needs to check the return to make sure it is not Color::EMPTY. Which defeats the purpose of creating a type specifically for colours.

So, back to Player. It should probably not be copyable, because it doesn’t make sense to copy players. So maybe:

class Player
{
public:
    constexpr explicit Player(std::string name, std::unique_ptr<View> view, std::unique_ptr<Controller> controller)
        // pre (not name.empty())
        // pre (view)
        // pre (controller)
        : _name{std::move(name)}
        , _view{std::move(view)}
        , _controller{std::move(controller)}
    {}

    ~Player() = default;

    constexpr auto view() & noexcept -> View& { return *_view; }

    constexpr auto controller() & noexcept -> Controller& { return *_controller; }

    constexpr auto name() const& noexcept -> std::string const& { return _name; }
    constexpr auto name() &&     noexcept -> std::string        { return std::move(_name); }

    // Non-copyable:
    constexpr Player(Player const&) = delete;
    constexpr auto operator=(Player const&) -> Player& = delete;

    // Movable:
    constexpr Player(Player&&) noexcept = default;
    constexpr auto operator=(Player&&) noexcept -> Player& = default;

private:
    std::unique_ptr<View>       _view       = {};
    std::unique_ptr<Controller> _controller = {};

    std::string _name = {};
};

And the model/game class could hold an array of players plus any extra info like so:

class Model
{
    // ... [snip] ...
    std::array<std::tuple<Player*, Color, /* anything else */>, 2> _players = {};
    std::size_t _active_player = {};
};

At this point, I saw you’d already accepted an answer, so there’s no point in going further. So, I guess I’ll wrap it up.

The key point I wanted to make is that you have not done the model-view-controller pattern properly.

In MVC, the model is the core; it is what handles all the data and logic. The view is just a way for users (players) to view the state of the model, and the controller is just a way for users to control the state of the model (by sending commands for what moves to make). The controller should absolutely not be setting up the game, or playing it. Neither should the view be getting moves.

Extended with discussion from comments

MVC pattern in games

So, an important thing to keep in mind is that there is no one true MVC pattern. There are many of versions of MVC. They all have the same basic idea, but they differ wildly in the details. In some versions, the user only interacts with the view, not the controller or the model. In some others, the user only interacts with the controller, not the view or the model.

One of the reasons for the many variations of MVC is that MVC is normally applied to GUI applications, and in that context, you can get a little vague about exactly where the components are and how they interact. An app’s GUI can be both view and controller (a checkbox, for example, must both view data in the model and control data in the model). You can say the GUI is the view, and it communicates with the controller when you change something in the GUI. So the pattern looks like this:

MODEL
  ↓  ↖
  ↓  CONTROLLER
  ↓  ↗
VIEW
  ⇅
USER

But that pattern doesn’t really work for games, because a game’s graphics and input systems are totally separate… often literally, being provided by totally different libraries (you may use GameInput for input and Vulkan for graphics, for example). And games are usually designed with the input wholly independent from the graphics, so you can play the same game with a game pad, or keyboard and mouse, or via a touchscreen… sometimes all on the same device.

You want a version of MVC that works well for games. And for that, you need to have strict separation between graphics and input, which means strict separation between view and controller. So, this:

     MODEL
    ↙    ↖
VIEW      CONTROLLER
    ↘    ↗
     USER

In my opinion, this is the flavour of MVC that works best for games, but there can still be some variation. For example, you may want to allow the controller to query the view, so that when the user clicks on an interface object, or points at an object in the visual scene, you can get the view/projection matrices and screen coordinates to transform what the user is pointing at back to world space, so you can detect what object they’re selecting.

What I’m trying to hammer home is: There is no one true “correct” way to apply MVC to games… but there are ways that work well and ways that don’t work so well, and unfortunately, a lot of MVC literature focuses on GUI applications, which often give flavours of MVC that don’t really work well for games.

So the direct answer to your question is: Only the model should be isolated from the user. The view and controller should both be independent, so you can swap out different views (hi-res screen display, low-res phone display, VR headset, etc.) and different controllers (game pad, keyboard/mouse, touch, etc.) independently.

As for the second part of the question… again, MVC is just a theoretical pattern, and a vaguely specified one at that, so there is no one true answer about who instantiates what.

One possibility is to have the user/player responsible for instantiating the view and controller. In my mind, this works well for client-server model games, which are great for multiplayer. In that flavour, the model is the game server; it handles all the game logic, and keeps all the connected players in sync. Each client/player gets their own view and controller, so they can each have not only different views and input methods, but different types of views and input methods (one might be playing on a touchscreen phone, another with keyboard/mouse on a desktop, etc.).

Another flavour would make the model/game responsible for setting up the view and controller. This is how most non-networked games are probably structured. By giving the model/game knowledge of the view and controller, the model/game can optimize its internal logic/calculations for the current view and input method. For example, if the game knows it’s being played on a small screen, it can avoid loading any hi-res models, and only load the low-res stuff.

So, the answer to the second part of your question is… dealer’s choice. You have options. As I was writing the main answer, I deliberately varied between different patterns of doing things. You could set up the players before the game.run() function… or you can have the player set up as part of the game.run() function. In a more complicated game, the game.run() function would usually kick off a loop that is controlled by a state machine, where the state determines the current “mode” of the game. So the game might start in the “introduction screen” state, then when conditions are met go to the “player set up” state, then once the players are set up, go to the “play” state, and stay there until the “win” state.

And to directly answer the question, you could instantiate the controller (and/or view) in the model (either in the constructor, or at the start of .run()), or in each player (either before instantiating the model entirely, or you could have the players created at the start of .run()).

There is no one “true” answer. You’re the engineer of this locomotive; you get to decide how it runs. Sure, there are some patterns that are just… not great; and some patters are generally superior. But once you weed out the really bad designs, you get to a point where there is no clear winner, only trade-offs. Some patterns are better than others for extensibility, but terrible for performance, and vice versa.

For example, instantiating the players before the model entirely is probably a great pattern if you are making a collection of games, where the players can set themselves up, then choose to play Othello or chess or whatever. On the other hand, that might mean you need two set-up phases, where first you set up the players (“enter your names!”), then start the game, then set up the players again (“choose your colour!”; and for some games, “choose your starting position!”, and so on). So for a single game, maybe having the model set up the players at the start of .run() is better. As I said: you’re the engineer!

And finally, keep in mind that whenever you try to apply theoretical structure to a practical application, you’ll often have to futz things. As they say, theory never survives contact with reality. A “pure” MVC structure is probably not possible. And this is especially true for games, which often do horrific and ugly things for the sake of optimization (and release deadlines). I’ll just straight out and say it: in practice, game programmers are not coders, they are butchers. They are hackers. And they’re often quite proud of that.

\$\endgroup\$
3
  • \$\begingroup\$ Thank you so much for taking the time to write such a detailed review. I had not given much thought to the pointer usage. I was aware of the first bug you mentioned (deleting all the naked pointers) and planned to fix it eventually, but the second bug I hadn't even thought of. As for getting MVC right, I should essentially treat the View and Model as almost "isolated" from the user in a way? If so, and I put the run() function in the model instead, where should I instantiate the Controller? Inside the run() function or in main.cpp? \$\endgroup\$
    – aadithyaa
    Dec 27, 2023 at 3:43
  • \$\begingroup\$ Rather than try to pack a detailed answer about software design and the MVC pattern into comments, I’ll extend the answer above with some suggestions about MVC in games. \$\endgroup\$
    – indi
    Dec 29, 2023 at 14:56
  • \$\begingroup\$ You deserve more upvotes \$\endgroup\$ Jan 11 at 11:46
4
\$\begingroup\$

Here are some things that may help you improve your code. The code already has a pretty good class structure and uses a namespace intelligently.

Make sure you have all required #includes

The code uses std::vector but doesn't #include <vector>. Also, carefully consider which #includes are part of the interface (and belong in the .h file) and which are part of the implementation. In the case of Model.h, the interface mentions std::vector so the #include <vector> should be in the Model.h file.

Use only necessary #includes

The #include <iostream> line is not necessary within main.cpp and can be safely removed.

Don't have cyclical #includes

This code has Player.h including Utility.h which includes Player.h. Most compilers won't be too bothered with that, but it's not a good practice and isn't needed at all here. Just delete the line in Utility.h. See SF.9 for more details.

Prefer to avoid using new and delete directly

In modern C++, there is generally very little reason to to use raw new and delete calls. One reason is that it's very easy to allocate memory with new but forget to free it with delete and in fact, there are no corresponding calls to delete for any of the objects allocated with new, include black_ and white_ from Model and model_ and view_ from Controller. None of these objects needs explicit memory handling. Instead, use references instead of pointers and construct the objects directly. See R.11 for details.

Use const where practical

In a number of places in the code, including the Player::GetName() and Player::GetPlayerColor() functions, the function does not alter the underlying object and so the function should be declared const. Similarly, for the Model::CanMove() function, the passed Player object is not altered and so that parameter should also be const.

Minimize public members

Nothing outside of the Model needs access to CanMove(), so that function could be private. See C.9 for details.

Don't create a non-functional object

The default constructor for Model creates a 0-sized board. That's not really very useful! Instead, I'd recommend moving the code for getting board dimensions and names out of StandardView and instead put it into main.cpp or move it to a different class. That would allow you to get the information required to create a useful board and then pass those parameters to the Model constructor which would then incorporate the code currently in SetUpGame. See C.41 and ES.22 for more.

Use an enum class where appropriate

For the Color enum, the only valid values are the listed ones. In that case, it makes sense to use an enum class like this:

enum class Color { Empty, Black,  White };

inline std::string ToString(Color color) {
    if (color == Color::Black) {
        return "black";
    } else if (color == Color::White) {
        return "white";
    }
    return "empty";
}

This will also allow you to easily avoid creating an invalid player:

Othello::Player::Player(std::string name, Color color) : name_(std::move(name)), color_(color) {
    if (color == Color::Empty) throw std::logic_error("You can't create an empty player");
}

Initialize strings efficiently

In a few places within PrintBoard we have things like this:

for (int _ = 0; _ < total_padding; _++) {
    letter_bar += " ";
}

Better would be to declare and initialize all at once like this:

std::string letter_bar(total_padding, ' ');

However, there are even more optimizations one could make as I'll describe later.

Rethink strict model-view-controller

While it's a good pattern to use, I think that the separation here is somewhat getting in the way of efficient code. For example, the PrintBoard could be a function of the Model object. This would allow many of the calculations , such as for the letter_bar shown above, and others, to be static and only initialized once rather than created and destroyed each invocation of PrintBoard.

Simplify printing by using stream inserters

There are many simplifications you could apply to PrintBoard. Here are a few. First, let's assume that we either eliminate the PrintBoard routine entirely in favor of an extractor or redefine its body in terms of an extractor: std::cout << model;

Now we need to write an inserter. Generally, an inserter for a custom object would have a signature like this:

std::ostream& operator<<(std::ostream& out, const Othello::Model& model);

The first three lines are these:

int size = model.GetBoardSize();
int num_width = (int)log10(size) + 1;
auto decoration_bar{std::string(num_width, ' ') + "+" + std::string(2 * size + 1, '-') + "+\n"};

Note that here, num_width is effectively the size of the largest number as printed. We use that when constructing the decoration_bar.

Next we want to print the letters. Since, unlike the decoration_bar we're only using this string once, there's no need to store it. We can just print it directly. Here's a way to do that and print the top decoration_bar:

out << std::setw(num_width) << ' ';
char ltr{'a'};
for (int i{size}; i; --i, ++ltr) {
    out << std::setw(2) << ltr; 
}
out << '\n' << decoration_bar;

Next, we need to print the rows. We can define a stream inserter for the Cell type. Here, I've used the enum class types I mentioned above.

std::ostream& operator<<(std::ostream& out, const Cell& c) {
    if (c.color == Color::Black) {
        out << 'B';
    } else if (c.color == Color::White) {
        out << 'W';
    } else if (c.color == Color::Empty) {
        out  << ' ';
    }
    return out;
}

This will make things easier, but only if we also add these two small functions to the Model class:

const auto begin() const { return std::begin(board_); }
const auto end() const { return std::end(board_); }

These provide a read-only way to access the underlying container. With that provided, the rest is fairly simple:

int row_num = 1;
for (const auto& row : model) {
    out << std::setw(num_width) << row_num << "|";
    for (const auto& cell : row) {
        out << std::setw(2) << cell; 
    }
    out << " |\n";
    ++row_num;
}
return out << decoration_bar;

Consider a newer compiler

Some things are made simpler with the new features of C++20 or C++23. For example, one could print the board as above, but using the ranges and format (from C++20).

set features by target (CMake)

The CMake file currently has this:

set(CMAKE_CXX_STANDARD 17)

That works, but it's often better to set by target:

target_compile_features(Othello PRIVATE cxx_std_17)

Reconsider the use of relative paths

Several files have a relative path such as this:

#include "../Model/Model.h"

If you want to move that file to another directory, you would have to touch every file that includes it and also modify your CMakeLists.txt file. For that reason, it's often better to omit relative names from the various C++ files and instead tell CMake to tell the various targets where they can be found. For instance:

target_include_directories(Othello PRIVATE ${CMAKE_SOURCE_DIR}/src/Model)
\$\endgroup\$
3
\$\begingroup\$

Model, View, Controller separation

It's great that you split your code mostly correctly into a model, view and controller parts. It limits the responsibilities of each part, resulting in simpler and more maintainable code. Another benefit of the MVC pattern is that you can easily swap out one part for another, for example a CLI view for a GUI one, without any other part having to be changed.

The only issue I am seeing here is that despite the separation, everything is still hardcoded; Controller always uses the exact same Model and StandardView. Ideally, the controller just takes the model and view as parameters. This can be done in two ways:

  1. Making base classes for the model, view and controller, and having a constructor for Controller that takes pointers to concrete view and model objects.
  2. Making Controller a template that takes the model and view types as parameters. The controller could then still instantiate the model and view itself, but even better would be to take references to concrete objects of those types as parameters in the constructor.

Another issue is that you still do a bit too much in Controller. For example, in EnactMove() you coded the fact that tiles might get flipped after placing a stone. However, this is something that should be done in the model instead; the controller should only worry about the things that the player controls, which is placing stones. The actual game logic is something for the model.

Make SetUpGame() a private member function

You have SetUpGame() and PlayGame(), and the only valid way to use them is to call them in succession. I would make SetUpGame() a private member function instead, and have PlayGame() call it at the start.

As a slight benefit, it can simplify your main() to:

int main() {
    Othello::Controller().PlayGame();
}

Avoid manual memory management

Instead of using new and forgetting to call delete like you did, which leads to memory leaks, prefer to use smart pointers like std::unique_ptr where possible.

Even better is not to use pointers at all where possible. I would store black_ and white_ by value. You could also change Player* active_player_ to Color active_color_, and have a helper function to get a reference to the active player object using the active color.

Unnecessary use of this->

You almost never have to write this-> in C++. You can remove it in PlayGame().

Store the cells in a one-dimensional array

While a vector of vectors is a quick way to create a two-dimensional vector, it's not very efficient. A better way is to store the cells in a one-dimensional vector of board_size_ * board_size_ elements, and instead of writing board_.at(row).at(column), write board_[row * board_size_ + column]. To avoid code duplication, you can write a helper function that returns a reference to a Cell given a row and column index.

C++23 will make it easier to work with multi-dimensional arrays by using std::mdspan.

Incorrect exception type

std::logic_error is meant for fauly logic in the program itself, not for incorrect user input. You want std::runtime_error instead, or create your own exception type that derives from it.

Avoid std::pair if possible

The problem with std::pair is that you have to remember which element is which. Create a struct instead where possible. For example:

struct PlayerNames {
    std::string black;
    std::string white;
};

PlayerNames Othello::StandardView::GetPlayerNames() {
    …
    return {black_name, white_name};
}

You can then still write:

auto [black_name, white_name] = Othello::StandardView::GetPlayerNames();

But that still assumes a particular order. I would rather write:

auto names = Othello::StandardView::GetPlayerNames();
model_->Init(board_size, names.black, names.white);

Or maybe even pass a reference to a PlayerNames directly to Init()?

\$\endgroup\$
2
  • \$\begingroup\$ Thank you so much for the detailed feedback! I implemented some of your suggestions, but I have a few questions about the templates. My linker couldn't find the template definitions, and it only seemed to work when I moved the template definitions into the Controller.hpp file. Is there a cleaner way to do this? I want to keep separation of implementation and declarations. \$\endgroup\$
    – aadithyaa
    Dec 27, 2023 at 1:36
  • 1
    \$\begingroup\$ You can use explicit template instantiation to fix this, although templates are most useful when they are fully defined in a .hpp file. \$\endgroup\$
    – G. Sliepen
    Dec 27, 2023 at 9:38

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.