RPG skeleton, Part 1 - The Character

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 

/// 
/// Represents a character, with certain attributes, like
/// health points, or the character's name.
/// 
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();
};

Don't recreate your random engine everytime

Here:

std::random_device                 randomDevice;
std::mt19937                       engine(randomDevice());
std::uniform_int_distribution 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;
}

StackExchange Code Review Q#102150, answer score: 27