Recreating Minesweeper

Question

First, I have to say that this game lacks 5 features that I don't care:

• Flags
• Chording
• Timer
• Actual mouse movement
• Good graphics

When starting the program, the player is meeted with a "Choose difficulty" menu. They will move around by pressing ASWD, choose an option by pressing enter/space, and quit at anytime by pressing ESC. After that, the board is shown, and they play the game until winning, losing or quitting. When won/lost, they're asked to either play again with this difficulty, choose a new one at the start menu, or quit.

These are what I made to implement them

#include<iostream>
#include<string>
#include<conio.h>   // controls and cls
#include<algorithm> // shuffle
#include <random>   // rands for shuffle
#include <chrono>   // seeds for rand
#define cls system("cls")
typedef unsigned short int us;
using namespace std;

bool cellnum[80][40];   // checks if the cell has bomb

string board,shownboard;
// both store info to cout the mine field, board is for modifying the cells, shownboard copies board and add cursor

us z,numcell;           // z is num of bombs on the field, numcell is num of unrevealed non-bomb cells on the field
char p2;                // player input
struct coord{us x,y;}   // coordinates
       xy;              // this is the board/field size

void quit(){            // endscreen
    cls;
    cout<<(char)-55<<string(60,-51)<<(char)-69<<endl
        <<(char)-70<<string(19,' ')<<"Thank you for playing!"<<string(19,' ')<<(char)-70<<endl
        <<(char)-56<<string(60,-51)<<(char)-68<<endl;
    exit(0);
}

class cursor{                       // handles true coord of cursor, cursor shown on screen (dot/arrow) is based on this
    public:
        coord board,now;
// now: coordinate of cursor, board: limits of cursor's coordinates
// now<board in both x and y

        void update(){              // when player tries to move around
            switch(p2){
                case 'a':case 'A':if(now.x)now.x--;return;
                case 'w':case 'W':if(now.y)now.y--;return;
                case 'd':case 'D':if(now.x<board.x-1)now.x++;return;
                case 's':case 'S':if(now.y<board.y-1)now.y++;return;
                case 27:{quit();}   // for the "Press ESC at anytime to quit."
            }
        }
        us code(){return now.x+now.y*board.x;}      // some functions use 1 var, this is for those functions
}g_sor,                 // Game cursor
 p_sor={{2,2},{0,0}},   // Choose-your-board cursor
 s_sor={{5,1},{0,0}};   // Start menu cursor

us &cx=g_sor.now.x,&cy=g_sor.now.y; // g_sor.now.x is too long, so I added references with shorter names

bool enter(){                       // for the "choose by pressing SPACE/ENTER"
    p2=getch();
    return (p2==' '||p2=='\r');
}

// because board/shownboard is 1D array, there should be a function translating 2D coordinates into those 2.
void modi(us &x,us &y,char c,bool m=1){     // bool m is to know which to be modified
    if(m)shownboard[(x+y*xy.x)*2]=c;
    else board[(x+y*xy.x)*2]=c;
}

// function for revealing a non-bomb cell and (possibly) its neighbors
void show(coord c){
    if(board[(c.x+c.y*xy.x)*2]!=-2)return;  // check if it's revealed

    char i='0';
    numcell--;
// check if cell is at any of the left-right-up-down border
    bool le= c.x,
         ri=(c.x)!=xy.x-1,
         up= c.y,
         dn=(c.y)!=xy.y-1;

// check if cell's neighbors are bombs
    if(le){
        if(up)i+=(cellnum[c.x-1][c.y-1]==1);
        if(dn)i+=(cellnum[c.x-1][c.y+1]==1);
        i+=(cellnum[c.x-1][c.y]==1);
    }
    if(ri){
        if(up)i+=(cellnum[c.x+1][c.y-1]==1);
        if(dn)i+=(cellnum[c.x+1][c.y+1]==1);
        i+=(cellnum[c.x+1][c.y]==1);
    }
        if(up)i+=(cellnum[c.x][c.y-1]==1);
        if(dn)i+=(cellnum[c.x][c.y+1]==1);

    if(i!='0'){modi(c.x,c.y,i,0);return;}

// cell has 0 neighboring bombs => reveals its neighbors
    modi(c.x,c.y,' ',0);
    if(le){
        show({us(c.x-1),c.y});
        if(up)show({us(c.x-1),us(c.y-1)});
        if(dn)show({us(c.x-1),us(c.y+1)});
    }
    if(ri){
        show({us(c.x+1),c.y});
        if(up)show({us(c.x+1),us(c.y-1)});
        if(dn)show({us(c.x+1),us(c.y+1)});
    }
        if(up)show({c.x,us(c.y-1)});
        if(dn)show({c.x,us(c.y+1)});
}

int main(){
cls;
    cout<<"Move around by using ASWD. Choose by pressing ENTER/SPACE. Quit by pressing ESC."<<endl
        <<"Beginner (10x10, 8 bombs)    Intermediate (13x15, 40 bombs)\n"
        <<((!p_sor.now.y)?(((p_sor.now.x)?string(44,' '):"           ")+(char)24+'\n'):"\n")    // check if the arrow
        <<"Expert (16x30, 100 bombs)    Custom (Whatever you want man)\n"                       // should be on above
        <<(( p_sor.now.y)?(((p_sor.now.x)?string(44,' '):"           ")+(char)24+'\n'):"\n");   // or below

// player moves around
    if(!enter()){
        p_sor.update();main();
        return 0;
    }

// player picks a board
    switch(p_sor.code()){
        case 0:xy={10,10};z=8  ;break;
        case 1:xy={15,13};z=40 ;break;
        case 2:xy={30,16};z=100;break;
        default:
            while(1){                           // while loop ends when player's input is valid
                cout<<"Enter the width and height of the board, and how many bombs you want: ";
                cin>>xy.x>>xy.y>>z;

                if(cin.fail()){
                    cin.clear();cin.ignore();
                    cin.clear();cin.ignore();
                    cin.clear();cin.ignore();
                    cout<<"Invalid. Those are not numbers.\n\n";
                    continue;
                }

                if(!(xy.x&&xy.y)){
                    cout<<"Invalid. Width and height must be atleast 1.\n\n";
                    continue;
                }

                if(z>xy.x*xy.y){
                    cout<<"Invalid. Too many bombs, the board explodes.\n\n";
                    continue;
                }

                if(max(xy.x,xy.y)>80||min(xy.x,xy.y)>40){       // Board with width 82 or height 42 has bad alignment
                    cout<<"Invalid. The width and height are capped at 80 and 40.\n\n";
                    continue;
                }

// Input is valid, but to make a nice board (imo) x shouldn't be less than y
                if(xy.x<xy.y)swap(xy.x,xy.y);
                break;
        }
    }

    retry:              // for retrying at this difficulty
    cout<<"\nYou are going to mienswipe a "<<xy.x<<'x'<<xy.y<<" board with "
        <<z<<" bomb"<<((z==1)?".":"s.")
        <<"\nPress anything to continue...\n";
    if(getch()==27)quit();

// setting up the board and bombs
    numcell=xy.x*xy.y;
    g_sor.board=xy;
    g_sor.now={(us)(xy.x/2),(us)(xy.y/2)};

// picking random coordinates for z+1 bombs. last bomb is for when player unluckily reveals a bomb 1st try
    us bomb[numcell];for(us i=0;i<numcell;i++)bomb[i]=i;
    unsigned seed = chrono::system_clock::now().time_since_epoch().count();
    shuffle (&bomb[0],&bomb[numcell], default_random_engine(seed));
    for(us i=0;i<z;i++)cellnum[bomb[i]%xy.x][bomb[i]/xy.x]=1;

    for(us i=0;i<xy.y;i++){
        for(us j=0;j<xy.x-1;j++){board+=(char)254;board+=' ';}
        board+=(char)254;board+='\n';
    }
    numcell-=z;
// setup completes

// loops until all non-bomb cells are revealed, or a bomb is revealed
    while(numcell){
        shownboard=board;modi(cx,cy,-7);    // shownboard copies board, add a dot as the screen's cursor
        cls;cout<<shownboard;
        if(enter()){
            if(cellnum[cx][cy]==1){
                if(xy.x*xy.y-z==numcell){   // if player unluckily picks a bomb 1st try
                    cellnum[cx][cy]=0;
                    cellnum[bomb[z]%xy.x][bomb[z]/xy.x]=1;
                    show(g_sor.now);
                    continue;
                }
                for(us i=0;i<z;i++)
                    board[(bomb[i]%xy.x+bomb[i]/xy.x*xy.x)*2]='#';
                modi(cx,cy,'x',0);
                break;
            }
            show(g_sor.now);
        }
        else g_sor.update();
    }

//game ends. cout game over message and reset some global variables
    cls;
    cout<<board
        <<((numcell)?"\nYou got swiped and lose! :(":"\nCongratulation, you won!")
        <<"\nType \"MENU\"  to go back to menu,\n"
        <<"     \"RETRY\" to try this difficulty again,\n"
        <<"  or \"QUIT\"   to quit.\n";
    board={};
    for(us i=0;i<=z;i++)cellnum[bomb[i]%xy.x][bomb[i]/xy.x]=0;
    while(1){
        cin>>shownboard;
        if(shownboard=="MENU" ||shownboard=="Menu" ||shownboard=="menu" ){main();exit(0);}
        if(shownboard=="RETRY"||shownboard=="Retry"||shownboard=="retry"){goto retry;exit(0);}
        if(shownboard=="QUIT" ||shownboard=="Quit" ||shownboard=="quit")  quit();
    }
}

Answer 1

Overall Impression

This code is not maintainable in its current state. Good code should always be maintainable.

Code Readability

The code is very difficult to read. This line of code in main():

    us bomb[numcell];for(us i=0;i<numcell;i++)bomb[i]=i;

should actually be 2 lines of code and there should be spaces between all the operators and operands

    us bomb[numcell];
    for (us i = 0; i < numcell; i++) bomb[i] = i;

Use C++ Container Classes

Since this is C++ and not the C programming language, container classes such as std::vector or std::array should be preferred over C style arrays such as

bool cellnum[80][40];

us bomb[numcell];

Flow of Control

The comments about the control flow below refer to this section of code:

    while (1) {
        cin >> shownboard;
        if (shownboard == "MENU" || shownboard == "Menu" || shownboard == "menu") { main(); exit(0); }
        if (shownboard == "RETRY" || shownboard == "Retry" || shownboard == "retry") { goto retry; exit(0); }
        if (shownboard == "QUIT" || shownboard == "Quit" || shownboard == "quit")  quit();
    }

In the C programming language it is okay to have goto statements for error handling, but in the C++ programming language which has exceptions there is no really good reason to ever use a goto statement.

The main() function should not be called recursively.

This program should not need to call the exit() function, especially from main().

The main() function needs to be broken up into more functions.

Prefer Braces { and } Around Single Statements in if or loops

Some programmers consider this a style issue, but it makes it much easier to read and maintain the code if each in an if, else or loop block is embedded within braces. Extending the functionality of these statements can be problematic when the braces are not used. For a more in depth discussion of this see the first 2 answers on this Stack Overflow question. As one of the answers points out this is true in all C like languages (C, C++, C#, JavaScript, Java, etc.). I have worked at multiple companies where this was required in the coding standard and flagged during code reviews.

Avoid Global Variables

It is very difficult to read, write, debug and maintain programs that use global variables. Global variables can be modified by any function within the program and therefore require each function to be examined before making changes in the code. In C and C++ global variables impact the namespace and they can cause linking errors if they are defined in multiple files. The answers in this stackoverflow question provide a fuller explanation.

Avoid using namespace std;

If you are coding professionally you probably should get out of the habit of using the using namespace std; directive. The code will more clearly define where cout and other identifiers are coming from (std::cin, std::cout). As you start using namespaces in your code it is better to identify where each function comes from because there may be function name collisions from different namespaces. The identifiercout you may override within your own classes, and you may override the operator << in your own classes as well. This stack overflow question discusses this in more detail.

Answer 2

Don't use system("cls")

Clearing the screen this way is very inefficient: it creates a new process running cmd.exe, which in turn has to interpret the string cls, and only then will it clear the screen. Consider then that cmd.exe is written in C++: there should therefore be a way to clear the screen from a C++ program that doesn't involve starting cmd.exe.

There are more issues with this: depending on the exact configuration of your system, system("cls") might actually do something different than clearing the screen. And using a macro is unnecessary, you could have created a regular function that calls system("cls").

Since you are including "conio.h", you can just call clrscr().

Answer 3

A couple of points:

clr macro

• Hiding functionality behind a macro is not great because it doesn't look like a method, but behaves like one.

cls;

This should be a method, then it's clearer:

void Cls() { system("cls"); }

Which then means the suggestion to replace the system call with clrstr doesnt need every use of cls; changing, but one line in the method:

void Cls() { clrstr(); }

'Unusual' characters

cout<<(char)-55<<string(60,-51)<<(char)-69<<endl

I presume -55 is ASCII character 200 which is some kind of border? I would define these as macros, so it's easier to work out what the characters actually are:

#define TopRightBorder (char)-55;

#define MiddleHorizontalBorder (char)-51

#define TopLeft (char)-69

cout<<TopRightBorder<<string(60,HorizontalBar)<<TopLeftBorder<<endl etc

Or how about encoding your file as UTF8, then you can include the graphics directly (without having to worry about ASCII encoding):

#define TopRightBorder '╔';

#define MiddleHorizontalBorder '═'

#define TopLeft '╗'

Answer 4

There are already some good reviews from the C++ point of view. So let me give suggestions from a software architecture point of view.

The code scores an absolute zero from an architecture point of view. There is no separation between UI code, Board and GameLogic. We will have to start afresh. Since you are a beginner, I took some time out and rewrote your code entirely. You can find it here: https://godbolt.org/z/W7rMK553j.

There are 5 important things in my code:

1. class Board : Responsible for representing the board
2. class Game : Responsible for maintaining and modifying current board state.
3. class BoardView : Responsible for UI when the game is running
4. class MainMenuView : Responsible for UI when the main menu is displayed.
5. run_game(): This is the glue that connects BoardView, Game and Board classes.

Board

To recreate a minesweeper board we need 2 pieces of information: [1] The size of the board (length and width) [2] A list of cells with mines. The board class stores these pieces of information. It also provides some convenience functions like get_neighbors().

Game

This is where the game logic and progress is stored. In minesweeper, the player has to progressively reveal different cells. If any of the revealed cells has a mine, the player loses. If all the cells without mines are revealed, the player wins. This means we can entirely boil the game into a list of revealed cells and an algorithm that reveals tiles based on player choice and board layout.

BoardView

This is the UI layer when the game is running. It is a relatively dumb class and knows nothing about the game logic. It only knows how to display a board. It is also responsible for maintaining and moving the cursor; and checking if the player wants to reveal a cell.

MainMenuView

This is the UI layer for the Game. It displays the main menu and based on user selection either starts the game or quits.

With the new architecture, it becomes very easy to read and understand the code. It is also easy to improve graphics as the game logic and UI are completely separated. With minor modifications to the Architecture, you can also add GUI and mouse support to the game. With a standalone Game class, adding support for things like chording and flags is easy too. I have added logic for chording. You can try adding logic for flags.