patterncppMinor
C++: Minesweeper Game v2
Viewed 0 times
gameminesweeperstackoverflow
Problem
In my implementation I have used
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
#includes and Global objects
Function prototypes
main()
```
int main()
{
srand(static_cast(time(NULL)));
generateMines();
display();
while (true)
{
std::cout << "Enter the row and column: ";
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
#include
#include
#include
#include
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> board(GRIDSIZE, std::vector
(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(time(NULL)));
generateMines();
display();
while (true)
{
std::cout << "Enter the row and column: ";
Solution
Here are some observations that may help you improve your code.
Use a better random number generator
You are currently using
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
#include
#include
#include
#include
// 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;
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, con
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 ` header.
Don't use system("clear")
There are two reasons not to use . 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> 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> 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 = 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
#include
#include
#include
// 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;
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, con
Code Snippets
row = rand() / (RAND_MAX / board.size() + 1);auto constexpr CLEAR = "\x1b[2J";
// then where you currently have system(CLEAR) use this instead:
std::cout << CLEAR;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;
};void reveal(const unsigned int row, const unsigned int col);
bool mineAt(const int row, const int col) const;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;
}Context
StackExchange Code Review Q#158810, answer score: 4
Revisions (0)
No revisions yet.