Skip to main content
Code Review

Return to Answer

Typos etc
Source Link
Toby Speight
  • 87.3k
  • 14
  • 104
  • 322

  1. Don't use #define for constants: This:

    //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

    The first step here is to realize that we now have true constants in C++ without using the preprocessor. Why bother? Well the preprocessor doesn't follow the rules of C++ always. For more detail see this: https://stackoverflow.com/questions/42388077/constexpr-vs-macros

    Anyway your code then becomes this:

    //read only variables - can be changed.
constexpr auto num_of_bombs{ 60 }; //HAS TO BE LESS THAN 100 AND LESS THAN HEIGHT * WIDTH
constexpr auto height{ 13 }; //the amount of actual "buttons" in the height of the grid
constexpr auto width{ 25 }; //the amount of actual "buttons" in the width of the grid
constexpr auto offset{ 3 }; //how far away is the minefield from the sides of the screen
constexpr auto offset_flag_w{ 20 }; //how far away is the flags counter from the left of the screen
constexpr auto offset_flag_h{ 1 }; //how far away is the flags counter from the top of the screen
constexpr auto button_ch{ 219 }; //this is the ascii value of a blank square - character: '█'
constexpr auto bomb_ch{ 15 }; //'☼'
//No change allowed
constexpr auto side_w(width * 2 - 1); //the horizontal side of the grid of the minefield (buttons + space between buttons)
constexpr auto side_h(height * 2 - 1); //the vertical side of the grid of the minefield (buttons + space between buttons)
constexpr auto size{ side_h * side_w };
constexpr auto flags_disp_size{ 4 };
//colors (b= background, f=foregruond):
constexpr auto gray_f{ FOREGROUND_BLUE | FOREGROUND_RED | FOREGROUND_GREEN };
constexpr auto white_f{ gray_f | FOREGROUND_INTENSITY };
constexpr auto gray_b{ BACKGROUND_BLUE | BACKGROUND_RED | BACKGROUND_GREEN };
constexpr auto white_b{ gray_b | BACKGROUND_INTENSITY };

  2. State intendintent directly in Code. For this only one example. Consider Thisthis code we have now:

    constexpr auto button_ch{ 219 }; //this is the ascii value of a blank square - character: '█'
constexpr auto bomb_ch{ 15 }; //'☼'

    What does 219 mean? You said it in the comment. It would be better to say it in the code like this:

    constexpr auto button_ch{ '█' };
constexpr auto bomb_ch{ '☼' };

    Now nobody needs to read the comments anymoreany more. Unfortunately these signs are not portable so iI switched them to some standard signs on the keyboard:

    constexpr auto button_ch{ '#' };
constexpr auto bomb_ch{ '*' };

  3. Limit your line width. Why? To open two source files next to each other without scrolling. Also it stresses the eye to scroll to the right aswellas well. For example this:

    int width = offset_flag_w + flags_disp_size + 1 > side_w + offset * 2 ? offset_flag_w + flags_disp_size + 1 : side_w + offset * 2;

    Isn't that better:

    int width = offset_flag_w + flags_disp_size + 1 > side_w + offset * 2 ? 
 offset_flag_w + flags_disp_size + 1 : 
 side_w + offset * 2;

    There are tools whothat can show you how long a line can be. Common limits are 80 or 100 chars. I personally stick to 80. I think it also forces you to write cleaner code.

  4. Limit your function size. You already made functionsfunctions; good. The bad part is they are still too long. For example your main function. Is it easy to follow how the program flows there? I think not. There are many nasty details to follow the function. You should consider hiding them into their own small functions. I would break them into many small functions who bestwhich each only do one thing. Generally a function should fit on one screen without scrolling. Otherwise the function is probably too big and does too many tasks.

  1. Don't use #define for constants: This:

    //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

    The first step here is to realize that we now have true constants in C++ without using the preprocessor. Why bother? Well the preprocessor doesn't follow the rules of C++ always. For more detail see this: https://stackoverflow.com/questions/42388077/constexpr-vs-macros

    Anyway your code then becomes this:

    //read only variables - can be changed.
constexpr auto num_of_bombs{ 60 }; //HAS TO BE LESS THAN 100 AND LESS THAN HEIGHT * WIDTH
constexpr auto height{ 13 }; //the amount of actual "buttons" in the height of the grid
constexpr auto width{ 25 }; //the amount of actual "buttons" in the width of the grid
constexpr auto offset{ 3 }; //how far away is the minefield from the sides of the screen
constexpr auto offset_flag_w{ 20 }; //how far away is the flags counter from the left of the screen
constexpr auto offset_flag_h{ 1 }; //how far away is the flags counter from the top of the screen
constexpr auto button_ch{ 219 }; //this is the ascii value of a blank square - character: '█'
constexpr auto bomb_ch{ 15 }; //'☼'
//No change allowed
constexpr auto side_w(width * 2 - 1); //the horizontal side of the grid of the minefield (buttons + space between buttons)
constexpr auto side_h(height * 2 - 1); //the vertical side of the grid of the minefield (buttons + space between buttons)
constexpr auto size{ side_h * side_w };
constexpr auto flags_disp_size{ 4 };
//colors (b= background, f=foregruond):
constexpr auto gray_f{ FOREGROUND_BLUE | FOREGROUND_RED | FOREGROUND_GREEN };
constexpr auto white_f{ gray_f | FOREGROUND_INTENSITY };
constexpr auto gray_b{ BACKGROUND_BLUE | BACKGROUND_RED | BACKGROUND_GREEN };
constexpr auto white_b{ gray_b | BACKGROUND_INTENSITY };

  2. State intend directly in Code. For this only one example. Consider This code we have now:

    constexpr auto button_ch{ 219 }; //this is the ascii value of a blank square - character: '█'
constexpr auto bomb_ch{ 15 }; //'☼'

    What does 219 mean? You said it in the comment. It would be better to say it in the code like this:

    constexpr auto button_ch{ '█' };
constexpr auto bomb_ch{ '☼' };

    Now nobody needs to read the comments anymore. Unfortunately these signs are not portable so i switched them to some standard signs on the keyboard:

    constexpr auto button_ch{ '#' };
constexpr auto bomb_ch{ '*' };

  3. Limit your line width. Why? To open two source files next to each other without scrolling. Also it stresses the eye to scroll to the right aswell. For example this:

    int width = offset_flag_w + flags_disp_size + 1 > side_w + offset * 2 ? offset_flag_w + flags_disp_size + 1 : side_w + offset * 2;

    Isn't that better:

    int width = offset_flag_w + flags_disp_size + 1 > side_w + offset * 2 ? 
 offset_flag_w + flags_disp_size + 1 : 
 side_w + offset * 2;

    There are tools who can show you how long a line can be. Common limits are 80 or 100 chars. I personally stick to 80. I think it also forces you to write cleaner code.

  4. Limit your function size. You already made functions good. The bad part is they are still too long. For example your main function. Is it easy to follow how the program flows there? I think not. There are many nasty details to follow the function. You should consider hiding them into their own small functions. I would break them into many small functions who best only do one thing. Generally a function should fit on one screen without scrolling. Otherwise the function is probably too big and does too many tasks.

  1. Don't use #define for constants: This:

    //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

    The first step here is to realize that we now have true constants in C++ without using the preprocessor. Why bother? Well the preprocessor doesn't follow the rules of C++ always. For more detail see this: https://stackoverflow.com/questions/42388077/constexpr-vs-macros

    Anyway your code then becomes this:

    //read only variables - can be changed.
constexpr auto num_of_bombs{ 60 }; //HAS TO BE LESS THAN 100 AND LESS THAN HEIGHT * WIDTH
constexpr auto height{ 13 }; //the amount of actual "buttons" in the height of the grid
constexpr auto width{ 25 }; //the amount of actual "buttons" in the width of the grid
constexpr auto offset{ 3 }; //how far away is the minefield from the sides of the screen
constexpr auto offset_flag_w{ 20 }; //how far away is the flags counter from the left of the screen
constexpr auto offset_flag_h{ 1 }; //how far away is the flags counter from the top of the screen
constexpr auto button_ch{ 219 }; //this is the ascii value of a blank square - character: '█'
constexpr auto bomb_ch{ 15 }; //'☼'
//No change allowed
constexpr auto side_w(width * 2 - 1); //the horizontal side of the grid of the minefield (buttons + space between buttons)
constexpr auto side_h(height * 2 - 1); //the vertical side of the grid of the minefield (buttons + space between buttons)
constexpr auto size{ side_h * side_w };
constexpr auto flags_disp_size{ 4 };
//colors (b= background, f=foregruond):
constexpr auto gray_f{ FOREGROUND_BLUE | FOREGROUND_RED | FOREGROUND_GREEN };
constexpr auto white_f{ gray_f | FOREGROUND_INTENSITY };
constexpr auto gray_b{ BACKGROUND_BLUE | BACKGROUND_RED | BACKGROUND_GREEN };
constexpr auto white_b{ gray_b | BACKGROUND_INTENSITY };

  2. State intent directly in Code. For this only one example. Consider this code we have now:

    constexpr auto button_ch{ 219 }; //this is the ascii value of a blank square - character: '█'
constexpr auto bomb_ch{ 15 }; //'☼'

    What does 219 mean? You said it in the comment. It would be better to say it in the code like this:

    constexpr auto button_ch{ '█' };
constexpr auto bomb_ch{ '☼' };

    Now nobody needs to read the comments any more. Unfortunately these signs are not portable so I switched them to some standard signs on the keyboard:

    constexpr auto button_ch{ '#' };
constexpr auto bomb_ch{ '*' };

  3. Limit your line width. Why? To open two source files next to each other without scrolling. Also it stresses the eye to scroll to the right as well. For example this:

    int width = offset_flag_w + flags_disp_size + 1 > side_w + offset * 2 ? offset_flag_w + flags_disp_size + 1 : side_w + offset * 2;

    Isn't that better:

    int width = offset_flag_w + flags_disp_size + 1 > side_w + offset * 2 ? 
 offset_flag_w + flags_disp_size + 1 : 
 side_w + offset * 2;

    There are tools that can show you how long a line can be. Common limits are 80 or 100 chars. I personally stick to 80. I think it also forces you to write cleaner code.

  4. Limit your function size. You already made functions; good. The bad part is they are still too long. For example your main function. Is it easy to follow how the program flows there? I think not. There are many nasty details to follow the function. You should consider hiding them into their own small functions. I would break them into many small functions which each only do one thing. Generally a function should fit on one screen without scrolling. Otherwise the function is probably too big and does too many tasks.

added 2 characters in body
Source Link
yuri
  • 4.5k
  • 3
  • 19
  • 40

This question is already quite old. Still theres alotthere's a lot more to suggest for improvements.

  1. Don't use #define for constants: This:

    //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

    The first step here is to realize that we now have true constants in C++ without using the preprocessor. Why bother? Well the preprozessorpreprocessor doesn't follow the rules of C**C++ always. For more detail see this: https://stackoverflow.com/questions/42388077/constexpr-vs-macros

    Anyway youreyour code thanthen becomes this:

    //read only variables - can be changed.
constexpr auto num_of_bombs{ 60 }; //HAS TO BE LESS THAN 100 AND LESS THAN HEIGHT * WIDTH
constexpr auto height{ 13 }; //the amount of actual "buttons" in the height of the grid
constexpr auto width{ 25 }; //the amount of actual "buttons" in the width of the grid
constexpr auto offset{ 3 }; //how far away is the minefield from the sides of the screen
constexpr auto offset_flag_w{ 20 }; //how far away is the flags counter from the left of the screen
constexpr auto offset_flag_h{ 1 }; //how far away is the flags counter from the top of the screen
constexpr auto button_ch{ 219 }; //this is the ascii value of a blank square - character: '█'
constexpr auto bomb_ch{ 15 }; //'☼'
//No change allowed
constexpr auto side_w(width * 2 - 1); //the horizontal side of the grid of the minefield (buttons + space between buttons)
constexpr auto side_h(height * 2 - 1); //the vertical side of the grid of the minefield (buttons + space between buttons)
constexpr auto size{ side_h * side_w };
constexpr auto flags_disp_size{ 4 };
//colors (b= background, f=foregruond):
constexpr auto gray_f{ FOREGROUND_BLUE | FOREGROUND_RED | FOREGROUND_GREEN };
constexpr auto white_f{ gray_f | FOREGROUND_INTENSITY };
constexpr auto gray_b{ BACKGROUND_BLUE | BACKGROUND_RED | BACKGROUND_GREEN };
constexpr auto white_b{ gray_b | BACKGROUND_INTENSITY };

  2. State intend directly in Code. For this only one example. Consider This code we have now:

    constexpr auto button_ch{ 219 }; //this is the ascii value of a blank square - character: '█'
constexpr auto bomb_ch{ 15 }; //'☼'

    What does 219 mean? You said it in the comment. It would be better to say it in the code like this:

    constexpr auto button_ch{ '█' };
constexpr auto bomb_ch{ '☼' };

    Now nobody needs to read the comments anymore. Unfortunately these signs are not portable so i switched them to some standard signs on the keyboard:

    constexpr auto button_ch{ '#' };
constexpr auto bomb_ch{ '*' };

  3. Limit youreyour line width. Why? To open two source files next to each other without scrolling. Also it stresses the eye twoto scroll to the right aswell. For example this:

    int width = offset_flag_w + flags_disp_size + 1 > side_w + offset * 2 ? offset_flag_w + flags_disp_size + 1 : side_w + offset * 2;

    Isn't that better:

    int width = offset_flag_w + flags_disp_size + 1 > side_w + offset * 2 ? 
 offset_flag_w + flags_disp_size + 1 : 
 side_w + offset * 2;

    There are tools who can show you how long a line can be. Common limits are 80 or 100 chars. I personally stick to 80. I think it also forces you to write cleaner code.

  4. Limit youre Functionyour function size. You already made functions good. The bad part is they are still totoo long. For example youreyour main function. Is it easy to follow how the program flows there? I think not. There are many nasty details to follow the function. You should consider hiding them into itstheir own small functions. I would break them into many small functions who best only do one thing. Generally a function should fit on one screen without scrolling. Otherwise the function is probalyprobably too big and does totoo many tasks.

This question is already quite old. Still theres alot more to suggest for improvements.

  1. Don't use #define for constants: This:

    //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

    The first step here is to realize that we now have true constants in C++ without using the preprocessor. Why bother? Well the preprozessor doesn't follow the rules of C** always. For more detail see this: https://stackoverflow.com/questions/42388077/constexpr-vs-macros

    Anyway youre code than becomes this:

    //read only variables - can be changed.
constexpr auto num_of_bombs{ 60 }; //HAS TO BE LESS THAN 100 AND LESS THAN HEIGHT * WIDTH
constexpr auto height{ 13 }; //the amount of actual "buttons" in the height of the grid
constexpr auto width{ 25 }; //the amount of actual "buttons" in the width of the grid
constexpr auto offset{ 3 }; //how far away is the minefield from the sides of the screen
constexpr auto offset_flag_w{ 20 }; //how far away is the flags counter from the left of the screen
constexpr auto offset_flag_h{ 1 }; //how far away is the flags counter from the top of the screen
constexpr auto button_ch{ 219 }; //this is the ascii value of a blank square - character: '█'
constexpr auto bomb_ch{ 15 }; //'☼'
//No change allowed
constexpr auto side_w(width * 2 - 1); //the horizontal side of the grid of the minefield (buttons + space between buttons)
constexpr auto side_h(height * 2 - 1); //the vertical side of the grid of the minefield (buttons + space between buttons)
constexpr auto size{ side_h * side_w };
constexpr auto flags_disp_size{ 4 };
//colors (b= background, f=foregruond):
constexpr auto gray_f{ FOREGROUND_BLUE | FOREGROUND_RED | FOREGROUND_GREEN };
constexpr auto white_f{ gray_f | FOREGROUND_INTENSITY };
constexpr auto gray_b{ BACKGROUND_BLUE | BACKGROUND_RED | BACKGROUND_GREEN };
constexpr auto white_b{ gray_b | BACKGROUND_INTENSITY };

  2. State intend directly in Code. For this only one example. Consider This code we have now:

    constexpr auto button_ch{ 219 }; //this is the ascii value of a blank square - character: '█'
constexpr auto bomb_ch{ 15 }; //'☼'

    What does 219 mean? You said it in the comment. It would be better to say it in the code like this:

    constexpr auto button_ch{ '█' };
constexpr auto bomb_ch{ '☼' };

    Now nobody needs to read the comments anymore. Unfortunately these signs are not portable so i switched them to some standard signs on the keyboard:

    constexpr auto button_ch{ '#' };
constexpr auto bomb_ch{ '*' };

  3. Limit youre line width. Why? To open two source files next to each other without scrolling. Also it stresses the eye two scroll to the right aswell. For example this:

    int width = offset_flag_w + flags_disp_size + 1 > side_w + offset * 2 ? offset_flag_w + flags_disp_size + 1 : side_w + offset * 2;

    Isn't that better:

    int width = offset_flag_w + flags_disp_size + 1 > side_w + offset * 2 ? 
 offset_flag_w + flags_disp_size + 1 : 
 side_w + offset * 2;

    There are tools who can show you how long a line can be. Common limits are 80 or 100 chars. I personally stick to 80. I think it also forces you to write cleaner code.

  4. Limit youre Function size. You already made functions good. The bad part is they are still to long. For example youre main function. Is it easy to follow how the program flows there? I think not. There are many nasty details to follow the function. You should consider hiding them into its own small functions. I would break them into many small functions who best only do one thing. Generally a function should fit on one screen without scrolling. Otherwise the function is probaly too big and does to many tasks.

This question is already quite old. Still there's a lot more to suggest for improvements.

  1. Don't use #define for constants: This:

    //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

    The first step here is to realize that we now have true constants in C++ without using the preprocessor. Why bother? Well the preprocessor doesn't follow the rules of C++ always. For more detail see this: https://stackoverflow.com/questions/42388077/constexpr-vs-macros

    Anyway your code then becomes this:

    //read only variables - can be changed.
constexpr auto num_of_bombs{ 60 }; //HAS TO BE LESS THAN 100 AND LESS THAN HEIGHT * WIDTH
constexpr auto height{ 13 }; //the amount of actual "buttons" in the height of the grid
constexpr auto width{ 25 }; //the amount of actual "buttons" in the width of the grid
constexpr auto offset{ 3 }; //how far away is the minefield from the sides of the screen
constexpr auto offset_flag_w{ 20 }; //how far away is the flags counter from the left of the screen
constexpr auto offset_flag_h{ 1 }; //how far away is the flags counter from the top of the screen
constexpr auto button_ch{ 219 }; //this is the ascii value of a blank square - character: '█'
constexpr auto bomb_ch{ 15 }; //'☼'
//No change allowed
constexpr auto side_w(width * 2 - 1); //the horizontal side of the grid of the minefield (buttons + space between buttons)
constexpr auto side_h(height * 2 - 1); //the vertical side of the grid of the minefield (buttons + space between buttons)
constexpr auto size{ side_h * side_w };
constexpr auto flags_disp_size{ 4 };
//colors (b= background, f=foregruond):
constexpr auto gray_f{ FOREGROUND_BLUE | FOREGROUND_RED | FOREGROUND_GREEN };
constexpr auto white_f{ gray_f | FOREGROUND_INTENSITY };
constexpr auto gray_b{ BACKGROUND_BLUE | BACKGROUND_RED | BACKGROUND_GREEN };
constexpr auto white_b{ gray_b | BACKGROUND_INTENSITY };

  2. State intend directly in Code. For this only one example. Consider This code we have now:

    constexpr auto button_ch{ 219 }; //this is the ascii value of a blank square - character: '█'
constexpr auto bomb_ch{ 15 }; //'☼'

    What does 219 mean? You said it in the comment. It would be better to say it in the code like this:

    constexpr auto button_ch{ '█' };
constexpr auto bomb_ch{ '☼' };

    Now nobody needs to read the comments anymore. Unfortunately these signs are not portable so i switched them to some standard signs on the keyboard:

    constexpr auto button_ch{ '#' };
constexpr auto bomb_ch{ '*' };

  3. Limit your line width. Why? To open two source files next to each other without scrolling. Also it stresses the eye to scroll to the right aswell. For example this:

    int width = offset_flag_w + flags_disp_size + 1 > side_w + offset * 2 ? offset_flag_w + flags_disp_size + 1 : side_w + offset * 2;

    Isn't that better:

    int width = offset_flag_w + flags_disp_size + 1 > side_w + offset * 2 ? 
 offset_flag_w + flags_disp_size + 1 : 
 side_w + offset * 2;

    There are tools who can show you how long a line can be. Common limits are 80 or 100 chars. I personally stick to 80. I think it also forces you to write cleaner code.

  4. Limit your function size. You already made functions good. The bad part is they are still too long. For example your main function. Is it easy to follow how the program flows there? I think not. There are many nasty details to follow the function. You should consider hiding them into their own small functions. I would break them into many small functions who best only do one thing. Generally a function should fit on one screen without scrolling. Otherwise the function is probably too big and does too many tasks.

Source Link
Sandro4912
  • 3.2k
  • 2
  • 23
  • 50

This question is already quite old. Still theres alot more to suggest for improvements.

Here are some suggestions not mentioned yet.

  1. Don't use #define for constants: This:

    //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

    The first step here is to realize that we now have true constants in C++ without using the preprocessor. Why bother? Well the preprozessor doesn't follow the rules of C** always. For more detail see this: https://stackoverflow.com/questions/42388077/constexpr-vs-macros

    Anyway youre code than becomes this:

    //read only variables - can be changed.
constexpr auto num_of_bombs{ 60 }; //HAS TO BE LESS THAN 100 AND LESS THAN HEIGHT * WIDTH
constexpr auto height{ 13 }; //the amount of actual "buttons" in the height of the grid
constexpr auto width{ 25 }; //the amount of actual "buttons" in the width of the grid
constexpr auto offset{ 3 }; //how far away is the minefield from the sides of the screen
constexpr auto offset_flag_w{ 20 }; //how far away is the flags counter from the left of the screen
constexpr auto offset_flag_h{ 1 }; //how far away is the flags counter from the top of the screen
constexpr auto button_ch{ 219 }; //this is the ascii value of a blank square - character: '█'
constexpr auto bomb_ch{ 15 }; //'☼'
//No change allowed
constexpr auto side_w(width * 2 - 1); //the horizontal side of the grid of the minefield (buttons + space between buttons)
constexpr auto side_h(height * 2 - 1); //the vertical side of the grid of the minefield (buttons + space between buttons)
constexpr auto size{ side_h * side_w };
constexpr auto flags_disp_size{ 4 };
//colors (b= background, f=foregruond):
constexpr auto gray_f{ FOREGROUND_BLUE | FOREGROUND_RED | FOREGROUND_GREEN };
constexpr auto white_f{ gray_f | FOREGROUND_INTENSITY };
constexpr auto gray_b{ BACKGROUND_BLUE | BACKGROUND_RED | BACKGROUND_GREEN };
constexpr auto white_b{ gray_b | BACKGROUND_INTENSITY };

  2. State intend directly in Code. For this only one example. Consider This code we have now:

    constexpr auto button_ch{ 219 }; //this is the ascii value of a blank square - character: '█'
constexpr auto bomb_ch{ 15 }; //'☼'

    What does 219 mean? You said it in the comment. It would be better to say it in the code like this:

    constexpr auto button_ch{ '█' };
constexpr auto bomb_ch{ '☼' };

    Now nobody needs to read the comments anymore. Unfortunately these signs are not portable so i switched them to some standard signs on the keyboard:

    constexpr auto button_ch{ '#' };
constexpr auto bomb_ch{ '*' };

  3. Limit youre line width. Why? To open two source files next to each other without scrolling. Also it stresses the eye two scroll to the right aswell. For example this:

    int width = offset_flag_w + flags_disp_size + 1 > side_w + offset * 2 ? offset_flag_w + flags_disp_size + 1 : side_w + offset * 2;

    Isn't that better:

    int width = offset_flag_w + flags_disp_size + 1 > side_w + offset * 2 ? 
 offset_flag_w + flags_disp_size + 1 : 
 side_w + offset * 2;

    There are tools who can show you how long a line can be. Common limits are 80 or 100 chars. I personally stick to 80. I think it also forces you to write cleaner code.

  4. Limit youre Function size. You already made functions good. The bad part is they are still to long. For example youre main function. Is it easy to follow how the program flows there? I think not. There are many nasty details to follow the function. You should consider hiding them into its own small functions. I would break them into many small functions who best only do one thing. Generally a function should fit on one screen without scrolling. Otherwise the function is probaly too big and does to many tasks.

I will here later to add more when I have the time...

lang-cpp

AltStyle によって変換されたページ (->オリジナル) /