RPG skeleton, Part 1 - The Character

Question

I've been on a quest to learn C++, and to learn, I've started building a small Role-Playing-Game skeleton to help myself learn the language. I've tried to make sure that I've followed C++ styles/standards correctly.

This is the first "part" of my development of the skeleton, and it includes a usable class, Character. The Character class has the following attributes:

• characterName - The name of the character.
• healthPoints - How much health the player has. (Stays within the range \$0\rightarrow\infty\$.)
• experiencePoints - How much experience the player has. (Stays within the range \$0\rightarrow\infty\$.)

It also has the following methods:

• applyRandomDamage - Apply a random amount of damage to the player in a certain range.
• applyDamage - Apply a set amount of damage to the player.
• addRandomExperience - Add a random amount of experience to the player.
• addExperience - Add a set amount of experience to the player.
• toString - Display the player's statistics. (Name/Health/experience)

I'm wondering the following things:

• Is there an override so that I can just std::cout an instantiated Character object without having to create my own custom toString method?
• Is this designed appropriately? Is there anything that could be designed differently?
• Am I following the correct C++ styles/standards?
• Is this idiomatic C++?

character.h

#pragma once
#include <iostream>

/// <summary>
/// Represents a character, with certain attributes, like
/// health points, or the character's name.
/// </summary>
class Character
{
public:
    std::string characterName;
    int         healthPoints;
    int         experiencePoints;

    Character(std::string c_characterName, int c_healthPoints, int c_experiencePoints);

    void applyRandomDamage(int minimumDamage, int maximumDamage);
    void applyDamage(int damage);
    void addRandomExperience(int minimumExperience, int maximumExperience);
    void addExperience(int experience);
    void toString();
};

character.cpp

#include <iostream>
#include <string>
#include <random>
#include "character.h"

/// <summary>
/// The constructor for our character.
/// </summary>
/// <param name="characterName">The name of our character.</param>
Character::Character(std::string c_characterName, int c_healthPoints, int c_experiencePoints)
{
    characterName    = c_characterName;
    healthPoints     = c_healthPoints;
    experiencePoints = c_experiencePoints;
}

/// <summary>
/// Apply a random amount of damage to the player, based on
/// a low and a high value.
/// </summary>
/// <param name="minimumDamage">The minimum damage to apply.</param>
/// <param name="maximumDamage">The maximum damage to apply.</param>
void Character::applyRandomDamage(int minimumDamage, int maximumDamage)
{
    std::random_device                 randomDevice;
    std::mt19937                       engine(randomDevice());
    std::uniform_int_distribution<int> distribution(minimumDamage, maximumDamage);

    int damage = distribution(engine);
    healthPoints = healthPoints - damage >= 0 ? healthPoints - damage : 0;
}

/// <summary>
/// Apply a set damage to the character, rather than a
/// random amount in a certain range.
/// </summary>
void Character::applyDamage(int damage)
{
    healthPoints = healthPoints - damage >= 0 ? healthPoints - damage : 0;
}

/// <summary>
/// Add a random amount of experience to the player.
/// </summary>
/// <param name="minimumExperience">The minimum experience to add.</param>
/// <param name="maximumExperience">The maximum experience to add.</param>
void Character::addRandomExperience(int minimumExperience, int maximumExperience)
{
    std::random_device                 randomDevice;
    std::mt19937                       engine(randomDevice());
    std::uniform_int_distribution<int> distribution(minimumExperience, maximumExperience);

    int experience = distribution(engine);
    experiencePoints = experiencePoints + experience >= 0 ? experience + experience : 0;
}

/// <summary>
/// Add a set amount of experience to the player.
/// </summary>
/// <param name="experience">The experience to add to the player.</param>
void Character::addExperience(int experience)
{
    experiencePoints = experiencePoints + experience >= 0 ? experience + experience : 0;
}

/// <summary>
/// Returns a string value representing the character, e.g
/// health points, or the character's name.
/// </summary>
void Character::toString()
{
    std::cout << "Name: "       << characterName    << "\n";
    std::cout << "Health: "     << healthPoints     << "\n";
    std::cout << "Experience: " << experiencePoints << "\n";
}

main.cpp (tests)

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

int main()
{
    Character character = Character("Billy Bob Jenkins", 100, 50);
    character.toString();

    character.applyRandomDamage(5, 10);
    character.addRandomExperience(5, 10);
    character.toString();

    character.applyDamage(50);
    character.addExperience(50);
    character.toString();

    std::cin.get();
}

Answer 1

Don't recreate your random engine everytime

Here:

std::random_device                 randomDevice;
std::mt19937                       engine(randomDevice());
std::uniform_int_distribution<int> distribution(minimumExperience, maximumExperience);

This does not make it more random and can affect the quality of numbers you get. Random engines have some state. Generally you only want one engine, though you can have more, but you don't want it to be recreated everytime the function is called. This answer does a good job of explaining it:

"... Think of a random number generator where the seed must be maintained on a per-thread basis. Using a thread-local seed means that each thread gets its own random number sequence, independent of other threads.

If your seed was a local variable within the random function, it would be initialised every time you called it, giving you the same number each time. If it was a global, threads would interfere with each other's sequences. ..."

— paxdiablo

So you can do something like this:

// credit: http://cpp.indi.frih.net/blog/2014/12/the-bell-has-tolled-for-rand/
auto& prng_engine()
{
  thread_local static std::random_device rd{};
  thread_local static std::mt19937 engine{rd()};

  // Or you can replace the two previous lines with:
  //thread_local static std::mt19937
  //  prng{std::random_device{}()};

  return engine;
}

Don't try to be clever

I find this line particularly unreadable:

experiencePoints = experiencePoints + experience >= 0 ? experience + experience : 0;

It can be replaced with:

if (experience >= 0)
    experiencePoints += experience * 2;

With a comment explaining why you're doing this (for example, I don't understand why you're doing experience + experience.) Documentation-generating comments are OK, but don't really do a good job of explaining the why rather than the what.

Code duplication

I find it ironic that you have a addExperience function right below where you've used its body in the function above. Try this:

void Character::addRandomExperience(int minimumExperience, int maximumExperience)
{
    // ...
    int experience = distribution(engine);
    addExperience(experience); // <---
}

Your toString is not idiomatic

Your comment says: "Returns a string value representing the character, e.g health points, or the character's name." but your method actually has a return value of void. This is bad for your user.

Here's the thing. If you want your toString() to behave like users expect, make it a serialization method and decouple printing it with std::cout. What if you want it to output to a file instead? What if you just need to mess with the object in a serialized form without outputting it at all? Here's something simple but will need to be changed depending on your use case:

// Serializes for OUTPUT purposes. Don't use this for internal
// data handling
std::string Character::toString() const
{
    // to_string is C++11. Use stringstream in C++03
    return "Name: "      + characterName +
           "Health:"     + std::to_string(healthPoints) +
           "Experience:" + std::to_string(experiencePoints);
}

// return hash of properties, maybe for
// save file purposes
std::string Character::serialize() const
{
    // ...
}

Note: I've added const because you're not modifying anything (this is a "get" method.)

Overloading operator<<

Read the Operator overloading FAQ on stackoverflow. In particular, this is what your stub should look like:

// https://stackoverflow.com/questions/4421706/operator-overloading/4421719#4421719

std::ostream& operator<<(std::ostream& os, const Character& obj)
{
    os << obj.toString();
    return os;
}

Answer 2

I would just like to add some ideas about your "healthPoints" and "experiencePoints". I find dangerous (in a maintenance way) to :

1. do the same check in every function applying damages/experience/whatever: as the logic is the same it should be all defined in one place!

2. let any function in this class to change these parameters (nothing prevents any function to modify directly your "healthPoints", as it's not even private, but even if it was so every function in your class could change it).

3. with this design you can by mistake add or subtract experience to health and vice versa, this should be disallowed by the compiler.

So what I would do would be to create a simple template containing the value and addition and subtraction methods which check that the behaviour is good. As a consequence, "AddDamage" and co won't need to check boundaries any more! The class should stay a POD and inline everything so the performance won't be hurt...

enum SpecType {HEALTH, MANA, EXPERIENCE};

template <SpecType T> class Spec
{
private:
    /* Could be unsigned int as only subtraction can
       be a problem (and we define it in the class),
       but beware exposing this to the outside xP */ 
    int points;
public:
    Spec<T>(int value) : points(value) {}
    // ...

    int Points() const
    {
      return points;
    }

    Spec<T> operator-=(int value)
    {
      if (value >= points)
        points = 0; // maybe do something about death too...
      else
        points -= value;

       return *this;
    }
};

// ...

class Character
{
public:
    std::string characterName;
    Spec<HEALTH>      healthPoints;
    Spec<EXPERIENCE>  experiencePoints;
    Spec<MANA>        manaPoints;
    // ...
}

// ...

Character::Character(std::string c_characterName, int c_healthPoints, int c_experiencePoints) : characterName(c_characterName), heathPoints(c_healthPoints), experiencePoints(c_experiencePoints) // just as easy
{}

void Character::applyRandomDamage(int minimumDamage, int maximumDamage)
{
    // ...

    int damage = distribution(engine);
    healthPoints -= damage; // <= so much easier, and no risk to forget checking for below zero or ...
}

If you want to add levels you could add them in the Spec class so your damage functions wouldn't have to care about it themselves too. Or derive healthPoint and add more operators which would take typed damage points (normal, blunt, aggravated, ...) so the calculus will differ depending on these...?

Little bonus: maybe create user literal for these types, cf. http://www.stroustrup.com/C++11FAQ.html#UD-literals

Answer 3

Many things have already been said about your code (I like how it looks from afar by the way :p). But I still want to add my two cents:

• #pragma once is not standard and it's highly unlikely that it will ever be standardized since the standard committee is working on a modules system. On the other hand, it should work on most compilers and will sometimes improve your compile times. If you don't want your lint tools to complain about it, you should replace it or complete it with include guards:

  #ifndef RPGGAME_CHARACTER_H_
  #define RPGGAME_CHARACTER_H_
  
  // Your code here...
  
  #endif // RPGGAME_CHARACTER_H_
  

• Try to include only the headers you need. If you decide to overload operator<< as suggested, character.h will only need to know that std::ostream exists and so you can include <iosfwd> in it. It's a header that contains the declaration of the classes from the IO streams library. On the other hand, character.cpp will probably need to know what std::ostream is and so you will need to include <ostream>. As long as you don't use std::cout and its friends, you don't need to include <iostream>.

• When you don't modify a value in a function, you can pass it by const reference instead of passing it by copy so that no unnecessary copies are performed:

  Character(const std::string& c_characterName, int c_healthPoints, int c_experiencePoints);
            ^^^^^            ^
  

  The rules are actually more complex than that. For example, small types (built-in ones such as numbers or pointers) tend to be passed by copy. Also, there are more subtles rules involving copy elision and move operations but most of the time you're food using const references when you don't modify a parameter.

• Use the constructor initialization list whenever you can. The difference between using it and simply initializing the member variables in the body of the constructor is that, using the initialization list, the object is constructed with the right values. On the other hand, when you initiallize the member variables in the body of the constructor, the object is constructed, then the member variables are assigned values, which means more potential work.

  Character::Character(const std::string& characterName, int healthPoints, int experiencePoints):
      characterName(characterName),
      healthPoints(healthPoints),
      experiencePoints(experiencePoints)
  {}
  

  Note that I deliberately changed the name of the variables you passed: the names before the parenthesis will correspond to the names of the member variables while those in the parenthesis will pick the parameter names before the member variables names. It's up to you how your name your variables though :p

• I see that you are using the MSVC documentation comments. If you want more portable comments, you could use Doxygen comments instead. They are generally used by more projects and understodd by more people.

Answer 4

This was a small thing that I noticed. Rather than having two separate experience modification functions, and two different health modification functions, I could simply provide two possible function overloads. This means that the public portion of the Character class would have these signatures instead:

void applyDamage(int damage);
void applyDamage(int minimumDamage, int maximumDamage);

void applyExperience(int experience);
void applyExperience(int minimumExperience, int maximumExperience);

There then would be the subsequent implementations of these overloads in character.cpp. I figured that this implementation might be more user friendly, requiring the user to know less names.

Some of my variable names are, iffy. Here are a few I found, and their subsequent replacements:

• characterName -> name
• healthPoints -> health
• experiencePoints -> experience

I also decided that defining a base Entity class with methods like the character would be useful if I ever need to create entity-like creatures/NPCs.

class Entity
{
public:
    std::string name;
    int health;

    Entity(const std::string& name, int health);
    ...
}