Class that counts its instances

Question

I'd love some feedback on the C++ base class shown below. It's basically a toy problem for a fake video game where the monsters that you have to avoid will all be sub-classed from the class shown below.

I'm particularly interested in feedback on the way I handled the character_count counter, which is meant to count the total number of monsters created (for simplicity, I'm assuming there is no need to ever decrement the counter): I'm practising "Avoid initialization order problems across translation units by replacing non-local static objects with local static objects." From Item 4 of Effective C++ by Scott Meyers (if you have the book and are going to look, the bit I quoted is the summary he provides for the last third of Item 4... there are two other points he makes earlier in the item).

Character.h

#pragma once

enum CharacterType { kCharacterBaseClass, kGhost, kGoblin, kVampire };

struct CharCount {
  int character_count = 0;
};

class Character {
 public:
  explicit Character(CharacterType type = kCharacterBaseClass);
  static CharCount &CharacterCount();
  [[nodiscard]] CharacterType GetType() const;
  virtual void Animate() const;


 protected:
  CharacterType type_;
};

Character.cpp

#include "GmChrctrs/Character.h"

#include <iostream>

Character::Character(CharacterType type) : type_(type) {
  ++Character::CharacterCount().character_count;
}

CharCount &Character::CharacterCount() {
  static CharCount char_count;
  return char_count;
};

CharacterType Character::GetType() const { return type_; }

void Character::Animate() const {
  std::cout << "generic character is animated" << std::endl;
}

Answer 1

The use-case isn't clear

From the code presented, it's hard to know why a count of how many characters have been constructed will be useful in your game.

If it's simply to provide a unique identifier for logging, there should be no need to expose it publicly like that.

If it's an aspect of gameplay, you're more likely to be interested in how many characters are currently active in that game/level - in which case, it should be an instance variable of the appropriate gameplay object, incremented when characters are added and decremented when they die or otherwise leave the game.

For now, let's assume there actually is a need for this, and continue with the review.

No need for the meaningless CharCount struct

I think you have misunderstood the advice from Effective C++. You quoted (my emphasis): "Avoid initialization order problems across translation units..." All our references to the count are in a single translation unit, and we have no concerns about the order other global variables are initialised.

This means we can simply use a static member variable.

The count should be private

The public method that exposes a reference to the count allows any code to set the count to any value (including meaningless values, such as negative numbers). I believe that public access should be read-only. It probably makes more sense to use an unsigned type, as the value will never be negative.

Enumerating subclasses is an anti-pattern

It looks like you're half-heartedly attempting an object-oriented design, but "switching by type" somewhere. This is a problem for several reasons, the most obvious being that you can't simply add another subclass without encoding that knowledge in the base class (the enum). Avoid this, and let subclasses express their individual behaviour through their virtual methods.

And is it really intended that a character can change its type after it's constructed? If that really is part of the gameplay, consider getting the same effect by substituting the character with a new one of different type, rather than mutating its type member.

[[nodiscard]] is unnecessary

GetType() is idempotent, so nothing is lost if the result is discarded. It can easily be retrieved again. Save [[nodiscard]] for results that can't be re-generated and could cause program errors if ignored.

Base class should have a virtual destructor

Classes intended to be subclassed should always have virtual destructors, so that deleting through a base-class pointer will perform the correct sub-class clean-up.

---

Modified

#include <cstddef>

class Character
{
    static std::size_t character_count;

public:
    Character() {
        ++character_count;
    }
    Character(const Character&) {
        ++character_count;
    }
    virtual ~Character() = default;

    static std::size_t CharacterCount() {
        return character_count;
    }

    // Iff we really need this...
    enum CharacterType { kGhost, kGoblin, kVampire };
    virtual CharacterType GetType() const = 0;

};

std::size_t Character::character_count = 0;