HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppModerate

Console maze program

Submitted by: @import:stackexchange-codereview··
0
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' },
{ '#', '#', '#', ' ', '#', '#', '#', '#', '#', ' ', '#', '#', '#', '#', '#' },
{ '#', '#', '#', ' ', ' ', ' ', '#', '#', '#', ' ', '#', '#', '#', '#', '#' },
{ '#', '#', '#', '#', '#', ' ', '#', '#', '#', ' ', '#', '#', '#', '#', '#' },
{ '#', '#', '#', '#', '#', ' ', '#', '#', '#', ' ', '#', '#', '#', '#', '#' },
{ '#', '#', '#', '#', '#', ' ', ' ', ' ', ' ', ' ', '#', '#', '#', '#', '#' },
{ '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#',

Solution

Use boolean conditions directly

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 statements

Avoid 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.