patterncppModerate
Console maze program
Viewed 0 times
mazeconsoleprogram
Problem
My most recent project was a console based maze game and I wanted to get some feedback on it. Is there a better way to design my code?
```
#include
using std::endl;
using std::cout;
using std::cin;
#include
using std::string;
#include
#include
void welcome();
char getKeyPress();
void printLevel(int);
void setMe(int);
bool isExit(int, int, int);
bool isWall(int, int, int);
int getPos(int, int&);
int getX(int, int &);
void update(int, int, int);
void makeSpace(int, int, int);
const char space = ' ';
const char me = '@';
char lvl1[15][15] = { { '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#' },
{ 'X', ' ', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#' },
{ '#', ' ', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#' },
{ '#', ' ', ' ', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#' },
{ '#', '#', ' ', ' ', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#' },
{ '#', '#', '#', ' ', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#' },
{ '#', '#', '#', ' ', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#' },
{ '#', '#', '#', ' ', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#' },
{ '#', '#', '#', ' ', '#', '#', '#', '#', '#', ' ', ' ', ' ', ' ', ' ', 'O' },
{ '#', '#', '#', ' ', '#', '#', '#', '#', '#', ' ', '#', '#', '#', '#', '#' },
{ '#', '#', '#', ' ', ' ', ' ', '#', '#', '#', ' ', '#', '#', '#', '#', '#' },
{ '#', '#', '#', '#', '#', ' ', '#', '#', '#', ' ', '#', '#', '#', '#', '#' },
{ '#', '#', '#', '#', '#', ' ', '#', '#', '#', ' ', '#', '#', '#', '#', '#' },
{ '#', '#', '#', '#', '#', ' ', ' ', ' ', ' ', ' ', '#', '#', '#', '#', '#' },
{ '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#',
```
#include
using std::endl;
using std::cout;
using std::cin;
#include
using std::string;
#include
#include
void welcome();
char getKeyPress();
void printLevel(int);
void setMe(int);
bool isExit(int, int, int);
bool isWall(int, int, int);
int getPos(int, int&);
int getX(int, int &);
void update(int, int, int);
void makeSpace(int, int, int);
const char space = ' ';
const char me = '@';
char lvl1[15][15] = { { '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#' },
{ 'X', ' ', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#' },
{ '#', ' ', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#' },
{ '#', ' ', ' ', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#' },
{ '#', '#', ' ', ' ', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#' },
{ '#', '#', '#', ' ', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#' },
{ '#', '#', '#', ' ', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#' },
{ '#', '#', '#', ' ', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#' },
{ '#', '#', '#', ' ', '#', '#', '#', '#', '#', ' ', ' ', ' ', ' ', ' ', 'O' },
{ '#', '#', '#', ' ', '#', '#', '#', '#', '#', ' ', '#', '#', '#', '#', '#' },
{ '#', '#', '#', ' ', ' ', ' ', '#', '#', '#', ' ', '#', '#', '#', '#', '#' },
{ '#', '#', '#', '#', '#', ' ', '#', '#', '#', ' ', '#', '#', '#', '#', '#' },
{ '#', '#', '#', '#', '#', ' ', '#', '#', '#', ' ', '#', '#', '#', '#', '#' },
{ '#', '#', '#', '#', '#', ' ', ' ', ' ', ' ', ' ', '#', '#', '#', '#', '#' },
{ '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#',
Solution
Use boolean conditions directly
This tedious if-else:
Can be written simply as:
Avoid mutually exclusive sequences of
Avoid these kind of conditions:
It will be one or the other.
For example, if
there's no need to check the value of
The above should be written with an
Extract common logic to a helper function
When you have similar looking code,
try to extract the common parts and parameterize them for the variable elements.
For example, in
The only difference between these 2 blocks of code is that in the first one you always do
You could create a function with the common element like this:
And then replace the
Also note that I moved the
no need to write the same thing repeatedly, just do it once.
Conclusion
Each of the previous sections explained different types of problems that stand out in your code,
each demonstrating with an example snippet taken from your code,
and how to rewrite better.
From easier to harder.
These are many other examples of these types of issues in your code,
if you apply the above logic everywhere,
your code should become much shorter and better.
This tedious if-else:
if (lvl2[x][y] == 'O'){
return true;
}
else {
return false;
}Can be written simply as:
return lvl2[x][y] == 'O';Avoid mutually exclusive sequences of
if statementsAvoid these kind of conditions:
if (lvl == 1) {
// ...
}
if (lvl == 2) {
// ...
}lvl cannot be 1 and 2 at the same time.It will be one or the other.
For example, if
lvl == 1 is true,there's no need to check the value of
lvl == 2.The above should be written with an
else if between the two conditions:if (lvl == 1) {
// ...
} else if (lvl == 2) {
// ...
}Extract common logic to a helper function
When you have similar looking code,
try to extract the common parts and parameterize them for the variable elements.
For example, in
switch (move), the handling of 'u' and 'd' are almost identical:case 'u':
x = getPos(lvl, y);
if (!isWall(x - 1, y, lvl)){
if (isExit(x - 1, y, lvl)){
system("CLS");
cout << "You Win!" << endl;
Sleep(2000);
goto begin;;
}
system("CLS");
makeSpace(lvl, x, y);
update(lvl, x - 1, y);
}
break;
case 'd':
x = getPos(lvl, y);
if (!isWall(x + 1, y, lvl)){
if (isExit(x + 1, y, lvl)){
system("CLS");
cout << "You Win!" << endl;
Sleep(2000);
goto begin;;
}
system("CLS");
makeSpace(lvl, x, y);
update(lvl, x + 1, y);
}
break;The only difference between these 2 blocks of code is that in the first one you always do
x + 1 and in the second one you always do x - 1.You could create a function with the common element like this:
bool applyMoveDelta(int x, int y, int lvl, int dx, int dy) {
if (!isWall(x + dx, y + dy, lvl)){
if (isExit(x + dx, y + dy, lvl)){
system("CLS");
cout << "You Win!" << endl;
Sleep(2000);
return true;
}
system("CLS");
makeSpace(lvl, x, y);
update(lvl, x + dx, y + dy);
}
return false;
}And then replace the
switch with:x = getPos(lvl, y);
switch (move) {
case 'u':
if (applyMoveDelta(x, y, lvl, -1, 0)) {
goto begin;
}
break;
case 'd':
if (applyMoveDelta(x, y, lvl, 1, 0)) {
goto begin;
}
break;
case 'l':
if (applyMoveDelta(x, y, lvl, 0, -1)) {
goto begin;
}
break;
case 'r':
if (applyMoveDelta(x, y, lvl, 0, 1)) {
goto begin;
}
break;
}Also note that I moved the
x = getPos(lvl, y) statement out of the cases:no need to write the same thing repeatedly, just do it once.
Conclusion
Each of the previous sections explained different types of problems that stand out in your code,
each demonstrating with an example snippet taken from your code,
and how to rewrite better.
From easier to harder.
These are many other examples of these types of issues in your code,
if you apply the above logic everywhere,
your code should become much shorter and better.
Code Snippets
if (lvl2[x][y] == 'O'){
return true;
}
else {
return false;
}return lvl2[x][y] == 'O';if (lvl == 1) {
// ...
}
if (lvl == 2) {
// ...
}if (lvl == 1) {
// ...
} else if (lvl == 2) {
// ...
}case 'u':
x = getPos(lvl, y);
if (!isWall(x - 1, y, lvl)){
if (isExit(x - 1, y, lvl)){
system("CLS");
cout << "You Win!" << endl;
Sleep(2000);
goto begin;;
}
system("CLS");
makeSpace(lvl, x, y);
update(lvl, x - 1, y);
}
break;
case 'd':
x = getPos(lvl, y);
if (!isWall(x + 1, y, lvl)){
if (isExit(x + 1, y, lvl)){
system("CLS");
cout << "You Win!" << endl;
Sleep(2000);
goto begin;;
}
system("CLS");
makeSpace(lvl, x, y);
update(lvl, x + 1, y);
}
break;Context
StackExchange Code Review Q#91178, answer score: 10
Revisions (0)
No revisions yet.