views:

386

answers:

7

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?

A: 

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

sbi
+19  A: 

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
Can't upvote this enough. If it's virtual then it you shouldn't count/depend on any base implementation.
sirrocco
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
Exactly what I'm so confused about. Nobody should have to call the base member, you're overriding it for a reason!
GMan
+1 to stress programmer vs runtime error type, of course !
neuro
@GMan : He wants to specialize part ot the processing. I think template method is the key for what he wants.
neuro
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
I think the example is throwing me off, then. I'd need a real example to see the point.
GMan
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
@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
+11  A: 

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
Here's one good link: http://www.gotw.ca/publications/mill18.htm
Mark Ransom
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
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
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
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
@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
+2  A: 

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
Basically that's the Template Method Pattern, except that you're mimicking virtual functions through functors.
sbi
@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
A: 

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
Basically that's the Template Method Pattern, except that you're mimicking virtual functions through callbacks.
sbi
+4  A: 

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
Can you correct the name of the inner class? It should be remember_to_call_base and not just call_base.
quamrana
Thanks, that's what I get for editing the code after trying it out. Fixed.
Motti
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
+1: This return type device is very clever indeed. I can see myself using that quite a lot now. Thanks!
Troubadour
@xtofl: I don't quite follow. How do you guarantee that the derived class aggregates the object?
Troubadour
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
@Carl that's correct, see the third paragraph in my answer.
Motti
A: 

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
@tkopec: This only works for one level of inheritance.
Carl Seleborg