Minesweeper in C++ using objects

Question

After taking a course in C++, I have written my first program and I'm looking for some feedback both on coding style and maybe algorithmic improvements. The code can also be found at GitHub. It compiles and runs fine but is not very robust for user input.

I opted to try to emulate the game Minesweeper. My implementation is as follows:

#include "minesweeper/minesweeper.h"

int main()
{
    Minesweeper game{9};
    game.play();
}

First I have a class which I use as a 'cell':

#ifndef MINESWEEPERCELL_H_
#define MINESWEEPERCELL_H_

#include <cstddef>

class MinesweeperCell
{
    bool d_isBomb = false;
    char d_state = '*';             // * or F
    std::size_t d_numBombsNear = 0;
    bool d_visited = false;

    public:
        MinesweeperCell() = default;
        void setBomb();
        bool isBomb() const;
        void setState(char const ch);
        char state() const;
        void setNumBombsNear(std::size_t const num);
        std::size_t numBombsNear() const;
        bool visited() const;
        void setVisited();
};    
#endif

The implementations of these getters and setters are trivial and hence are left out for brevity. They can be found at the GitHub project.

Next is the most important class:

#ifndef MINESWEEPER_H_
#define MINESWEEPER_H_

#include <cstddef>
#include <vector>
#include <iosfwd>
#include <utility> 

#include "../minesweepercell/minesweepercell.h"

class Minesweeper
{
    std::size_t d_size;        // square only so far
    std::vector<std::vector<MinesweeperCell>> d_gameBoard;
    std::vector<std::pair<int, int>> d_bombLocations;
    public:
        Minesweeper(std::size_t const size = 9);
        void play();
    private:
        void initializeBoard();     // Places randomly d_size + 1 bombs on board
        void setNumBombsNear();
        void checkSurrounding(std::size_t const xCoord, std::size_t const yCoord);
        void showBombs();
        bool processInput(char const cmd, std::size_t const xCoord, std::size_t const yCoord);
        friend std::ostream &operator<<(std::ostream &out,
                                        Minesweeper const &minesweeper);
};    
#endif

I have implemented the member functions as follows (following the 1 function per file idiom, using internal headers):

// minesweeper.ih
#include <random>
#include <iostream>

#include "minesweeper.h"

using namespace std;

I am aware of the pitfalls of including the whole standard namespace. In my opinion, its ability to improve the readability of my code is more important than rare namespace clashes.

#include "minesweeper.ih"

Minesweeper::Minesweeper(size_t const size)
:
    d_size{size},
    d_gameBoard{vector<vector<MinesweeperCell>>(size, vector<MinesweeperCell>(size))}
{
    initializeBoard();
    setNumBombsNear();
}

.

#include "minesweeper.ih"

void Minesweeper::play()
{
    bool notDone = true;
    while (notDone)
    {
        char cmd;
        cin >> cmd;
        if (cmd ==  'q')
            break;

        size_t xCoord, yCoord;
        cin >> xCoord >> yCoord;
        if (xCoord < 0 or xCoord >= d_size or yCoord < 0 or yCoord >= d_size)
        {
            cout << "Please input valid coordinates\n";
            continue;
        }

        notDone = processInput(cmd, xCoord, yCoord);

        cout << *this;
    }
}

.

#include "minesweeper.ih"

void Minesweeper::initializeBoard()
{
    size_t numOfBombs = 0;
    uniform_int_distribution<int> distribution(0, d_size - 1);
    default_random_engine generator(random_device{}());

    while (numOfBombs != d_size + 1)
    {
        // generate a pair of indices
        pair<int, int> randomNums{distribution(generator), distribution(generator)};
        if (d_gameBoard[randomNums.first][randomNums.second].isBomb())
            continue;
        ++numOfBombs;
        d_gameBoard[randomNums.first][randomNums.second].setBomb();
        d_bombLocations.push_back(randomNums);
    }
}

.

#include "minesweeper.ih"

void Minesweeper::setNumBombsNear()
{
    int moves[8][2] = { {-1,0}, {-1, -1}, {0, -1}, {1, -1},
                        {1, 0}, {1, 1}, {0, 1}, {-1, 1}     };
    for (size_t row = 0; row < d_size; ++row) 
    {
        for (size_t col = 0; col < d_size; ++col) 
        {
            size_t num = 0;
            for (size_t idx = 0; idx != 8 /*sizeof(move) / sizeof(move[0]) */; ++idx)
            {
                size_t possibleRow = row + moves[idx][0];
                size_t possibleCol = col + moves[idx][1];
                if (possibleRow < 0 or possibleRow >= d_size or possibleCol < 0 or possibleCol >= d_size)
                    continue;
                num += d_gameBoard[possibleRow][possibleCol].isBomb() ? 1 : 0;
            }
            d_gameBoard[row][col].setNumBombsNear(num);
        }
    }
}

.

#include "minesweeper.ih"

void Minesweeper::checkSurrounding(size_t const xCoord, size_t const yCoord)
{
    static int moves[8][2] = { {-1,0}, {-1, -1}, {0, -1}, {1, -1},
                        {1, 0}, {1, 1}, {0, 1}, {-1, 1}     };
    for (size_t idx = 0; idx != 8 /*sizeof(move) / sizeof(move[0]) */; ++idx)
    {
        size_t possibleX = xCoord + moves[idx][0];
        size_t possibleY = yCoord + moves[idx][1];
        if (possibleX < 0 or possibleX >= d_size or possibleY < 0 or possibleY >= d_size)
            continue;
        if (d_gameBoard[possibleX][possibleY].visited())
            continue;
        d_gameBoard[possibleX][possibleY].setVisited();
        if (d_gameBoard[possibleX][possibleY].numBombsNear() == 0)
        {
            d_gameBoard[possibleX][possibleY].setState('0');
            checkSurrounding(possibleX, possibleY);
        }
        else if (d_gameBoard[possibleX][possibleY].numBombsNear() != 0)
        {
            d_gameBoard[possibleX][possibleY].setState('0' + d_gameBoard[possibleX][possibleY].numBombsNear());
        }
    }
}

.

#include "minesweeper.h"

void Minesweeper::showBombs()
{
    for (auto &location : d_bombLocations)
    {
        d_gameBoard[location.first][location.second].setState('B');
    }
}

.

#include "minesweeper.ih"

bool Minesweeper::processInput(char const cmd, size_t const xCoord, size_t const yCoord)
{
    switch (cmd)
    {
        case 'f':
            d_gameBoard[xCoord][yCoord].setState('F'); 
            break;
        case 'u':
            d_gameBoard[xCoord][yCoord].setState('*'); 
            break;
        case 'p':
            if (d_gameBoard[xCoord][yCoord].isBomb() and d_gameBoard[xCoord][yCoord].state() != 'F')
            {
                cout << "Boom! You have hit a bomb and lost the game!\n";
                showBombs();
                return false;
            }
            if (d_gameBoard[xCoord][yCoord].state() == 'F')
            {
                cout << "This coordinate is flagged, please unflag first\n";
                break;
            }
            d_gameBoard[xCoord][yCoord].setState('0' + d_gameBoard[xCoord][yCoord].numBombsNear());
            if (d_gameBoard[xCoord][yCoord].numBombsNear() != 0)
                break;
            else
                checkSurrounding(xCoord, yCoord);
            break;
        default:
            cout << "Invalid command, please try again\n";
            break;    
    }
    return true;
}

.

#include "minesweeper.ih"

ostream &operator<<(ostream &out, Minesweeper const &minesweeper)
{
    for (auto &row: minesweeper.d_gameBoard)
    {
        for (auto &element: row)
        {
            out << element.state();
        }
        out << '\n';
    }
}

Answer 1

I see a number of things that may help you improve your code.

Do not use the .ih convention

The "one function per file" rule forces a proliferation of files, each of which require some thought to name intelligently. It turns what could be a single coherent file, say minesweeper.cpp into nine different source files, meaning that a reader of the code must look at all nine files to fully understand the class. This is not at all user friendly, and is not realistically sustainable for larger projects. I'm sure Frank Brokken is a nice guy, but this particular idea of his does no favors to new programmers in my humble opinion.

Avoid explicit path names in #includes

Generally speaking, it's better to tell your compiler where to find the #include files rather than putting it explicitly into the source code like this:

#include "minesweeper/minesweeper.h"

The only reason one would add such a thing is if there were a concern that your local include files have the same name as a standard include file and that you need both simultaneously.

Be careful with signed vs. unsigned

As you've already noted in a comment, there's no point to checking to see if a size_t variable is less than zero because size_t is an unsigned type.

Fix the missing return

The ostream &operator<< function has no return statement. It's simply missing this line:

return out;

Be aware that friends are not private

In the Minesweeper class we have this declaration:

friend std::ostream &operator<<(std::ostream &out,
                Minesweeper const &minesweeper);

It's in the private section, but it's important for you to be aware that it's not actually private. It's a freestanding function that can be called from anywhere. To avoid misleading the unwary, I always put friend declarations in the public section.

Use C++ idioms

The use of or and and instead of || and && in C++ code is supported by the standard, however, it's an anachronism that stems from a time when some keyboards didn't have | or & available. While some argue that it's more readable, you should be aware that many experienced C++ programmers (and some C++ compilers!) are confused by their use. For that reason, I recommend using && and ||.

Use static const where appropriate

The SetNumBombsNear() function starts with an int moves array declaration and definition. However, it doesn't change and isn't used outside the function, so I'd recommend instead that it should be declared static const or static constexpr since you're using C++11.

Think of the user

When the game first runs, it displays ... nothing. The user can't tell if it's running or locked up or just sulking. It would be nicer to the user to list the valid command (which I understand are f, u, p and q from reading the source) and maybe to add another command, h for "help" which would describe these commands. Also, the display is rather terse and requires the user to count squares in order to enter input. This makes for a tedious game! Better would be to either label the rows and columns or provide some other way for the user to indicate the desired move. Also, there is currently no way to actually win the game because the program does not track and compare the number of flags to the number of bombs.

Make your classes useful

The current MinesweeperCell class is not very useful. It provides no intelligence and has Java-style setters and getters. I'd recommend either making it a struct that is private within the Minesweeper class and eliminating all the setter/getter code or (better), make the class actually work, while hiding implementation details. For instance, at the moment, the user can flag an already revealed cell. If they then attempt to undo that faulty move by using the u command, the square reverts to an unrevealed state. Better would be to build intelligence into the MinesweeperCell class (which I would simply name Cell by the way) to prevent flagging a revealed square in the first place.

Hide class internals

The MinesweeperCell class has isBomb() and setBomb() member functions which is good because it allows the Minesweeper class to deal with the logic of the class rather than the internal details of the Cell class. I'd suggest that the setState and state member functions (and their associated d_state member) be eliminated in favor of setFlag and isFlagged functions (with an associated d_flagged member). This would allow the Minesweeper class to deal with the logic rather than the representation of the class. Then the only place that the Cell would need to be turned into a character is for display. For that, I'd suggest writing an operator function like this:

char operator()() const { 
    return (d_visited ? (d_isBomb ? 'B' : '0' + d_numBombsNear) : (d_flagged ? 'F' : '*'));
}

Some people find the use of the ?: operator difficult to read or understand. For those people, this is functionally identical:

char operator()() const { 
    if (d_visited) {
        if (d_isBomb) {
            return 'B';
        }
        return '0' + d_numBombsNear;
    }
    if (d_flagged) {
        return 'F';
    }
    return '*';
}

Then the display routine can look like this:

ostream &operator<<(ostream &out, Minesweeper const &minesweeper) {
    for (auto &row: minesweeper.d_gameBoard) {
        for (auto &element: row) {
            out << element();
        }
        out << '\n';
    }
    return out;
}

And, for example, the showBombs() function can look like this:

void Minesweeper::showBombs() {
    for (auto &location : d_bombLocations) {
        d_gameBoard[location.first][location.second].setVisited();
    }
}

Store pointers instead of coordinates

Normally we prefer references to raw pointers, but in the case of the current d_bombLocations vector, I'd suggest that it could instead be declared like this:

std::vector<MinesweeperCell *> d_bombLocations;

Then the showBombs() routine looks like this:

void Minesweeper::showBombs() {
    for (auto &cell : d_bombLocations) {
        cell->setVisited();
    }
}

And the initializeBoard() could look like this:

void Minesweeper::initializeBoard() {
    uniform_int_distribution<int> dist(0, d_size - 1);
    default_random_engine gen(random_device{}());

    for (auto bombCount = d_size + 1; bombCount; ) {
        auto &cell = d_gameBoard[dist(gen)][dist(gen)];
        if (!cell.isBomb()) {
            --bombCount;
            cell.setBomb();
            d_bombLocations.push_back(&cell);
        }
    }
}

Note however, that if you do this, you should explicitly delete the default copy constructor:

Minesweeper(const Minesweeper &) = delete;

Name things carefully

The checkSurrounding function might be better named something like revealSurrounding to more accurately reflect what it's really doing.

Minimize work

The current constructor body for the Minesweeper class looks like this:

{
    initializeBoard();
    setNumBombsNear();
}

There isn't really much need for two separate functions, since the only actual task being accomplished is the setting of bombs. Updating the bomb counts is simplified if done as the bombs are being placed. For example:

void Minesweeper::placeBombs() {
    uniform_int_distribution<size_t> dist(0, d_size - 1);
    default_random_engine gen(random_device{}());

    for (auto bombCount = d_size + 1; bombCount; ) {
        auto x = dist(gen);
        auto y = dist(gen);
        auto &cell = d_gameBoard[x][y];
        if (!cell.isBomb()) {
            --bombCount;
            setBomb(x, y);
            d_bombLocations.push_back(&cell);
        }
    }
}

This requires a new function setBomb which not only sets a bomb, but also updates all neighboring bomb counts. I'm sure it will be easy for you to write such a function.