Encapsulated text-based RPG using a randomized combat system

Question

For my rags-to-riches submission, I've decided to improve this code:

Text-based RPG game using classes

However, I've decided to start off with something different. The original code is more interactive, but mine just runs automatically. I may consider making my code the same, but I wanted to try this first. I think my chosen approach is less error-prone as I don't have to worry about input validation and such.

I am proud to declare that my code appears more encapsulated than the original code, which was full of getters and setters. Mine does not have any at all, and instead uses inheritance, which was inspired by this answer to the same question. As it's my first real attempt at it, I'd especially like that aspect reviewed. I feel that my attempt isn't quite right, also because the derived classes don't have their own functions.

Other than inheritance, I'm concerned about maintainability. I will expand on this, such as by adding player/monster stats (attack and defense) and different monsters. I just didn't want to include it until I had this cleaned up first, especially if my use of inheritance could be problematic.

I've kept everything as integers to make things easier, and because RPG games already have numbers (such as health and experience) as integers. I'm also not sure if I should worry about making anything unsigned instead, except for health. I think it's okay for the player or a monster to end up with negative health after damage calculations, as it would still consider the creature to be dead.

I'm sort of playing around with the stats and combat numbers, which is also why the player ends up dying quickly sometimes (but they might still be reasonable). I do also plan on studying them further.

Please feel free to comment on everything. I want this to be cleaned up well at the start so that it's easier to keep it that way as it gets larger.

Test run on Ideone

LifeForm.hpp

#ifndef LIFEFORM_HPP
#define LIFEFORM_HPP

#include <ostream>
#include <random>

namespace
{
    std::random_device rd;
    std::mt19937 gen(rd());
    std::uniform_int_distribution<> attack_dist(20, 50);
}

class LifeForm
{
protected:
    int level;
    int exp;
    int req_EXP;
    int max_health;
    int health;

    LifeForm(int level, int exp, int req_EXP, int max_health)
        : level(level)
        , exp(exp)
        , req_EXP(req_EXP)
        , max_health(max_health)
        , health(max_health)
    {}

    void level_up()
    {
        level++;
        exp = 0;
        req_EXP += level * 100;
        max_health += 50 * level;
        health = max_health;
    }

public:
    bool health_depleted() const { return health <= 0; }

    void attack(LifeForm& attacked_lifeform)
    {
        attacked_lifeform.health -= level * attack_dist(gen);
    }

    void add_EXP(LifeForm const& life_form)
    {
        exp += life_form.level * 50;

        if (exp >= req_EXP)
        {
            level_up();
        }
    }

    friend std::ostream& operator<<(std::ostream& out, LifeForm const& lifeform)
    {
        out << "Level: " << lifeform.level;
        out << "\nHP   : (" << lifeform.health << '/' << lifeform.max_health << ')';
        out << "\nEXP  : (" << lifeform.exp << '/' << lifeform.req_EXP << ')';
        return out;
    }
};

class Player : public LifeForm
{
public:
    Player() : LifeForm(1, 0, 100, 200) {}
};

class Monster : public LifeForm
{
public:
    Monster() : LifeForm(1, 50, 0, 100) {}
};

#endif

Game.hpp

#ifndef GAME_HPP
#define GAME_HPP

#include "LifeForm.hpp"
#include <vector>

class Game
{
private:
    Player player;
    std::vector<Monster> monsters;

    void battle();
    Monster current_monster() { return monsters.back(); }
    bool all_monsters_dead() const { return monsters.empty(); }

public:
    Game();
    void play();
};

#endif

Game.cpp

#include "Game.hpp"
#include <iostream>

Game::Game() : monsters(50) {}

void Game::play()
{
    std::cout << "Number of monsters: " << monsters.size() << "\n\n";

    while (!player.health_depleted() && !monsters.empty())
    {
        std::cout << "\nPlayer\n" << player;
        battle();

        if (all_monsters_dead())
        {
            std::cout << "\n\n> All monsters killed!!!";
        }

        if (player.health_depleted())
        {
            std::cout << "> Player dead...\n\n";
        }
    }
}

void Game::battle()
{
    std::cout << "\n\n> Player attacks!\n";
    player.attack(current_monster());

    if (current_monster().health_depleted())
    {
        std::cout << "> Monster killed!\n";
        player.add_EXP(current_monster());
        monsters.pop_back();
    }

    std::cout << "\nMonster " << monsters.size() << "\n" << current_monster();
    std::cout << "\n\n> Monster attacks!\n";
    current_monster().attack(player);
}

Main.cpp

#include "Game.hpp"

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

Answer 1

As it is, I can see an oversight in the level_up function: imagine that you need 5xp to level up. You gain 50xp, you level up, and your xp is 0 again. You just wasted 45xp! Here is the corrected function:

void level_up()
{
    level++;
    exp -= req_EXP;      // do not waste EXP
    req_EXP += level * 100;
    max_health += 50 * level;
    health = max_health;
}

That said, I don't really like the case of the name req_EXP. If you want your naming to be consistent, you should change it to req_exp. And the same applies to add_EXP.

---

You have useless elements in this loop:

while (!player.health_depleted() && !monsters.empty())
{
    std::cout << "\nPlayer\n" << player;
    battle();

    if (all_monsters_dead())
    {
        std::cout << "\n\n> All monsters killed!!!";
    }

    if (player.health_depleted())
    {
        std::cout << "> Player dead...\n\n";
    }
}

The two conditions used to display messages will only appear once the whole battle is finished, and they are already checked in the while condition. Therefore, they should be moved out of the loop:

// Fight the monsters
while (!player.health_depleted() && !monsters.empty())
{
    std::cout << "\nPlayer\n" << player;
    battle();
}

if (all_monsters_dead())
{
    std::cout << "\n\n> All monsters killed!!!";
}
else if (player.health_depleted())
{
    std::cout << "> Player dead...\n\n";
}

If you killed all the monsters, you can't be dead, so I added a little if to make it clear. Also, I would change the name of battle to something akin to play_turn since battle gives the feeling that we are playing the whole battle.

---

I also have the feeling that there is a problem in the function battle as is. This line:

current_monster().attack(player);

seems to always be executed, even if we just killed the last monster. In such a case, it appears to be calling pop_back on an empty std::vector, which is undefined behaviour. I don't think I am mistaken, so you should put a condition before executing that last instruction.

---

One late remark: design-wise, I don't really like that function signature:

void add_EXP(LifeForm const& life_form);

I would have changed it to:

void add_EXP(int exp);

and added a function exp_gained or exp_reward or something like that to LifeForm. That allows you to add new means of earning experience points without having to change the class LifeForm (rewards from quests for example). It makes it easier to add new elements to the game without having to modify those which already exist. Add it makes the name add_exp more consistent.

---

And yet another small error: if you want damages to remain from one turn to another, current_monster should return a Monster& and not a Monster. Otherwise, you are merely trying to kill a temporary monster; in other words, a monster that heals between turns. You probably just introduced this error with your last edit.

Monster& current_monster()
{
    return monsters.back();
}

Actually, you could do that and provide a const overload that returns by value:

Monster current_monster() const
{
    return monsters.back();
}

Answer 2

A small thing related to inheritance that you have to be careful of with C++: since you don't define a destructor, one will be defined for you. This is fine in all cases where you aren't managing a resource of some kind, excepting when you use inheritance with a base class that is not supposed to be directly instantiated. The way the code is at the moment, I can happily write:

auto x = std::unique_ptr<LifeForm>(new Player);

And this will happily compile, which as you know is bad (non-virtual destructor + polymorphism = bad times).

The fix for this is very simple: if you have a base class that shouldn't be used polymorphically (and in fact, shouldn't be created directly), you should make the destructor protected:

class LifeForm
{
protected:
    ~LifeForm() = default;
    ....
};

This will stop any polymorphic usage (as when the compiler tries to insert a call to ~LifeForm in the above unique_ptr example, it'll be protected and won't compile), while allowing any derived class to properly clean up its inherited base class.