Simple Console Maze Game

Question

I am fairly new to C++ and programmed a maze game as an exercise. However, looking at it I can see it can be improved a lot. These are my requests if you don't mind:

1. I do not want to use the library <conio.h>.
2. I want to use another method instead of system("CLS").
3. This code is old; I think it has many unsafe codes in it and I cannot get used to C++11, C++17 etc. If you can help me update this game to the newer variants of C++, I would be more than happy.
4. In main function I just want to use function calling, so an Update() method would be nice.
5. OOP is highly appreciated.
6. Please do not hesitate to use advanced C++ like Lambda Expressions, auto type declarations etc. (see number 3). I don't know them yet, but I will try to understand.
7. Criticize like there is no tomorrow!

//maze game in console

#include <iostream>
#include <conio.h>
#include <stdlib.h>

#define KEY_UP 72
#define KEY_DOWN 80
#define KEY_LEFT 75
#define KEY_RIGHT 77

// S: Start
// #: Wall
// .: Space
// F: Finish
// P: Player

const char maze[5][5]={{'#','S','#','#','#'},
                       {'#','.','.','#','#'},
                       {'#','#','.','#','#'},
                       {'#','.','.','.','#'},
                       {'#','#','#','F','#'}};

void Clear(){
    system("CLS");//clear console
}
void InputHandle(int move, int &pX, int &pY){
    switch(move=getch()){
            case KEY_UP:
            if( pY-1 >= 0 ){
                if(maze[pY-1][pX] != '#'){//if it is not the limit or it is not the wall(#) move player 
                pY--;
            }
        }
        break;

        case KEY_DOWN:
        if( pY+1 <= 4 ){
            if(maze[pY+1][pX] != '#'){
                pY++;
            }
        }
        break;

        case KEY_RIGHT:
        if( pX+1 <= 4 ){
            if(maze[pY][pX+1] != '#'){
                pX++;
            }
        }
        break;

        case KEY_LEFT:
            if( pX-1 >= 0 ){
            if(maze[pY][pX-1] != '#'){
                pX--;
            }
        }
}
}
void Display(int &pX, int &pY){
for(int i=0;i<5;i++){
    for(int j=0;j<5;j++){
        if(j==pX && i==pY){//instead of the maze char, put player's 'P' 
            std::cout<<"P ";
            continue;
        }
        std::cout<<maze[i][j]<<" ";
    }
    std::cout<<"\n";
}
}
bool CheckWinCondition(int &pX, int &pY){
if(pX==3 && pY ==4){
    return true;
}
return false;
}

int main(){

//movement
int playerPosX=1;
int playerPosY=0;
int movement;

while(!CheckWinCondition(playerPosX,playerPosY)){//if player is not in the finish, loop 
    Display(playerPosX, playerPosY);//Show current maze
    InputHandle(movement, playerPosX, playerPosY);//take input
    Clear();//Clear the screen
}
std::cout<<"Congrats, you finished the maze!";     


}

Answer 1

Here are some things that may help you improve your code.

Use consistent formatting

The code as posted has inconsistent indentation which makes it hard to read and understand. Pick a style and apply it consistently.

Use more whitespace

Lines like this one:

for(int i=0;i<5;i++){

are easier for most humans to read and understand with more whitespace like this:

for (int i = 0; i < 5; i++) {

Don't use system("cls")

There are two reasons not to use system("cls") or system("pause"). The first is that it is not portable to other operating systems which you may or may not care about now. The second is that it's a security hole, which you absolutely must care about. Specifically, if some program is defined and named cls or pause, your program will execute that program instead of what you intend, and that other program could be anything. First, isolate these into a seperate functions cls() and pause() and then modify your code to call those functions instead of system. Then rewrite the contents of those functions to do what you want using C++. For example, if your terminal supports ANSI Escape sequences, you could use this:

void Clear()
{
    std::cout << "\x1b[2J";
}

Minimize the scope of variables

The variable movement is only used within InputHandle. This means that instead of a parameter, it could and should be a local variable within the member functions rather than a passed parameter.

Only update when needed

The current code updates the screen as fast as possible whether or not there's any reason to do so. This causes an annoying flicker on my computer and needlessly impedes the quick response of the program to user input. I'd suggest instead modifying the InputHandle routine to return a bool which is true only if the player's position has changed.

Prefer std::array to raw C-style arrays

Modern C++ provides std::array as an enhancement of a raw C array because it has knowledge of its own size, which helps with bounds checking.

Eliminate "magic numbers"

This code has a number of "magic numbers," that is, unnamed constants such as 4, 5, etc. Generally it's better to avoid that and give such constants meaningful names. That way, if anything ever needs to be changed, you won't have to go hunting through the code for all instances of "5" and then trying to determine if this particular 5 is relevant to the desired change or if it is some other constant that happens to have the same value.

Simplify your code

The current code contains this function:

bool CheckWinCondition(int &pX, int &pY)
{
    if (pX == 3 && pY == 4) {
        return true;
    }
    return false;
}

This can be simplified:

bool CheckWinCondition(int &pX, int &pY)
{
    return pX == 3 && pY == 4;
}

Use a portable library

You are right to want to eliminate the use of conio.h because it is neither standard nor portable. For example, your constants for keys don't work on my machine at all. One possible alternative would be to use the ncurses library. See Ncurses Snake game for inspiration on how to use the ncurses library in C++.

Use better names

The function CheckWinCondition and variable maze are good, descriptive names, but InputHandle and pX are not as good. I'd probably change InputHandle to handleUserInput and pX to simply x. Also, CheckWinCondition could be named isPlayerAtGoal to make it easy at a glance to understand what true and false might mean.

Don't use macros for constants

Instead of having KEY_UP and friends as old-style #define values, a better, more C++ approach is to use constexpr instead. See ES.32 and Con.5.

Use an object

The Maze would make a rather obvious object. It could include the maze structure, the player's current position and the goal's location. Methods (member functions) could include all of the current free-standing functions and the main could be reduced to this:

int main() {
    Maze maze;
    maze();
}

Further hints

Here's a rewritten display() function written using ncurses as a member function:

void Maze::display() const 
{
    clear();
    for (int row = 0; row < height; row++) {
        for (int col = 0; col < width; col++) {
            addch(tokenAt(row, col));
            addch(' ');
        }
        addch('\n');
    }
}

The clear() and addch() functions are from ncurses and tokenAt() looks like this:

char Maze::tokenAt(int row, int col) const {
    return (col == x && row == y) ? 'P' : maze[row][col];
}

Here's the member function that actually runs the maze:

void Maze::operator()() {
    initscr();
    noecho();
    nodelay(stdscr, TRUE);
    keypad(stdscr, TRUE);
    display();   
    while (!isPlayerAtGoal()) { 
        if (handleUserInput()) {
            display();
        }
    }
    endwin();
    std::cout << "Congrats, you finished the maze!\n";
}