0
\$\begingroup\$

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;
}
\$\endgroup\$
3
  • \$\begingroup\$ I'd just have implemented a static member variable on Character and have it tick up on constructor calls and tick down in destructor calls. Also int is a poor choice for counting things. Depending on the expected amount of instances I'd use an unsigned int or unsigned long. Also I feel like Goblin, Vampire and Ghost should be subclasses of Character. \$\endgroup\$ Oct 26, 2022 at 22:15
  • \$\begingroup\$ It is generally advised not to use unsigned types for counting, or for anything except bit-twiddling or modulo arithmetic. For just one example of this advice in the core guidelines, see ES.106. If you’re really concerned about running out of counter, merely using unsigned int instead of int hardly helps. If ~32,000 isn’t enough, use long or one of the types in <cstdint>. \$\endgroup\$
    – indi
    Oct 27, 2022 at 19:31
  • \$\begingroup\$ as for unsigned usage the trick is to check before change... \$\endgroup\$
    – neu-rah
    Oct 28, 2022 at 4:07

1 Answer 1

1
\$\begingroup\$

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;
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.