Minesweeper C++

This is my improvement of a minesweeper game I took from the internet, which I am quite proud of. Nevertheless I am willing to hear your opinions, suggestions, and comments.

```
/* Minesweeper V2.0
Written by:-Shivam Shekhar
Improved and understood by Eshel BM*/

#include
#include
#include
#include

//read only variables - can be changed.
#define NUM_OF_BOMBS 60 //HAS TO BE LESS THAN 100 AND LESS THAN HEIGHT * WIDTH
#define HEIGHT 13 //the amount of actual "buttons" in the height of the grid
#define WIDTH 25 //the amount of actual "buttons" in the width of the grid
#define OFFSET 3 //how far away is the minefield from the sides of the screen
#define OFFSET_FLAG_W 20 //how far away is the flags counter from the left of the screen
#define OFFSET_FLAG_H 1 //how far away is the flags counter from the top of the screen
#define BUTTON_CH 219 //this is the ascii value of a blank square - character: '█'
#define BOMB_CH 15 //'☼'

//No change allowed
#define SIZE SIDE_H * SIDE_W
#define SIDE_W (WIDTH * 2 - 1) //the horizontal side of the grid of the minefield (buttons + space between buttons)
#define SIDE_H (HEIGHT * 2 - 1) //the vertical side of the grid of the minefield (buttons + space between buttons)
#define FLAGS_DISP_SIZE 4

//colors (b= background, f=foregruond):
#define GRAY_F FOREGROUND_BLUE | FOREGROUND_RED | FOREGROUND_GREEN
#define WHITE_F GRAY_F | FOREGROUND_INTENSITY
#define GRAY_B BACKGROUND_BLUE | BACKGROUND_RED | BACKGROUND_GREEN
#define WHITE_B GRAY_B | BACKGROUND_INTENSITY

//function declaration:
int menu(HANDLE out, HANDLE in, bool print);
void instructions();
void updateFlagsDisp(CHAR_INFO * flags, int flagsLeft, bool initColor);
void initFields(CHAR_INFO mines, CHAR_INFO map);
bool validClick(COORD clickLocation, CHAR_INFO * field);
void dropMines(CHAR_INFO * map, COORD firstClick);
void fillMap(CHAR_INFO * map);
void reveal(COORD clickLocation, CHAR_INFO *

C++ Vs C

As has been said in the comments, your code reads very much like it's been written in C. You're using C functions like printf instead of the more C++ cout and you haven't broken your logic down into classes etc.

Commented out Code

Commented out code creates noise that distracts from the rest of the code. If you want to have conditional compilation, it's better to build it in to your process. So, instead of:

//To see map, uncomment next 5 lines:
/*fieldsRect.Left += SIDE_W + OFFSET;
fieldsRect.Right += SIDE_W + OFFSET;
WriteConsoleOutput(out,map,fieldsGrid,origin,&fieldsRect);
fieldsRect.Left -= SIDE_W + OFFSET;
fieldsRect.Right -= SIDE_W + OFFSET;*/

You could have:

#ifdef SHOW_MAP
    fieldsRect.Left += SIDE_W + OFFSET;
    fieldsRect.Right += SIDE_W + OFFSET;
    WriteConsoleOutput(out,map,fieldsGrid,origin,&fieldsRect);
    fieldsRect.Left -= SIDE_W + OFFSET;
    fieldsRect.Right -= SIDE_W + OFFSET;
#endif

Function length

Your main is about 180 lines long. This is quite long, and whilst it's not always the case, there is likely to be some aspects of it that could be broken up into separate functions to make the intent and flow of the code clearer. Generally speaking, if I get a method that is much more than a screen full of code (80 lines) I start considering if the code needs broken up more.

labels

I'm not totally against labels, but if you need to use them, then it can be a good indicator as to where there is a distinct responsibility in your code. For example, you have the label start:. You jump to this if the player says they want to play again. If the code had been broken up a bit, this label wouldn't have been necessary:

do {
    playGame();
}
while(userWantsToPlayAgain());

map

map is a collection in the STL, so I would tend to avoid using it as a variable name. I'd also consider using a two dimensional array to represent the play area, rather than a 1 dimensional array. That way instead of doing the hard work yourself:

minefield[i + j * SIDE_W].Char.AsciiChar = ' ';

You can get the compiler to help you out:

minefield[i][j].Char.AsciiChar = ' ';

One letter variables

One letter iterators are ok, however when you start getting multiple iterators in the same method it may be worth giving them proper names so that it's more obvious what's going on. This is far from transparent:

for (k = j - 1; k = 0 && 0 <= k && l < WIDTH && k < HEIGHT)
        {
            minesSurrounding++;
        }
    }
}

Next steps

Consider allowing the user to set the difficulty (and varying the number of mines in the minefield accordingly), rather than having it hard coded.

StackExchange Code Review Q#134637, answer score: 15