7
\$\begingroup\$

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';
    }
}
\$\endgroup\$
  • \$\begingroup\$ I just noticed that I am testing size_t's if they are negative, which is of course nonsense. This will be fixed in the GitHub version, but I will leave it intact here. \$\endgroup\$ – Nigel Overmars Mar 27 '17 at 23:37
  • \$\begingroup\$ And I was inspired by codereview.stackexchange.com/questions/158795/…, but didn't look at the answers before posting this question. So there might be some similarities. \$\endgroup\$ – Nigel Overmars Mar 27 '17 at 23:44
  • \$\begingroup\$ It is very controversial to use std::size_t for a replacement of nonnegative integers anyway, since they do not overflow. An alternative is a conditional value type nonnegative<int>. \$\endgroup\$ – Maikel Mar 28 '17 at 8:04
7
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you for your input! Especially the part about storing pointers to the Cells instead of coordinates is clever and I will certainly remember it. I do have some further questions/remarks, in order: (1) (Full disclosure: I followed Frank's C++ course and used his book, "C++ Annotations") This is probably a matter of preference. I use the function name as file name and I prefer to have many small files (possibly next to each other) than have to look through one big file. (2) Makes sense, will do in the future. (3) (4) Fixed! \$\endgroup\$ – Nigel Overmars Mar 28 '17 at 17:46
  • \$\begingroup\$ (5) I was aware of this fact and this should be obvious from the implementation function signature (e.g. no Minesweeper::(..) ) (6) Fair enough, but I prefer a more verbose C++. It should be clear what it means. (7) I agree! (8) Again agree, I was probably just happy that it worked for me that I didn't consider a users perspective. I will definitely add a help command and a way to win! (9) I thought about making it a struct, but your solution sounds better. Will look at it. \$\endgroup\$ – Nigel Overmars Mar 28 '17 at 17:46
  • \$\begingroup\$ (10) I am always a bit sceptic about overloading operator(), since it almost never provides any information on what is does without knowing the implementation. But your underlying point makes sense. Again, I will reconsider my current approach. (11) I fully agree! Copy constructing or move constructing a Minesweeper object is weird anyhow, so I will probably delete them anyway. (12) True, but revealSurrounding is also not perfect. I will think about a better name! (13) I agree, will be adjusted in a future version. \$\endgroup\$ – Nigel Overmars Mar 28 '17 at 17:46
  • 1
    \$\begingroup\$ @NigelOvermars Regarding (6), I really hope you reconsider. If you want to ever work on anything other than a solo project, you should be following any widely held conventions. In over 15 years of C++ dev I have never seen any projects use the 'and' and 'or' operators over && and ||. \$\endgroup\$ – theosza Mar 30 '17 at 8:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.