Escape the Trolls console game

Question

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:

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

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

3. Should I document header, cpp or both files?

Here is the code:

Input file:

#####################################
# #       #       #     #         # #
# # ##### # ### ##### ### ### ### # #
#       #   # #     #     # # #   # #
##### # ##### ##### ### # # # ##### #
#   # #       #     # # # # #     # #
# # ####### # # ##### ### # ##### # #
# #       # # #   #     #     #   # #
# ####### ### ### # ### ##### # ### #
#     #   # #   # #   #     # #     #
# ### ### # ### # ##### # # # #######
#   #   # # #   #   #   # # #   #   #
####### # # # ##### # ### # ### ### #
#     # #     #   # #   # #   #     #
# ### # ##### ### # ### ### ####### #
# #   #     #     #   # # #       # #
# # ##### # ### ##### # # ####### # #
# #     # # # # #     #       # #   #
# ##### # # # ### ##### ##### # #####
# #   # # #     #     # #   #       #
# # ### ### ### ##### ### # ##### # #
# #         #     #       #       # #
#X###################################

Headers:

actor.h

#ifndef ACTOR_H
#define ACTOR_H

#include "enums.h"
#include "position.h"

class Actor
{
public:
    Actor(bool isOrc, Position pos) noexcept;
    friend class Game;
private:
    Position m_pos;

    Dir m_direction;
    bool m_isOrc;
};

#endif // ACTOR_H

consoleFunctions.h

#ifndef CONSOLEFUNCTIONS_H
#define CONSOLEFUNCTIONS_H

void setCursorPosition(int x, int y);
void setConsoleColour(unsigned short int colour);

#endif // CONSOLEFUNCTIONS_H

constants.h

#ifndef CONSTANTS_H
#define CONSTANTS_H

#include <windows.h>

namespace constants
{
    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;
}

#endif // CONSTANTS_H

enums.h

#ifndef ENUMS_H
#define ENUMS_H

#include <exception>
#include <stdexcept>

// For the arrow keys, _getch() returns 224 first
// followed by 72 (up), 80 (down), 75 (left)
// and 77 (right). If the num-pad arrow keys
// (with NumLock off) are pressed, _getch ()
// returns 0 first instead of 224

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
};

enum class Dir
{
    up,
    right,
    down,
    left
};

// Precondition - a valid movement key
inline Dir keyToDirection(Key key)
{
    Dir res;
    switch(key)
    {
    case Key::arrowUp:
    case Key::keyW:
        res = Dir::up;
        break;
    case Key::arrowDown:
    case Key::keyS:
        res = Dir::down;
        break;
    case Key::arrowLeft:
    case Key::keyA:
        res = Dir::left;
        break;
    case Key::arrowRight:
    case Key::keyD:
        res = Dir::right;
        break;
    case Key::escape:
    default:
        throw std::invalid_argument("Precondition of keyToDirection()"
                                     " has been violated.");
    }
    return res;
}

#endif // ENUMS_H

game.h

#ifndef GAME_H
#define GAME_H

#include "map.h"
#include "actor.h"
#include "position.h"
#include <vector>

class Game
{
public:
    Game();
    void play();

private:
    Map m_map;
    Actor m_player;
    Actor m_playerOrc;
    bool m_secondPlayerEnabled;

    enum class GameState
    {
        won,
        lost,
        ongoing
    };

    // Asks for user to input parameters, sets sprites on map and sets prevGrid
    // Has to be called at the start of play()
    void initialize() noexcept;
    // General update function called after valid input
    GameState update(Key key);
    // Called by update(), does player-associated updating
    GameState updatePlayer(Key key);

    void display() const noexcept;

    // Precondition - previous buffer displayed
    // Changes only the parts needed
    void displayOver() noexcept;

    // Returns valid empty cell coordinates
    Position generatePosition() const;
    // Returns next position after moving in a direction indicated by key
    // Precondition: valid key
    Position nextPosition(const Position& pos, Key key) const;
};

#endif // GAME_H

genericInput.h

#ifndef GENERICINPUT_H
#define GENERICINPUT_H

#include <string>
#include <iostream>
#include <limits>

template<typename T>
void getGenericInput(T& value, const std::string& request, const std::string& error = "Input failed, try again: ")
{
    std::cout << request;
    while(!(std::cin >> value)) {
        std::cin.clear();
        std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
        std::cout << error;
    }
    std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
}

template<typename T, typename F>
void getGenericInput(T& value, const std::string& request, F check, const std::string& error = "Input failed, try again: ")
{
    std::cout << request;
    while(!(std::cin >> value) || !check(value)) {
        std::cin.clear();
        std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
        std::cout << error;
    }
    std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
}
#endif // GENERICINPUT_H

map.h

#ifndef MAP_H
#define MAP_H

#include <vector>
#include <string>   // Temporary
#include "position.h"

class Map
{
public:
    Map() noexcept;  /* For future use. Will generate map when such function is implemented */
    Map(const std::string& fName);  /* Get the maze from a file. Temporary */
    friend class Game;

private:
    // Current(updated) map
    std::vector<std::vector<char>> m_grid;
    // Previous version of the map
    std::vector<std::vector<char>> m_prevGrid;

    static const char wall          = '#';
    static const char playerUp      = '^';
    static const char playerDown    = 'v';
    static const char playerLeft    = '<';
    static const char playerRight   = '>';
    static const char playerDead    = 'T';
    static const char orc           = '*';
    static const char exit          = 'X';

    bool isEmpty(int x, int y) const noexcept {return m_grid[y][x] == ' ';}

    /* Reconsider whether this function should be implemented as static */
    static bool isActor(char ch) noexcept;
    char& at(const Position& pos);
    char at(const Position& pos) const;
};

#endif // MAP_H

position.h

#ifndef POSITION_H
#define POSITION_H

struct Position
{
    unsigned int x, y;
    Position(unsigned x1, unsigned int y1) :
        x{x1}, y{y1} {}
    bool operator==(const Position& rhs) const noexcept {
        return x == rhs.x && y == rhs.y;
    }
};

#endif // POSITION_H

Sources:

actor.cpp

#include "actor.h"

Actor::Actor(bool isOrc, Position pos) noexcept :
    m_pos{pos},
    m_direction{Dir::down},
    m_isOrc{isOrc}
{
}

consoleFunctions.cpp

#include "consoleFunctions.h"
#include <windows.h>
#include <iostream>

// x is the column, y is the row. The origin (0,0) is top-left.
void setCursorPosition(int x, int y)
{
    static const HANDLE hOut = GetStdHandle(STD_OUTPUT_HANDLE);
    std::cout.flush();
    COORD coord = { (SHORT)x, (SHORT)y };
    SetConsoleCursorPosition(hOut, coord);
}

void setConsoleColour(unsigned short colour)
{
    static const HANDLE hOut = GetStdHandle(STD_OUTPUT_HANDLE);
    std::cout.flush();
    SetConsoleTextAttribute(hOut, colour);
}

game.cpp

#include "game.h"
#include "consoleFunctions.h"
#include "constants.h"
#include "genericInput.h"
#include <windows.h>
#include <conio.h>
#include <iostream>
#include <string>
#include <random>
#include <ctime> // For seed, std::random_device does not work properly
#include <exception>
#include <stdexcept>

Game::Game() :
    m_map("Map.txt"),
    m_player(false, generatePosition()),
    m_playerOrc(true, generatePosition())
{
}

void Game::initialize() noexcept
{
    char answ;
    getGenericInput(answ, "Welcome to Escape The Trolls! Do you want to enable 2-player mode? (y/n): ",
                    [](char answer){return answer == 'y' || answer == 'n';});
    if(answ == 'y') {
        m_secondPlayerEnabled = true;
    }
    else {
        m_secondPlayerEnabled = false;
    }

    m_map.at(m_player.m_pos) = Map::playerDown;
    if(m_secondPlayerEnabled) {
        m_map.at(m_playerOrc.m_pos) = Map::orc;
    }

    m_map.m_prevGrid = m_map.m_grid;
}

Position Game::generatePosition() const
{
    // std::random_device does not work properly with compiler used.
    static std::mt19937 gen(std::time(nullptr));
    std::uniform_int_distribution<> dis(1, m_map.m_grid.size() - 1);
    std::uniform_int_distribution<> dis2(1, m_map.m_grid[0].size() - 1);

    int x, y;
    do {
        y = dis(gen);
        x = dis2(gen);
    } while(!m_map.isEmpty(x, y));

    return Position(x, y);
}

void Game::display() const noexcept
{
    for(int i = 0; i < constants::upperMargin; ++i) {
        std::cout << std::string(m_map.m_grid[0].size(), ' ') << '\n';
    }

    for(const auto& a : m_map.m_grid) {
        std::cout << std::string(constants::leftMargin, ' ');
        for(const auto& b : a) {
            if(Map::isActor(b)) {
                setConsoleColour(constants::actorColour);
                std::cout << b;
                setConsoleColour(constants::normalColour);
            }
            else {
                std::cout << b;
            }
        }
        std::cout << '\n';
    }
    std::cout << '\n';
}

void Game::displayOver() noexcept
{
    for(std::size_t i = 0; i < m_map.m_grid.size(); ++i) {
        for(std::size_t j = 0; j < m_map.m_grid[i].size(); ++j) {
            // If a block has not changed
            if(m_map.m_grid[i][j] == m_map.m_prevGrid[i][j]) {
                continue;
            }

            setCursorPosition(j + constants::leftMargin, i + constants::upperMargin);

            if(Map::isActor(m_map.m_grid[i][j])) {
                setConsoleColour(constants::actorColour);
                std::cout << m_map.m_grid[i][j];
                setConsoleColour(constants::normalColour);
            }
            else {
                std::cout << m_map.m_grid[i][j];
            }

        }
    }
    std::cout.flush();
    m_map.m_prevGrid = m_map.m_grid;
    setCursorPosition(0, m_map.m_grid.size() + constants::upperMargin);
}

void Game::play()
{
    initialize();
    system("cls");
    setConsoleColour(constants::normalColour);
    display();
    GameState state = GameState::ongoing;
    while(state == GameState::ongoing) {
        int c = _getch();
        switch(c)
        {
        case static_cast<int>(Key::keyA):
        case static_cast<int>(Key::keyD):
        case static_cast<int>(Key::keyS):
        case static_cast<int>(Key::keyW):
            if(!m_secondPlayerEnabled) {
                break;
            }
        case static_cast<int>(Key::arrowUp):
        case static_cast<int>(Key::arrowDown):
        case static_cast<int>(Key::arrowLeft):
        case static_cast<int>(Key::arrowRight):
            state = update(static_cast<Key>(c));
            break;
        case static_cast<int>(Key::escape):
            state = GameState::lost;
            break;
        default:
            break;
        }
        displayOver();
    }
    setConsoleColour(constants::defaultColour);
}

// Precondition: valid key
Position Game::nextPosition(const Position& pos, Key key) const
{
    Position tPos = pos;
    switch(key)
    {
    case Key::arrowUp:
    case Key::keyW:
        --tPos.y;
        break;
    case Key::arrowDown:
    case Key::keyS:
        ++tPos.y;
        break;
    case Key::arrowLeft:
    case Key::keyA:
        --tPos.x;
        break;
    case Key::arrowRight:
    case Key::keyD:
        ++tPos.x;
        break;
    default:
        throw std::invalid_argument("Precondition of nextPosition()"
                                    " has been violated.");
    }
    return tPos;
}

// Precondition - a valid movement key
Game::GameState Game::updatePlayer(Key key)
{
    Actor& player = (key == Key::arrowDown || key == Key::arrowLeft
       || key == Key::arrowRight || key == Key::arrowUp ? m_player : m_playerOrc);

    Game::GameState resultingGameState;
    bool isEaten = false; // Triggered on collision with troll

    // Direction change takes one turn
    Dir tDir = keyToDirection(key);
    if(tDir != player.m_direction) {
        player.m_direction = tDir;
        resultingGameState = Game::GameState::ongoing;
    }
    // Else player tries to move
    else {
        // Would-be position
        Position tPos = nextPosition(player.m_pos, key);

        switch(m_map.at(tPos))
        {
        case Map::exit:
            if(player.m_isOrc) {
                resultingGameState = Game::GameState::ongoing;
            }
            else {
                resultingGameState = Game::GameState::won;
            }
            break;
        // Any collision of players result in a loss
        case Map::playerDown:
        case Map::playerLeft:
        case Map::playerRight:
        case Map::playerUp:
        case Map::orc:
            isEaten = true;
            m_map.at(player.m_pos) = ' ';
            player.m_pos = tPos;
            resultingGameState = Game::GameState::lost;
            break;
        case Map::wall:
            // Player can push walls
            if(!player.m_isOrc) {
                bool isValid(true);
                Position nextPos(0, 0);
                try
                {
                    nextPos = nextPosition(tPos, key);
                    m_map.at(nextPos);
                }
                catch(const std::out_of_range& e)
                {
                    isValid = false;
                }
                if(!isValid) {
                    resultingGameState = Game::GameState::ongoing;
                    break;
                }
                else if(m_map.at(nextPos) == ' ') {
                    static Position lastWallPos = Position(0, 0);
                    if(lastWallPos == tPos) {
                        m_map.at(nextPos) = Map::wall;
                        m_map.at(tPos) = ' ';
                        m_map.at(player.m_pos) = ' ';
                        player.m_pos = tPos;
                    }
                    else {
                        lastWallPos = tPos;
                    }
                }
            }
            resultingGameState = Game::GameState::ongoing;
            break;
        case ' ':
            m_map.at(player.m_pos) = ' ';
            player.m_pos = tPos;
            // Intentional fall-through to default case
        default:
            resultingGameState = Game::GameState::ongoing;
        }
    }

    // Sprite of the player is updated
    char playerSprite;
    if(player.m_isOrc) {
        playerSprite = Map::orc;
    }
    else if(isEaten) {
        playerSprite = Map::playerDead;
    }
    else {
        switch(player.m_direction)
        {
        case Dir::up:
            playerSprite = Map::playerUp;
            break;
        case Dir::down:
            playerSprite = Map::playerDown;
            break;
        case Dir::left:
            playerSprite = Map::playerLeft;
            break;
        case Dir::right:
            playerSprite = Map::playerRight;
        }
    }

    m_map.at(player.m_pos) = playerSprite;

    return resultingGameState;
}

Game::GameState Game::update(Key key)
{
    auto cGameState = updatePlayer(key);
    // Future updating

    return cGameState;
}

map.cpp

#include "map.h"
#include "constants.h"
#include <iostream>
#include <fstream>  // temporary
#include <exception>
#include <stdexcept>

Map::Map() noexcept
{
    // Eventually will call generate()
}

// Temporary, thus no exception handling ect.
Map::Map(const std::string& fName) : Map()
{
    int i(0);
    char ch;
    m_grid.push_back(std::vector<char>());
    std::ifstream in(fName);
    while(in) {
        in.get(ch);
        if(ch == '\n') {
            ++i;
            m_grid.push_back(std::vector<char>());
        }
        else if(in){
            m_grid[i].push_back(ch);
        }
    }
    m_prevGrid = m_grid;
}

bool Map::isActor(char ch) noexcept
{
    if(ch == playerDown || ch == playerLeft
       || ch == playerRight || ch == playerUp
       || ch == orc || ch == playerDead) {
        return true;
    }
    return false;
}

char& Map::at(const Position& pos)
{
    if(pos.y >= m_grid.size() || pos.x >= m_grid[0].size()) {
        throw std::out_of_range("Attempted access of Map out of bounds.");
    }
    return m_grid[pos.y][pos.x];
}

char Map::at(const Position& pos) const
{
    if(pos.y >= m_grid.size() || pos.x >= m_grid[0].size()) {
        throw std::out_of_range("Attempted access of Map out of bounds.");
    }
    return m_grid[pos.y][pos.x];
}

main.cpp

#include <iostream>
#include "game.h"

int main()
{
    Game ETT;
    ETT.play();
}

The project is not yet finished. Things that I plan to implement:

• Maze generation
• Ai-controlled Actors

And probably more. I want to remove all the errors / bad practices/ bad design etc. before I continue working on it.

Answer 1

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 call... displayOver()? This is what got my attention. This method also draws characters on the screen, which is not a good thing. Normally, you should divide your game loop into several independent steps:

1. Check if the game is still ongoing.
2. If it is, render things according to their transform (In your case, just print the grid)
3. Handle player's input.
4. Update player's and/or world's state.
5. Return to step 2.
6. If the game has ended, end the session.

So, consider merging displayOver() and display() methods together as they perform the same task (To draw characters) at different times.

Beware of using system("cls") as it calls external program that can be easily replaced by a different one with the same name and this is not all. Here is a list of alternatives you may want to use in the future instead.

You use enum class instead of simple enum, even though you don't even use features that enum class provides. You don't use strongly typed enumerations either. Therefore, I suggest you to replace enum class with enum. That way you also can get rid of static_cast<...>(...) calls in your switch inside play() method.

switch(c)
{
    case Key::A:
    case Key::D:
    case Key::S:
    case Key::W:
        if(!m_secondPlayerEnabled) {
            break;
        }
    case Key::ARROW_UP:
    case Key::ARROW_DOWN:
    case Key::ARROW_LEFT:
    case Key::ARROW_RIGHT:
        state = update(c);
        break;
    case Key::ESCAPE:
        state = GameState::lost;
        break;
    default:
        break;
}

As a matter of fact, I do not recommend you to use functions provided by conio.h, it's not part of C++ Standard Library, and not every compiler supports it. It's good for small exercises and prototypes, but normally you should use functions from C++ Standard Library, such as std::cin.get().

Lastly, I'd replace your if-statement in isActor(...) method with a switch as you compare characters. It still doesn't look perfect, yet this way you compare them a little quicker than using if.

switch (ch)
{
    case playerDown:
    case playerLeft:
    case playerRight:
    case playerUp:
    case orc:
    case playerDead:
        return true;
}

Questions

Are libraries included properly?

You shouldn't include libraries in source files unless there's no header file tied with this source file. Put all your includes inside MyClass.h, and keep MyClass.cpp as clean as possible.

Is my implementation of players acceptable?

Actor class looks quite clean and simple. The only thing that I didn't like is displayOver() method that did rendering multiple times.

Should I document header, cpp or both files?

Ideally good and clean code doesn't need huge blocks of comments to understand it, but if your solutions are quite messy, it is essential to comment them. Document only those functions or methods whose purpose is unclear at first glance, like displayOver().