Escape the Trolls console game

I wrote a simple console game, with the idea taken from a Reddit post.

The basic Idea is: Player, controlled via arrow keys, has to escape the maze (reach the exit marked X). If a 2nd player is enabled, he controls a troll via WASD and has to catch the player before he reaches the exit. The 1st player can push walls by moving in their direction.

There are a few specific points I'd like to be addressed:

• Are libraries included properly? I go by the logic "include library in a file if it is used in that file", is this correct?

-
Is my implementation of players acceptable? And, if not, how should I implement them? I originally planned to create a generic Actor class and derive Player, Orc, etc. from it, but I could not find a good way to update the state of the Actor objects. If I created a separate update() method for each of the derived classes, there would be a lot of code repetition. Handling everything in a single update() seemed to have the least code repetition, but the function gets bigger every time I add a new feature, so it might soon be hard to maintain.

-
Should I document header, cpp or both files?

Here is the code:

Input file:

```
#####################################
# # # # # # #
# # ##### # ### ##### ### ### ### # #
# # # # # # # # # #
##### # ##### ##### ### # # # ##### #
# # # # # # # # # # #
# # ####### # # ##### ### # ##### # #
# # # # # # # # # #
# ####### ### ### # ### ##### # ### #
# # # # # # # # # #
# ### ### # ### # ##### # # # #######
# # # # # # # # # # # #
####### # # # ##### # ### # ### ### #
# # # # # # # # # #
# ### # ##### ### # ### ### ####### #
# # # # # # # # # #
# # ##### # ### ##### # # ####### # #
# # # # # # # # # # #
# ##### # # # ### ##### ##### # #####
# # # # # # # # # #
# # ### ### ### ##### ### # ##### # #
# # #

It surprised me that no one actually decided to write a review, even though it's been 4 days now.

It was quite a challenge to read and review the entire project you've posted here. I wrote down everything I found suspicious, but I'm still more than sure that I've missed something. Nevertheless, here's my list of things I didn't like about your code:

Naming

Naming you use throughout your code is quite confusing. Names are meant to describe what this method does, or that variable contains. You should pay attention when naming your objects. For instance, you have at(...) method, which basically returns a character. This name doesn't really tell us what exactly we gonna get. Is this just a character, or a player, or a wall? You should always pick names that match object's purpose. In this case, I'd rename this method to something like getChar(...) as it tells us that we're going to get a character that occupies this position.

Another thing is, you often shrink your names. Dir enumeration and fName don't really tell us what is their purpose unless we either see their usages or go to their declarations. That's bad. Don't be afraid to use longer names, they help others a lot to get the point. I suggest you to rename Dir to Direction, so no one confuses it with Directory, and fName to fileName. Of course, you're free to use shorter versions in case they're easy to understand, like pos. pos is somewhat "traditional" when it comes to working with positions.

In your Key enumeration, you have following members:

enum class Key
{
    //skip1     = 0,     // Handled by default case
    escape      = 27,
    //space     = 32,
    arrowUp     = 72,
    arrowLeft   = 75,
    arrowRight  = 77,
    arrowDown   = 80,
    keyA        = 97,
    keyD        = 100,
    keyS        = 115,
    keyW        = 119,
    //skip2     = 224    // Handled by default case
};

Although names for its members are okay and you probably wanted to separate arrow keys from letters with key prefix, I'd suggest you to get rid of it anyway. That way it looks a little better, especially in cases such as this:

switch (key)
{
    case Key::W:
        // Do sth...
        break;
    case Key::A:
        // Do sth...
        break;
    // Others...
}

You don't have to write key after Key enumeration all the time, you only need to write a specific key.

It's not mandatory, but I also recommend to capitalize enumaretation members, e.g. arrowUp to ARROW_UP and w to W. Capitalized named constants and enumeration members are very common in C and C++, so this way you eliminate a small possiblity of others making mistakes when working with your code.

Scoping

You put all enumerations and constants used in your game into enums.h and constants.h header files, and it looks like an attempt to gather global data and enumerations into a single place, or to avoid increasing the amount of files. You see, you may want to use only Key or Direction in certain files in the future, but both of these enums are situated in enums.h, thus forcing you to include both of them, including a completely unnecessary function. It'd be better to use separate header files for these enumerations, thus moving Direction to Direction.h and Key to Key.h. Surely you get more files in the end, but as the White King from great Alice: Madness Returns said, "Sacrifices must be made.".

Also, you have this weird namespace constants with global constants within, and their only usage is in game.cpp, so there's no need for neither namespace nor global constants. That means, you can safely move all these constants to your Game class, and make them private there.

class Game 
{
// Other things...
private:
    // Other things...

    const int leftMargin = 5;
    const int upperMargin = 1;

    // Default console foreground colour (white)
    const unsigned short defaultColour = FOREGROUND_BLUE | FOREGROUND_GREEN | FOREGROUND_RED;
    const unsigned short normalColour = FOREGROUND_GREEN;
    const unsigned short actorColour = FOREGROUND_RED | FOREGROUND_INTENSITY;   
};

Always try to avoid global variables, arrays or similar data. In the vast majority of cases (Including yours), you don't really need them at all. Instead, you should pass such data as arguments if it is used across several files, or make it a private field if it is used in a single class only.

Refactoring

Now, to the hardest part. Correctness is essential indeed, but you must also dedicate as much effort as possible to keep your program clean and simple, which is always a challenge for every programmer, especially if you're working on something brand new, undiscovered by others, and have no idea how you could possibly optimize or refactor it.

Firstly, I'd sit and re-think how your game works once more. In order to play the game, you call play() method that creates a Game Loop, where you clear the screen, display the grid, handle input and cal

StackExchange Code Review Q#141001, answer score: 2