4
\$\begingroup\$

In my implementation I have used Cell as an object to represent each cell in the board.
I've used Flood Fill algorithm for finding the adjacent empty cells in the board, I would like my code reviewed emphasising on design and structure.

Class Cell

class Cell
{
    char state;     // keeps track of cells which are untouched and available to user.
    bool mine;      // true if cell contains a mine.

  public:
    // initialise each cell as '+', containing no mine.
    Cell() : state('+'), mine(false) {} 

    void show(bool reveal)
    {
        if (state == ' ')
            std::cout << "   ";

        else if (isMine() && reveal)
            std::cout << RED << " # ";

        else if (!isMine() && state != '+')
            std::cout << GREEN << ' ' << state
                      << ' ';

        else
            std::cout << TILE << " + ";
    }

    char getState(void) const { return state; }
    void setState(char c) { state = c; }
    bool isMine(void) const { return mine; }
    void setMine(void) { mine = true; }
};

#includes and Global objects

#include <iostream>
#include <vector>
#include <ctime>
#include <iomanip>
#include <stdexcept>

auto constexpr RED = "\x1b[31;1m";
auto constexpr GREEN = "\x1b[32;1m";
auto constexpr BLUE = "\x1b[34;1m";
auto constexpr TILE = "\x1b[30;47m";
auto constexpr RESET = "\x1b[0m";
auto constexpr CLEAR = "clear";

// should be placed after the Cell object is defined.
const int GRIDSIZE = 8;
std::vector<std::vector<Cell>> board(GRIDSIZE, std::vector<Cell>
(GRIDSIZE));

Function prototypes

// by default don't reveal mine positions to the user.
void display(bool reveal = false);

void generateMines(void);
void reveal(const unsigned int row, const unsigned int col);
char mineNear(const int i, const int j);
bool mineAt(const int row, const int col);
int getCoordinate(void);
void drawLine(void);

main()

int main()
{
    srand(static_cast<unsigned>(time(NULL)));
    generateMines();
    display();

    while (true)
    {
        std::cout << "Enter the row and column: ";
        const int row = getCoordinate();
        const int col = getCoordinate();

        if (board[row][col].isMine())
        {   
            // reveal all mine positions to the user.
            display(true);
            std::cout << "Oops! You stepped on a Mine"
                      << std::endl;
            break;
        }

        reveal(row, col);
        display();
    }
}

display()

void display(bool reveal)
{
    system(CLEAR);

    std::cout << "   ";
    for (size_t i = 0; i < board.size(); ++i)
        std::cout << std::setw(3) << i << ' ';
    std::cout << std::endl;

    for (size_t i = 0; i < board.size(); ++i)
    {
        drawLine();
        std::cout << std::setw(2) << i << " |";

        for (size_t j = 0; j < board[i].size(); ++j)
        {
            board[i][j].show(reveal);
            std::cout << RESET << '|';
        }

        std::cout << std::endl;
    }
    drawLine();
}

void drawLine(void)
{
    std::cout << "   "; 
    for (size_t i = 0; i < GRIDSIZE; ++i)
        std::cout << "----";

    std::cout << std::endl;
}

getCoordinate()

int getCoordinate(void)
{
    int val;

    while (true)
    {
        std::cin >> val;

        try
        {
            if (val >= GRIDSIZE || val < 0)
                throw std::out_of_range("Exceeded the GRIDSIZE!");
            break;
        }
        catch (std::out_of_range err)
        {
            std::cout << '\n'
                      << err.what() << '\n'
                      << "Max input: " << GRIDSIZE - 1 << '\n'
                      << "Min input: 0" << '\n'
                      << "Try again: ";
        }
    }

    return val;
}

generateMines()

void generateMines(void)
{
    for (int i = 0; i < 10; ++i)
    {
        int row, col;
        // make sure mines are not generated at the same place.
        do
        {
            row = rand() / (RAND_MAX / board.size() + 1);
            col = rand() / (RAND_MAX / board[0].size() + 1);
        } while (board[row][col].isMine());

        board[row][col].setMine();
    }
}

reveal()

void reveal(const unsigned int row, const unsigned int col)
{
    if (board[row][col].getState() == ' ')
        return;

    board[row][col].setState(mineNear(row, col));

    if (board[row][col].getState() != ' ')
        return;

    if (row != 0)
        reveal(row - 1, col);

    if (col != 0)
        reveal(row, col - 1);

    if (row < board.size() - 1)
        reveal(row + 1, col);

    if (col < board[row].size() - 1)
        reveal(row, col + 1);
}

mineNear()

char mineNear(const int row, const int col)
{
    unsigned int mines = 0;

    for (int rc = 0, i = row - 1; rc < 3; ++rc, ++i)
        for (int cc = 0, j = col - 1; cc < 3; ++cc, ++j)
            mines += mineAt(i, j);

    if (mines)
        return mines + 48;

    return ' ';
}

mineAt()

bool mineAt(const int row, const int col)
{
    return (row >= 0 && col >= 0 
            && row < static_cast<signed> (board.size()) 
            && col < static_cast<signed> (board[row].size()) 
            && board[row][col].isMine());
}

Note: This implementation does not include the option of adding flags.

\$\endgroup\$
4
\$\begingroup\$

Here are some observations that may help you improve your code.

Use a better random number generator

You are currently using

row = rand() / (RAND_MAX / board.size() + 1);

That's not bad, and it avoids most of the usual problems, but since C++11, we have had better random number generators available. In particular, instead of rand, you might want to look at std::uniform_real_distribution and friends in the <random> header.

Don't use system("clear")

There are two reasons not to use system("clear"). The first is that it is not portable to other operating systems which you may or may not care about now. The second is that it's a potential security hole, depending on which shell the user has, which you absolutely must care about. Specifically, if some program is defined and named clear, your program may execute that program instead of what you intend, and that other program could be anything. Since you're already using and relying on ANSI escape sequences, do this instead:

auto constexpr CLEAR = "\x1b[2J";

// then where you currently have system(CLEAR) use this instead:
std::cout << CLEAR;

Eliminate global variables where practical

Having routines dependent on global variables makes it that much more difficult to understand the logic and introduces many opportunities for error. See the next suggestion for how that might be done.

Use more objects

The game is written much more in the procedural style of C rather than in the object-oriented style of C++. The game itself could be an object, with most of the procedures as functions of that object. This would reduce coupling and make the program easier to understand. Also,it would also eliminate the global variables that now exist within the code. I'd suggest something like this:

class Minesweeper {
public:
    Minesweeper();
    void display(bool reveal = false) const;
    void reveal(const unsigned int row, const unsigned int col);
    int getCoordinate(void) const;
    bool mineAt(const int row, const int col) const;
private:
    void generateMines(void);
    char mineNear(const int i, const int j) const;
    void drawLine(void) const;
    static constexpr int GRIDSIZE{8};
    std::vector<std::vector<Cell>> board;
};

Use const where practical

The Cell::show() routine doesn't alter the underlying Cell object, and so it should be declared const.

Use an enum for state variables

Right now, there is no place that collects all of the different possible cell states and their corresponding tokens. I'd recommend collecting the states into an enum and then using those instead of raw characters.

Think carefully about data representation

Each Cell currently has two variables: state and mine. However, these could both reasonably be represented either by two boolean variables (e.g. revealed and mine) or by a single state variable as an enum as mentioned above.

Think of the user

It's understandable that the game doesn't currently allow the planting of flags (but I'm hoping that will come later), but it's much less understandable that there is no way to actually win the game at the moment. It would be nice if the program kept track of the number of mines and number of unrevealed squares and declared a win when the numbers were equal.

Use the appropriate data structures

The board is currently represented as a std::vector<std::vector<Cell>> but is never dynamically resized. I'd suggest using a std::array instead.

Be consistent with signed and unsigned

The current code has two similar functions:

void reveal(const unsigned int row, const unsigned int col);
bool mineAt(const int row, const int col) const;

Why are row and col unsigned in one instance and signed in the other? Pick one (I'd recommend unsigned) and use it consistently.

Don't use exceptions for unexceptional events

It's not at all unexpected for the user to enter a wrong number that's outside the grid space, so using an exception within getCoordinate is not a good idea. Instead, a simple validation loop would be more appropriate:

int Minesweeper::getCoordinate(void) const
{
    int val;
    for (std::cin >> val; val < 0 || val >= GRIDSIZE; std::cin >> val) {
    std::cout << "\nExceeded the GRIDSIZE!\n"
              "\nMax input: " << GRIDSIZE - 1 << 
              "\nMin input: 0"
              "\nTry again: ";
    }
    return val;
}

Reworked version

Here's a reworked version of the original with all of these things implemented and more.

#include <iostream>
#include <iomanip>
#include <array>
#include <random>

// simple random int generator derived from Stroustrup: 
// http://www.stroustrup.com/C++11FAQ.html#std-random
int rand_int(int low, int high)
{
    static std::default_random_engine re {std::random_device{}()};
    using Dist = std::uniform_int_distribution<int>;
    static Dist uid {};
    return uid(re, Dist::param_type{low,high});
}


class Minesweeper {
public:
    Minesweeper();
    void display(bool reveal = false) const;
    void reveal(const unsigned row, const unsigned col);
    bool hasWon() const { return numHidden == numMines; }
    unsigned getCoordinate(bool wantCol) const;
    bool mineAt(const unsigned row, const unsigned col) const {
        return inBounds(row, col) && board[row][col].isMine();
    }

private:
    class Cell {
    public:
        void show(bool reveal) const
        {
            if (isMine() && reveal)
                std::cout << RED << ' ' << state << ' ';
            else if (isRevealed())
                std::cout << GREEN << ' ' << state << ' ';
            else
                std::cout << TILE << ' ' << '+' << ' ';
        }
        bool setCount(unsigned cnt) { state = cnt ? '0'+cnt : ' '; return cnt==0; }
        bool isRevealed(void) const { return state!='#' && state!='+'; }
        bool isMine(void) const { return state=='#'; }
        // return true if this is an existing mine
        bool setMine(void) { bool ret=isMine(); state = '#'; return ret; }
    private:
        char state{'+'};
    };
    bool inBounds(const unsigned row, const unsigned col) const {
        return row < height && col < width; 
    }
    unsigned mineNear(const unsigned i, const unsigned j) const;

    static constexpr auto RED = "\x1b[31;1m";
    static constexpr auto GREEN = "\x1b[32;1m";
    static constexpr auto BLUE = "\x1b[34;1m";
    static constexpr auto TILE = "\x1b[30;47m";
    static constexpr auto RESET = "\x1b[0m";
    static constexpr auto CLEAR = "\x1b[2J";
    static constexpr unsigned width{10};
    static constexpr unsigned height{8};

    std::array<std::array<Cell, width>, height> board;
    unsigned numHidden;
    unsigned numMines;
};

Minesweeper::Minesweeper() :
    board{},
    numHidden{width*height},
    numMines{numHidden/6}  // 1 of 6 is a mine
{
    for (unsigned i = 0; i < numMines; ++i) {
        while (board[rand_int(0, height-1)][rand_int(0, width-1)].setMine()) {
            // continue until a new mine is set
        }
    }
}

void Minesweeper::display(bool reveal) const {
    static const std::string line(4*width, '-');
    std::cout << CLEAR << "   ";
    for (size_t j = 0; j < width; ++j)
        std::cout << std::setw(3) << j << ' ';
    std::cout << "\n";
    for (size_t i = 0; i < height ; ++i) {
        std::cout << "   " << line << '\n'
                  << std::setw(2) << i << " |";
        for (size_t j = 0; j < width; ++j) {
            board[i][j].show(reveal);
            std::cout << RESET << '|';
        }
        std::cout << "\n";
    }
    std::cout << "   " << line << '\n';
}

unsigned Minesweeper::getCoordinate(bool wantCol) const {
    unsigned val;
    unsigned limit = wantCol ? width : height;
    for (std::cin >> val; val >= limit; std::cin >> val) {
        std::cout << "\nError: please enter a number in the range 0 through " << limit - 1 << "\n";
    }
    return val;
}

void Minesweeper::reveal(const unsigned row, const unsigned col) {
    if (!inBounds(row, col) || board[row][col].isRevealed())
        return;
    --numHidden;
    if (board[row][col].setCount(mineNear(row, col))) {
        reveal(row - 1, col);
        reveal(row, col - 1);
        reveal(row + 1, col);
        reveal(row, col + 1);
    }
}

unsigned Minesweeper::mineNear(const unsigned row, const unsigned col) const {
    static constexpr std::array<int,3> offset{ -1, 0, +1 };
    unsigned mines = 0;
    for (const auto i : offset) {
        for (const auto j : offset) {
            mines += mineAt(row + i, col + j);
        }
    }
    return mines;
}

int main()
{
    Minesweeper ms;
    ms.display();

    for (bool hasLost = false; !ms.hasWon() && !hasLost; ms.display(hasLost)) {
        std::cout << "Enter the row and column: ";
        const unsigned row = ms.getCoordinate(false);
        const unsigned col = ms.getCoordinate(true);

        hasLost = ms.mineAt(row, col);
        if (!hasLost) {
            ms.reveal(row, col);
        }
    }
    if (ms.hasWon()) {
        std::cout << "Congratulations! You have won!\n";
    } else {
        std::cout << "Oops! You stepped on a Mine\n";
    }
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you for the review! The reason for using signed and unsigned integers is because of comparison that takes place in each routine, in case of mineAt the row and col can potentially be lesser than zero, hence signed. \$\endgroup\$ – CaptainDaVinci Mar 26 '17 at 5:04
  • \$\begingroup\$ If you reimplement as unsigned you can eliminate any checks for negative numbers. \$\endgroup\$ – Edward Mar 26 '17 at 10:37
  • \$\begingroup\$ But there is a possibility of a negative integer being passed as the arguments to the mineAt function, if the parameters row and col are unsigned, wouldn't this result in a very large value being stored in row and col as the conversion would be machine dependent? \$\endgroup\$ – CaptainDaVinci Mar 26 '17 at 11:15
  • \$\begingroup\$ If every number is unsigned, there is no way to pass a negative number and thus no need to check for one. A single check against the upper bound is sufficient to catch any numeric wraparound. I've updated my answer to show the reworked version in its entirety. \$\endgroup\$ – Edward Mar 26 '17 at 15:00
1
\$\begingroup\$

This review deals mostly with coding style issues. Please see it as a supplementary answer only, since it doesn't deal with either design or structure.

Still, I think I have found some points that should be considered by you, so here are my two cents to your code.


Readability

for (int rc = 0, i = row - 1; rc < 3; ++rc, ++i)
    for (int cc = 0, j = col - 1; cc < 3; ++cc, ++j)
        mines += mineAt(i, j);

The two nested for-loops are quite hard to read. I advise splitting these loop and renaming rc and cc, since their meaning is not clear to me.

Magic numbers

if (mines)
    return mines + 48;

Wait, what? What does this 48 mean? You should avoid magic numbers as much as possible. In this case, replacing 48 with '0' would make the code much more understandable.

Casting

static_cast<signed>(...);

Using signed in a cast is unusual (although not wrong). Usually, you'd want to use int here instead (refer to this question for more information).

Exceptions/try-catch

try
{
    if (val >= GRIDSIZE || val < 0)
        throw std::out_of_range("Exceeded the GRIDSIZE!");
    break;
}
catch (std::out_of_range err)
{
    std::cout << '\n'
              << err.what() << '\n'
              << "Max input: " << GRIDSIZE - 1 << '\n'
              << "Min input: 0" << '\n'
              << "Try again: ";
}

This really seems unnecessary to me. Why would throw an exception and accept the incuring overhead when you could just print your error message right there? Use exceptions only when they're really useful (i.e. when you can't recover from an error in situ or have no other means to return error information).

In the same piece of code: Why do you split '\n' off from your string literals? Instead of "Min input: 0" << '\n', "Min input: 0\n" would easily be enough and would save you a call to operator<<(...).

Consistency

Although this is just a really small point, you should stay consistent in how you express certain things. For example, in reveal() you wrote if (row != 0) while in mineNear() one can find the line if (mines). IMHO, both ways of writing this are fine, but you should decide for one and stay with it.

The case against std::endl

It's simple: Unless you really know what you are doing, don't use std::endl. Why? Because it does not only insert a linebreak, but also flushes the output buffer. This can harm performance quite a lot if you are writing a lot.

| improve this answer | |
\$\endgroup\$

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.