5
\$\begingroup\$

In a small game project I'm using 'Init' and 'Shutdown' functions on Systems/Managers/Components to make resetting them easier without having to call the constructor/destructor.

For example when reloading a level you can just call Shutdown and Init to reset to the starting state.

A potential problem however I noticed is that I have to be careful when I call Init and Shutdown, and unless I track the init, or 'active', state of the object and explicitly check for it, undefined behavior could creep in. For example, if I ever accidentally shutdown without init first, etc.

If for no reason other than curiosity, I wanted to see if I could make an abstract interface that would make tracking and maintaining the active state easier. It would be included in any class that uses the 'Init' 'Shutdown' format and with minimal other work from the user would help prevent some of the potential issues. Here is the behavior I wanted to achieve:

  • Calling Init when not active makes the object active
  • Calling Shutdown when active makes the object inactive
  • Calling Shutdown when inactive is ignored
  • Calling Init when already active calls Shutdown, then Init.

The interface I came up with is as follows:

class InitInterface
{
public:
    InitInterface() : IsActive(IsActive_), IsActive_(false) {}

    // 1. If we are not active, set us to active
    // 2. If we are active, run shutdown (virtually)
    void PreInit()
    {
        if(IsActive) this->Shutdown(); // virtual shutdown call
        IsActive_ = true;
    }

    // return value represents if a shutdown is necessary given our current active state.
    // 1. If we are not active then we can't shut down, so return true (skip shutting down in the derived class)
    // 2. If we are active, set us to not active and return false      (we do need to shut down and not skip it in the derived class)
    bool PreShutdown()
    {
        if(!IsActive) return true;
        IsActive_ = false;
        return false;
    }

    // pure virtual functions we need to implement
    virtual void Init()     = 0;
    virtual void Shutdown() = 0;

    const bool &IsActive; // read-only
private:
    bool IsActive_;
};

And to use it the minimum user implementation I've come up with would be:

class B : public A, public InitInterface
{
public:
    // REQUIRED 1: Shutdown in Destructor
    ~B() { Shutdown(); }

    // REQUIRED 2: Implement Init and call PreInit() as first line
    void Init()
    {
        PreInit(); // calls shutdown virtually
        // INIT CODE HERE
    }

    // REQUIRED 3: Implement Shutdown and check if PreShutdown() indicates a shutdown is necessary
    void Shutdown()
    {
        if(PreShutdown()) return;
        // SHUT DOWN CODE HERE
    }
};

Basically when the user writes a class that should support Init/Shutdown, they include it as a base class and must include the three sections marked as REQUIRED.

Here's an example program to test.


My question is, Is there any way to simplify what the user is required to write even further?

For example I initially wanted to virtually call Shutdown in the destructor of InitInterface to eliminate requirement #1, but realized that this wouldn't work since class B would be destructed before InitInterface.

\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

I'm afraid to say I don't like this design. You seem to have put quite a lot of thought into it, but you really don't gain much with it.

  • This is not an interface in the usual sense: it has state. Interfaces are collections of (usually) pure-virtual functions (with perhaps a non-pure virtual destructor) with no non-static data members. Multiple inheritance is tricky, picking up state from multiple bases just makes it trickier.
  • Implementing your interface forces every user to add a bunch of boiler-plate code for your design to work. That doesn't scale, leads to copy-pasta bugs (or forgetting to copy/paste bugs).
  • Your goal: "make resetting [the components] easier". Your interface reads like "provide a way of initializing and shutting down things". Minor semantic nitpick but a reset interface should have only one method: reset. This ties into the next point:
  • "unless I track the [...] state of the object [...], undefined behavior could creep in." - this is not good C++ design. Your objects should be responsible for their invariants, not some third-party addon to their class structure. Constructors should establish the invariants, destructors should to whatever cleanup is required. In between, i.e. when the object is alive, all its public members should maintain a consistent state. That you're worried about undefined behavior if you call a method "out of sequence" seems to indicate that you're not (sure of) following this rule. If that's the case, then you should fix your classes to follow it rather than try to tack on something artificial from outside, with its accompanying boilerplate.

So if you want to stick with this resetting interface, I'd go with something much simpler:

struct Resetable {
  virtual void reset() = 0;
  /* or, if passing in the "new state of the world" is needed */
  virtual void reset(GameState &) = 0;
};

Your classes must know if they need to release resources or not when reset is called (you can't write a correct destructor if they don't). For some of your classes that reset member will be trivial, for some others it might need to call into several helpers to get the job done - that's fine. Its the class's job to know how it needs to be reset, the public interface shouldn't expose that complexity.

You might also want to take a second look at your class hierarchies and figure out whether this actually should be an independent interface or if reset should be directly in the base class(es).

Or you could drop that interface completely and just use the C++ builtin stuff to create shiny fresh objects and get rid of them: constructors and destructors.


Random thoughts:

  • Your classes, methods and data members all start with capital letters. That seems a bit strange to me. But if you apply it consistently, well, that's your choice.
  • Init and Shutdown don't return anything, so the only way for them to signal failure is exceptions (or just terminating the program). That's fine if you use (and handle) exceptions, otherwise you should make them return some sort of status or error code (same for reset if you go that route).
  • Careful if you run multiple threads, the posted code isn't thread-safe.
  • const bool &IsActive; seems like a pessimisation to me. I don't think compilers will optimize that reference member out, so you're adding something like a pointer on top of the bool overhead to each and every implementer of the interface. bool isActive() const { return IsActive_; } would almost certainly be inlined and has no per-object overhead.
\$\endgroup\$

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.