11
\$\begingroup\$

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 <iostream>
using std::endl;
using std::cout;
using std::cin;
#include <string>
using std::string;

#include<Windows.h>
#include<conio.h>

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' },
                    { '#', '#', '#', ' ', '#', '#', '#', '#', '#', ' ', '#', '#', '#', '#', '#' },
                    { '#', '#', '#', ' ', ' ', ' ', '#', '#', '#', ' ', '#', '#', '#', '#', '#' },
                    { '#', '#', '#', '#', '#', ' ', '#', '#', '#', ' ', '#', '#', '#', '#', '#' },
                    { '#', '#', '#', '#', '#', ' ', '#', '#', '#', ' ', '#', '#', '#', '#', '#' },
                    { '#', '#', '#', '#', '#', ' ', ' ', ' ', ' ', ' ', '#', '#', '#', '#', '#' },
                    { '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#' } };

char lvl2[15][15] = { { '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#' },
                    { '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#' },
                    { '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#' },
                    { '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#' },
                    { '#', ' ', ' ', ' ', ' ', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#' },
                    { '#', ' ', '#', '#', ' ', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#' },
                    { '#', ' ', '#', '#', ' ', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#' },
                    { '#', ' ', ' ', '#', ' ', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#' },
                    { '#', '#', ' ', '#', ' ', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#' },
                    { '#', '#', ' ', '#', ' ', '#', '#', '#', '#', '#', '#', '#', '#', ' ', 'O' },
                    { '#', '#', ' ', '#', ' ', '#', '#', '#', '#', '#', '#', '#', '#', ' ', '#' },
                    { '#', '#', ' ', '#', ' ', '#', '#', ' ', ' ', ' ', ' ', ' ', ' ', ' ', '#' },
                    { '#', '#', ' ', '#', ' ', '#', ' ', ' ', '#', '#', '#', '#', '#', '#', '#' },
                    { 'X', ' ', ' ', '#', ' ', ' ', ' ', '#', '#', '#', '#', '#', '#', '#', '#' },
                    { '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#', '#' } };


int main(void){
    welcome();
begin:
    system("CLS");
    cout << "\n\n\n\n\n\n\n    \t\t\t";
    cout << "PLEASE SELECT A LEVEL \n\n    \t\t\t1---2---3---4---5---6\n    \t\t\t";
    int lvl;
    cin >> lvl;
    if (lvl != 2 && lvl != 1 && lvl > 0){
        cout << endl << endl;
        cout << "\t\t";
        cout << "Level not available yet, please check back later." << endl;
        Sleep(1000);
        goto begin;
    }
    system("CLS");
    setMe(lvl);
    printLevel(lvl);
    int x, y;

    while (1){

        char move = getKeyPress();

        switch (move){

        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;
        case 'l':
            x = getPos(lvl, y);
            if (!isWall(x, y - 1, lvl)){
                if (isExit(x, y - 1, lvl)){
                    system("CLS");
                    cout << "You Win!" << endl;
                    Sleep(2000);
                    goto begin;;
                }
                system("CLS");
                makeSpace(lvl, x, y);
                update(lvl, x, y - 1);
            }
            break;
        case 'r':
            x = getPos(lvl, y);
            if (!isWall(x, y + 1, lvl)){
                if (isExit(x, y + 1, lvl)){
                    system("CLS");
                    cout << "You Win!" << endl;
                    Sleep(2000);
                    goto begin;;
                }
                system("CLS");
                makeSpace(lvl, x, y);
                update(lvl, x, y + 1);
            }
            break;
        default:
            break;
        }

    }

    return 0;
}

void welcome(){
    string start = "WELCOME TO MAZE RUNNER v1.0";
    string indev = "Currently in development, only two levels available.";
    string howto = "Use the arrow keys and traverse through the maze. Exit is marked 'O'.";
    cout << endl;
    cout << "\n\n\n\n\n\n\n    \t\t\t";
    for (auto ch : start){
        cout << ch;
        Sleep(40);
    } cout << endl << endl;
    cout << "    \t   ";
    for (auto ch : indev){
        cout << ch;
        Sleep(40);
    } cout << endl << endl << endl;
    cout << "\t";
    for (auto ch : howto){
        cout << ch;
        Sleep(40);
    }
    Sleep(1500);
}


void printLevel(int lvl){
    cout << "\n\n\n\n\n";
    if (lvl == 1){
        for (int i = 0; i != 15; ++i){
            cout << endl << "\t\t\t\t";
            for (int j = 0; j != 15; ++j){
                cout << lvl1[i][j];
            }
        } cout << endl;
    }
    if (lvl == 2){
        for (int i = 0; i != 15; ++i){
            cout << endl << "\t\t\t\t";
            for (int j = 0; j != 15; ++j){
                cout << lvl2[i][j];
            }
        } cout << endl;
    }
}

void setMe(int lvl){
    int x, y;
    if (lvl == 1){
        x = getX(lvl, y);
        lvl1[x][y] = me;
    }
    if (lvl == 2){
        x = getX(lvl, y);
        lvl2[x][y] = me;
    }
}
//got this function from a CPP forum
char getKeyPress(){

    char key = 127;

    key = _getch();

    if (key == 0 || key == -32){

        key = _getch();

        if (key == 72) {
            key = 'u';
        } else if (key == 75){
            key = 'l';
        } else if (key == 77){
            key = 'r';
        } else if (key == 80){
            key = 'd';
        }
    }
    return key;
}

bool isExit(int x, int y, int lvl){
    if (lvl == 1){
        if (lvl1[x][y] == 'O'){
            return true;
        }
        else {
            return false;
        }
    }
    if (lvl == 2){
        if (lvl2[x][y] == 'O'){
            return true;
        }
        else {
            return false;
        }
    }
    return true;
}

int getPos(int lvl, int &y){
    int xCoord;
    if (lvl == 1){
        for (int i = 0; i != 15; ++i){
            for (int j = 0; j != 15; ++j){
                if (lvl1[i][j] == '@'){
                    xCoord = i;
                    y = j;
                    return xCoord;
                }
            }
        }
    }
    if (lvl == 2){
        for (int i = 0; i != 15; ++i){
            for (int j = 0; j != 15; ++j){
                if (lvl2[i][j] == '@'){
                    xCoord = i;
                    y = j;
                    return xCoord;
                }
            }
        }
    }
    return 0;
}

bool isWall(int x, int y, int lvl){
    if (lvl == 1){
        if (lvl1[x][y] == '#'){
            cout << "\n\t\t\tCannot move! That is a wall / boundary.";
            Sleep(400);
            system("CLS");
            printLevel(lvl);
            return true;
        }
        else {
            return false;
        }
    }
    if (lvl == 2){
        if (lvl2[x][y] == '#'){
            cout << "\n\t\t\tCannot move! That is a wall / boundary.";
            Sleep(400);
            system("CLS");
            printLevel(lvl);
            return true;
        }
        else {
            return false;
        }
    }
    return true;
}

int getX(int lvl, int &y){
    int xCoord;
    if (lvl == 1){
        for (int i = 0; i != 15; ++i){
            for (int j = 0; j != 15; ++j){
                if (lvl1[i][j] == 'X'){
                    xCoord = i;
                    y = j;
                    return xCoord;
                }
            }
        }
    }
    if (lvl == 2){
        for (int i = 0; i != 15; ++i){
            for (int j = 0; j != 15; ++j){
                if (lvl2[i][j] == 'X'){
                    xCoord = i;
                    y = j;
                    return xCoord;
                }
            }
        }
    }
    return 0;
}

void update(int lvl, int x, int y){
    if (lvl == 1){
        lvl1[x][y] = me;
        printLevel(lvl);
    }
    if (lvl == 2){
        lvl2[x][y] = me;
        printLevel(lvl);
    }
}

void makeSpace(int lvl, int x, int y){
    if (lvl == 1){
        lvl1[x][y] = space;
    }
    if (lvl == 2){
        lvl2[x][y] = space;
    }
}

Having an if-statement for each level I add seems extremely tedious so I'm working on an algorithm at the moment to generate a unique level each time a new level is selected. Still, some feedback/review would be great.

\$\endgroup\$
11
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thank you for taking the time to go through the program and for the great tips! Gonna go make some changes! \$\endgroup\$ – Ishaan May 19 '15 at 18:20
8
\$\begingroup\$

In addition to janos' answer, I'd like to add some additional tips.

Avoid Goto

Goto simply creates "spaghetti code" that will become harder to maintain over time. Instead, use do/while and break as appropriate. Once the user wins, simply return from the function that should be maintaining the play loop; once the user requests an exit, return from the main application loop (examples further down).

Keep Main Simple

In a typical application, the main function only calls 1 or 2 functions, usually in the form of init(); mainLoop(); Your main function should be close to:

void main() {
    welcome();
    mainLoop();
}

From there, your mainLoop function should pretty much be:

void mainLoop() {
    while(1) {
        int level = selectLevel();
        if(!level) {
            return;
        }
        play(level);
    }
}

Avoid Hard-Coding Values

selectLevel should validate the level range. In a real game engine, you'd probably have files in a data folder instead of hard-coding the levels within the source code. For example, you might have files that look like this:

data/level.001
data/level.002
data/level.003
data/level.004

And so on. Your selectLevel function could open the data directory and read the list of files (it doesn't necessarily need to actually to read the contents of the files, just a directory list).

Use Structs/Classes To Track Data

You can make your design simpler by having a struct that contains the game state. You could even eventually add a save/load routine later by isolating the game state from the rest of general memory. Here's a suggestion (feel free to modify as desired):

struct mapData {
    int x, y, width, height;
    char** map;
}

Load From Data Files Instead of Source Code

Once the user selects a valid level, you can then have the play function start by loading the level, when repeatedly drawing the map and polling user input until they win:

void play(int level) {
    mapData map = load(level);
    while(1) {
        drawLevel(map);
        if(processInput(map)) {
            return;
        }
    }
}

Using this design means that you could literally edit levels into the data folder and have them appear when the menu is displayed, and offer (potentially) unlimited levels.

Since we're talking about C++, the files could be simple, something like:

Level 1: Beginner's Luck

###############
s #   #   #   e
# # #####   ###
#     #   ### #
##### ### #   #
...

Where 's' is the starting point, 'e' is the goal, and # are the walls (basically, similar to the format you have now). I'd also include a title, but that's a personal preference. In this case, I'd read the first line from each file while displaying the menu. Alternatively, to avoid reading tons of files, you might just have an "idx" (index) file (e.g. levels.idx) that contains some format, like:

1 Beginner's Luck
2 Down the Twisted Path
3 Confused I Am
4 The Next Annoyance

One file per level still applies, though, as it will reduce loading time and memory usage (not that either is significant in a game this small, but the principle is important).

\$\endgroup\$
  • \$\begingroup\$ Thanks for the tips! I'll be sure to implement them sooner or later. Also, great names for the first four levels, haha. \$\endgroup\$ – Ishaan May 19 '15 at 23:19
  • 2
    \$\begingroup\$ Your answer seems good enough to me but please stop the "evil goto" thing. break and continue are just fancy gotos after all. \$\endgroup\$ – Vladimir Cravero May 20 '15 at 7:17
  • 1
    \$\begingroup\$ I'm not saying that goto must be avoided at all costs, as there are clever and appropriate uses of goto. My main point was only that one should prefer functions over goto in normal situations. Plus, modern languages rarely have a goto syntax, so getting in the habit of not using goto is really about preparing budding developers for future projects. However, it's also likely that they will eventually stumble upon a proper use of goto, at which point they should definitely use it. \$\endgroup\$ – phyrfox May 20 '15 at 13:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.