Krdytkyu

This question is a followup to Simple RPG skeleton, Part 1 - The Character.

This is the second "part" of my development of this skeleton, and it now includes two new classes, Entity, and SpecType. The Entity class essentially serves as a base class for any character-like object, and has the following attributes and methods:

• 
  name - The name of the entity.


• 
  health - How much health the entity has.


• 
  toString() - Return a string representation of the entity.

The SpecType class serves as a small utility class for creating number-related specs that apply to character-like object, and has the following attributes and methods:

• 
  specValue - The value of the spec itself.


• 
  changeValue() - Change the value of the spec, and make sure it stays in the range $0rightarrowinfty$.

I'm wondering the following things:

• Should SpecType be an immutable class? Right now, it's just modifying itself when changeValue is called.


• Is the Entity class designed in an appropriate way?


• Am I following standard C++ practices?

entity.h

#ifndef RPGGAME_ENTITY_H_

#define RPGGAME_ENTITY_H_



#include <string>

#include "spectype.h"



/// <summary>

/// A base class that represents an entity, and implements

/// some base attributes, like health or name, and a few helper

/// methods for apply damage.

/// </summary>

class Entity

{

public:

    std::string name;

    SpecType    health;



    Entity(const std::string& name, SpecType health);

    std::string toString();

};



#endif

entity.cpp

#include <string>

#include <random>

#include "entity.h"



/// <summary>

/// Constructor for the Entity class.

/// </summary>

/// <param name="name">The name of the entity.</param>

/// <param name="health">The health of the entity.</param>

Entity::Entity(const std::string& name, SpecType health):

    name(name),

    health(health)

{}



/// <summary>

/// Return a string value representing the entity.

/// </summary>

/// <remarks>

/// This is mostly not used unless a raw Entity is being

/// used by itself, which should be very uncommon, or not

/// happen at all.

/// </remarks>

std::string Entity::toString()

{

    return "Name: "   + name                             + ", " +

           "Health: " + std::to_string(health.specValue) + "n";

}

spectype.h

#ifndef RPGGAME_SPECTYPE_H_

#define RPGGAME_SPECTYPE_H_



/// <summary>

/// The SpecType class describes a non-constant unsigned

/// integer spec that can be used in any entity-derived class.

/// or structure.

/// </summary>

class SpecType

{

public:

    int specValue;



    SpecType(int specValue);

    void changeValue(int value);

};



#endif

spectype.cpp

#include "spectype.h"



/// <summary>

/// Constructor for the SpecType class.

/// </summary>

/// <param name="specValue">The value that the SpecType contains.</param>

SpecType::SpecType(int specValue):

    specValue(specValue)

{}



/// <summary>

/// Change the Spec's value by a certain amount.

/// <summary>

/// <param name="value">The value to change specValue by.</param>

void SpecType::changeValue(int value)

{

    if (specValue + value >= 0) { specValue += value; }

    else { specValue = 0; }

}

main.cpp (tests)

#include <iostream>

#include "spectype.h"

#include "entity.h"



int main()

{

    SpecType testSpec1(0);

    testSpec1.changeValue(10);

    testSpec1.changeValue(-11);

    std::cout << testSpec1.specValue << "n";



    SpecType testSpec2(51);

    testSpec2.changeValue(-2143);

    testSpec2.changeValue(10);

    std::cout << testSpec2.specValue << "n";



    std::cin.get();



    Entity testEntity1("FooBarBlahName", 30);

    std::cout << testEntity1.toString();



    Entity testEntity2("Goku", 9001);

    std::cout << testEntity2.toString();



    std::cin.get();

}

It's not much code, so there's little else to change or fix here. The only tiny mistake you made was forgetting to mark toString with const, so that it can be called on a const object, very tiny and easy fix:

std::string toString() const;

                       ^^^^^

And the same on the definition. I'm assuming you're already familiar with the meaning of const in a method, but if not, here's an explanation. In summary, a const method is not allowed to mutate the object, only to inspect its member data, thus callable if the object itself is const.

Right now, there's not much reason for the existence of SpecType. In my opinion, it does too little to be a separate class. Assuming you are already extending that class, then it makes sense, but otherwise, it is better to avoid predicting the future and don't bother adding boilerplate classes that end up unused.

The suggestion made by a previous answer to the other question of creating a template class was interesting because each template instantiation has a unique type, so that would prevent, for instance, mixing health with mana because those would be unrelated types. Going that way, then I'd say the template Spec type would be a nice addition, even if just as a tiny wrapper, since it adds type-safety to the code.

Commenting for auto-generated documentation

I get it that some companies really value generating documentation from comments in the code, but take a piece like this one for example:

/// <summary>

/// Constructor for the SpecType class.

/// </summary>

/// <param name="specValue">The value that the SpecType contains.</param>

SpecType::SpecType(int specValue):

    specValue(specValue)

{}

What is that comment adding to the understanding of the code, that couldn't be inferred just by reading the code? Nothing whatsoever. And those XML tags are simply atrocious! They completely distract me from the actual comment text and code.

If you really must comment mechanically to generate documentation, then I'd suggesting using a better tool: doxygen.

Doxygen uses a less invasive markup than those XML tags (actually, you can choose from a few different styles), and more, it doesn't force you to exhaustively comment every method. Methods you don't comment are copied verbatim to the output doc, so obvious things like a constructor don't have to be commented with "this is the constructor for class XYZ".

Disallow default methods if you don't want them. If you do want them, make that explicit

Your code is silent as to whether you want default constructors to be available. For example, do you want programmers to be able to create empty Entity instances? Right now they can with code like the following because your compiler will generate a default constructor if you don't write one or disallow it:

Entity emptyDude;

If you're alright with that fine, but even if you are, it inspires confidence if you write something like

Entity() = default

so folks know you have planned for this possibility. On the other hand, if you don't want empty Entity instances, you can explicitly disallow them:

Entity() = delete

You should make a similar decision about default assignment and copy constructors. You can read more about this at cppreference.com.

These comments apply to SpecType as well as to Entity

Consider whether a new class is really necessary

You're probably going to build on these types, but if you weren't, I'd say that Spec type isn't really necessary. It just holds an integer and a simple member function.

Use Structs for class members that have entirely public structure

It seems that you don't take advantage of class features, so you might consider using a struct instead. Since you have no private members of any kind, it's unclear what help the class structure is.