15

One very common mistake with class hierarchies is to specify a method in a base class as being virtual, in order for all overrides in the inheritance chain to do some work, and forgetting to propagate the call on to base implementations.

Example scenario

class Container
{
public:
  virtual void PrepareForInsertion(ObjectToInsert* pObject)
  {
    // Nothing to do here
  }
};

class SpecializedContainer : public Container
{
protected:
  virtual void PrepareForInsertion(ObjectToInsert* pObject)
  {
    // Set some property of pObject and pass on.
    Container::PrepareForInsertion(pObject);
  }
};

class MoreSpecializedContainer : public SpecializedContainer
{
protected:
  virtual void PrepareForInsertion(ObjectToInsert* pObject)
  {
    // Oops, forgot to propagate!
  }
};

My question is: is there a good way/pattern to ensure that the base implementation always gets called at the end of the call chain?

I know of two methods to do this.

Method 1

You can use a member variable as a flag, set it to the correct value in the base implementation of the virtual method, and check its value after the call. This requires to use a public non-virtual method as interface for the clients, and making the virtual method protected (which is actually a good thing to do), but it requires the use of a member variable specifically for this purpose (which needs to be mutable if the virtual method must be const).

class Container
{
public:
  void PrepareForInsertion(ObjectToInsert* pObject)
  {
    m_callChainCorrect = false;
    PrepareForInsertionImpl(pObject);
    assert(m_callChainCorrect);
  }

protected:
  virtual void PrepareForInsertionImpl(ObjectToInsert* pObject)
  {
    m_callChainCorrect = true;
  }

private:
  bool m_callChainCorrect;
};

class SpecializedContainer : public Container
{
protected:
  virtual void PrepareForInsertionImpl(ObjectToInsert* pObject)
  {
    // Do something and pass on
    Container::PrepareForInsertionImpl(pObject);
  }
};

Method 2

The other way to do it is to replace the member variable with an opaque "cookie" parameter and do the same thing:

class Container
{
public:
  void PrepareForInsertion(ObjectToInsert* pObject)
  {
    bool callChainCorrect = false;
    PrepareForInsertionImpl(pObject, &callChainCorrect);
    assert(callChainCorrect);
  }

protected:
  virtual void PrepareForInsertionImpl(ObjectToInsert* pObject, void* pCookie)
  {
    *reinrepret_cast<bool*>(pCookie) = true;
  }
};

class SpecializedContainer : public Container
{
protected:
  virtual void PrepareForInsertionImpl(ObjectToInsert* pObject, void* pCookie)
  {
    // Do something and pass on
    Container::PrepareForInsertionImpl(pObject, pCookie);
  }
};

This approach is inferior to the first one in my opinion, but it does avoid the use of a dedicated member variable.

What other possibilities are there?

Carl Seleborg
  • 13,125
  • 11
  • 58
  • 70
  • 1
    I don't get it. Your base class does nothing, why would you need to call it? This looks like a design flaw to me. – GManNickG Aug 13 '09 at 17:25
  • 3
    GMan -- I presume this is an example. – tpdi Aug 13 '09 at 17:26
  • Even then, if the derived class is overriding the base class, it should be able to without worry. – GManNickG Aug 13 '09 at 17:28
  • 1
    @GMan: I assume that this is just for the sake of the example. In the real code, the base class would do something essential in its implementation. – JSBձոգչ Aug 13 '09 at 17:30
  • Then why let it be overwritten, is my point. – GManNickG Aug 13 '09 at 17:30
  • I assume you meant to show in your example code for SpecializedContainer to inherit from Container. Can you edit that in? – quamrana Aug 13 '09 at 17:34
  • If that's the case, the base class should hide it's container from any derived classes. It can then provide an accessor to it, which will let it do any work that needs to be done. – GManNickG Aug 13 '09 at 17:37
  • 1
    @GMan: It's not about the container: it's about having a piece of work be performed by each level of the inheritance hierarchy. That the base method happens to be empty is not a problem, derived classes should need not be aware of it. The point is: how to ensure that each level of the inheritance does its job? With virtual methods, as far as I can tell, this requires cooperation. If 'PrepareForInsertion()' bothers you, think of its name as being 'Init()', in a two-step initialization scheme. The initialization has be propagated. – Carl Seleborg Aug 14 '09 at 09:33

7 Answers7

22

You've come up with some clever ways to do this, at (as you acknowledge) the cost of bloating the class and adding code that addresses not the object's responsibilities but programmer deficiences.

The real answer is not to do this at runtime. This is a programmer error, not a runtime error.

Do it at compile time: use a language construct if the language supports it, or use a Pattern the enforces it (e.g,, Template Method), or make your compilation dependent on tests passing, and set up tests to enforce it.

Or, if failing to propagate causes the derived class to fail, let it fail, with an exception message that informs the author of the derived class that he failed to use the base class correctly.

tpdi
  • 34,554
  • 11
  • 80
  • 120
  • 6
    Can't upvote this enough. If it's virtual then it you shouldn't count/depend on any base implementation. – sirrocco Aug 13 '09 at 17:30
  • I agree but i suppose that in certain circumstance you must ensure that the call chain is correct and that it is not easy to do with static tools or tests. I think that runtime tests used only when testing (using #define) are acceptable. – neuro Aug 13 '09 at 17:31
  • 4
    Exactly what I'm so confused about. Nobody should have to call the base member, you're overriding it for a reason! – GManNickG Aug 13 '09 at 17:31
  • 1
    @GMan : He wants to specialize part ot the processing. I think template method is the key for what he wants. – neuro Aug 13 '09 at 17:34
  • 2
    I can see wanting to call the base class method, but I don't really see the necessity for mandating it. You won't want to do it every time, and so enforcing it is usually not the right thing to do. If you should be doing it, then not doing it is a bug, and presumably will be found by code reviews and tests. – David Thornley Aug 13 '09 at 17:39
  • I think the example is throwing me off, then. I'd need a real example to see the point. – GManNickG Aug 13 '09 at 17:42
  • I can see the theoretical need for this, but I haven't seen any runtime checks been done for that sort of thing anywhere. Convention seems to be good enough. Anyway, you might want to propose an attribute for this, such as `[[override_must_call_base]]``, for C++0x - though I doubt it'd get in. – Pavel Minaev Aug 13 '09 at 20:01
  • @ptdi: that's a reasonable point of view for some situations. The first issue is that the C++ compiler cannot help me here (it does it for constructors and destructors, but for all other virtual methods, you're on your own). I'd love to test it, but either you test explicitly that the call is correctly propagated (which is my question), or you test functionality, and you may see the effects of the missing call much later and in some whole other place. – Carl Seleborg Aug 14 '09 at 14:14
13

What you are looking for is simply the Non-Virtual Interface pattern.

It's similar to what you are doing here, but the base class implementation is guaranteed to be called because it's the only implementation that can be called. It eliminates the clutter that your examples above require. And the call through the base class is automatic, so derived versions don't need to make an explicit call.

Google "Non-Virtual Interface" for details.

Edit: After looking up "Template Method Pattern", I see that it's another name for Non-Virtual Interface. I've never heard it referred to by the name before (I'm not exactly a card-carrying member of the GoF fan club). Personally, I prefer the name Non-Virtual Interface because the name itself actually describes what the pattern is.

Edit Again: Here's the NVI way of doing this:

class Container
{
public:
  void PrepareForInsertion(ObjectToInsert* pObject)
  {
    PrepareForInsertionImpl(pObject);

    // If you put some base class implementation code here, then you get
    // the same effect you'd get if the derived class called the base class
    // implementation when it's finished.
    //
    // You can also add implementation code in this function before the call
    // to PrepareForInsertionImpl, if you want.
  }

private:
  virtual void PrepareForInsertionImpl(ObjectToInsert* pObject) = 0;
};

class SpecializedContainer : public Container
{
private:
  virtual void PrepareForInsertionImpl(ObjectToInsert* pObject)
  {
    // Do something and return to the base class implementation.
  }
};
Dan Moulding
  • 211,373
  • 23
  • 97
  • 98
  • Dan, if you look at my two solutions, that's exactly what I already use. The problem is the NVI pattern does not solve my call-chain problem. The NVI is not the same as the Template Method Pattern - it's more like along the lines of "C++ Coding Standards" item 39: "Consider making virtual functions nonpublic, and public functions nonvirtual." – Carl Seleborg Aug 14 '09 at 09:21
  • Carl, I think you are misunderstanding NVI. Done right, it works like this: A call is made to PrepareForInsertion on a SpecializedContainer. This invokes Container::PrepareForInsertion, which calls PrepareForInsertionImpl. But this call actually invokes SpecializedContainer::PrepareForInsertionImpl (not Container::PrepareForInsertionImpl). Note that there is no need to provide an implementation of PrepareForInsertionImpl in the base class. In fact, done properly the NVI way, it *should* be declared pure virtual in the base class, to ensure that derived classes must provide an implementation. – Dan Moulding Aug 14 '09 at 11:47
  • Sorry Dan, I don't understand what it is I'm not understanding :-) Look at my two examples again. Anyway, I want the base version to be called. In fact, I want all versions to be called, and I'm asking for a way to ensure this when the design intends for it. – Carl Seleborg Aug 14 '09 at 13:05
  • Right, and if you follow my previous comment, you see that the base version gets called, just as you want it to (the base version *is* Container::PrepareForInsertion). When using NVI, the base version calls the derived version, not the other way around. – Dan Moulding Aug 14 '09 at 15:58
  • @Carl: Let me try to put this another way. Whatever it is that you want Container::PrepareForInsertionImpl to do, put that functionality in Container::PrepareForInsertion instead, and don't supply an implementation of PerpareForInsertionImpl in the base class -- make it pure virtual instead. – Dan Moulding Aug 14 '09 at 16:06
6

When there's only one level of inheritance you can use the template method pattern where the public interface is non-virtual and calls a virtual implementation function. Then the base's logic goes in the public function which is assured to be called.

If you have more than one level of inheritance and want each class to call its base class then you can still use the template method pattern, but with a twist, make the return value of the virtual function only constructable by base so derived will be forced to call the base implementation in order to return a value (enforced at compile time).

This doesn't enforce that each class calls its direct base class, it may skip a level (I can't think of a good way to enforce that) but it does force the programmer to make a conscious decision, in other words it works against inattentiveness not malice.

class base {
protected:
    class remember_to_call_base {
        friend base;
        remember_to_call_base() {} 
    };

    virtual remember_to_call_base do_foo()  { 
        /* do common stuff */ 
        return remember_to_call_base(); 
    }

    remember_to_call_base base_impl_not_needed() { 
        // allow opting out from calling base::do_foo (optional)
        return remember_to_call_base();
    }

public:
    void foo() {
        do_foo();
    }
};

class derived : public base  {

    remember_to_call_base do_foo()  { 
        /* do specific stuff */
        return base::do_foo(); 
    }
};

If you need the public (non virtual) function to return a value the inner virtual one should return std::pair<return-type, remember_to_call_base>.


Things to note:

  1. remember_to_call_base has an explicit constructor declared private so only its friend (in this case base) can create a new instance of this class.
  2. remember_to_call_base doesn't have an explicitly defined copy constructor so the compiler will create one with public accessibility which allows returning it by value from the base implementation.
  3. remember_to_call_base is declared in the protected section of base, if it was in the private section derived won't be able to reference it at all.
Motti
  • 110,860
  • 49
  • 189
  • 262
  • Can you correct the name of the inner class? It should be remember_to_call_base and not just call_base. – quamrana Aug 13 '09 at 19:37
  • Thanks, that's what I get for editing the code after trying it out. Fixed. – Motti Aug 13 '09 at 19:57
  • Looks like a neat trick. You could as well put the `do_foo` logic inside the `remember_to_call_base` constructor, and have the derived class create a type that _aggregates_ a `remember_to_call_base` object. This way, you're actually inventing your own inheritance scheme, proving the inner-platform effect :) – xtofl Aug 13 '09 at 20:34
  • 1
    +1: This return type device is very clever indeed. I can see myself using that quite a lot now. Thanks! – Troubadour Aug 13 '09 at 20:35
  • @xtofl: I don't quite follow. How do you guarantee that the derived class aggregates the object? – Troubadour Aug 13 '09 at 20:41
  • Interesting solution, but this only works for one level of inheritance, right? Otherwise, you have to add friends to remember_to_call_base as you add new levels on inheritance so that each intermediary override can call its parent one. This breaks the open/closed principle. – Carl Seleborg Aug 14 '09 at 09:26
  • I'm stumbling on this answer a decade after it was written, but in C++11 and above this is entirely too easy to bypass. You can return `{}` to aggregate-construct `remember_to_call_base`. – Human-Compiler Aug 28 '20 at 20:57
4

A completely different approach would be to register functors. Derived classes would register some function (or member function) with the base class while in the derived class constructor. When the actual function is called by the client it is a base class function which then iterates through the registered functions. This scales to many levels of inheritance, each derived class only has to be concerned with its own function.

quamrana
  • 37,849
  • 12
  • 53
  • 71
  • Basically that's the Template Method Pattern, except that you're mimicking virtual functions through functors. – sbi Aug 14 '09 at 08:14
  • @sbi: how is it the Template Method Pattern? As I understand it, the TMP lets different parts of an abstract algorithm be implemented in derived classes. It is not about propagating work to inherited classes. – Carl Seleborg Aug 14 '09 at 09:36
  • While the highest voted answer is probably right for most people, this is my vote for the best programmatic solution. In particular, I'm reminded that often the best design is the code that makes it easy to understand the intended behavior. This is the only solution that is self-documenting and doesn't rely one weird tricks for the compiler and other things. It just does exactly what the question asks it to do! – Peter M Oct 14 '16 at 20:04
  • I also agree that this is not a Template Pattern, it is more "chain of responsibility," in GoF terms. – Peter M Oct 14 '16 at 20:09
0

Look at the template method pattern. (The basic idea is that you don't have to call the base class method anymore.)

sbi
  • 219,715
  • 46
  • 258
  • 445
0

One way out is to not use virtual methods at all, but instead to allow the user to register callbacks, and call those before doing the work of prepareForInsertion. This way it becomes impossible to make that mistake since it is the base class that makes sure that both the callbacks and the normal processing happen. You can end up with a lot of callbacks if you want this behaviour for a lot of functions. If you really do use that pattern so much, you might want to look into tools like AspectJ (or whatever the C# equivalent is) that can automate this sort of thing.

redtuna
  • 4,586
  • 21
  • 35
  • Basically that's the Template Method Pattern, except that you're mimicking virtual functions through callbacks. – sbi Aug 14 '09 at 08:13
0

If you found out you can hide virtual function and make interface non-virtual, try instead of checking if other users did call your function just call it by yourself. If your base code should be called at the end, it would look like this:

class Container
{
public:
  void PrepareForInsertion(ObjectToInsert* pObject)
  {
    PrepareForInsertionImpl(pObject);
    doBasePreparing(pObject);
  }

protected:
  virtual void PrepareForInsertionImpl(ObjectToInsert* pObject)
  {
    // nothing to do
  }

private:
  void doBasePreparing(ObjectToInsert* pObject)
  {
    // put here your code from Container::PrepareForInsertionImpl
  }
};
Tadeusz Kopec for Ukraine
  • 12,283
  • 6
  • 56
  • 83