Defective Language

A pattern bandage for messy virtual functions

In my previous article the mess of virtual functions, I show how easily they introduce bugs and propose language extensions to fix it. Several comments suggested an alternate solution with the “non-virtual interface” pattern. That’s good, since it’s what I was referring to in the last paragraph of the article. The pattern can help sometimes, but it’s definitely not a generic solution. It has limitations and problems of its own.

The non-virtual interface

In this pattern the base decides when to call the virtual part of the implementation, not relying on the derived class to call the base:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
class stream_reader {
    virtual void read_impl( input & in ) = 0;

public:
    void read( input & in ) {
        //read header stuff, do checks

        //call "true" implementation
        read_impl( in );

        //read footer stuff, do checks
    }
}

class read_yaml : stream_reader {
    /*override*/ void read_impl( input & in ) {
        //read the yaml format part of the input
    }
}

This seems good. It solves the basic problem I had in the last article. The derived class doesn’t have to worry about calling the base class at all. The virtual function is private and the base class decides precisely when it is called.

Bloat and false abstraction

The obvious issue here is syntactic bloat. In an isolated example it doesn’t look so bad, but imagine if every virtual function in your entire class hierarchy was written this way.

It’s slightly confusing what is actually happening. Logically the read function is virtual: the caller of the read is expecting a polymorphic behaviour. This logical understanding does not however match with the implementation. Let’s just hope our entire code base uses the same _impl prefix so it’s at least easy to recognize.

The use of the _impl suffix also makes me suspicious; it’s one of the key red flags I mentioned in the false abstraction antipattern. I don’t think it truly is a false abstraction, but the inability to pick a good name here should always be a warning. I’ve also see the names on_read and do_read used, both of which just use random fluff words to distinguish the name from the base. It’s a kind of language syntax fighting that I don’t like — it implies there’s a problem with the language itself.

One-level only

This pattern only “fixes” one-level polymorphism. What if we need to create a read_yaml_extended class the derives from read_yaml? Not only are we back to the same problems described in my previous article, the bandage we’ve applied actually makes it more confusing.

One approach is to continue to apply the pattern to each derived class, something we do in Fuse’s drawing system. We start with this base definition (simplified from the real code):

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
public class Visual {
    public void Draw() {
        //setup context
        OnDraw();
        //cleanup context
    }

    virtual protected void OnDraw() {
    }
}

Note we don’t have an abstract OnDraw since it’s okay for a Visual not to draw anything.

The Element type derives from Visual and does some standard drawing.

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
public class Element : Visual {
    protected sealed void OnDraw() {
        //visibility check
        //bounds clipping test
        OnElementDraw();
    }

    virtual protected void OnElementDraw() {
    }
}

The pattern is extended, but the naming issue comes up. The new virtual requires a new name, so we simply put the name of the class on it OnElementDraw. In practice this isn’t what we named them: we tried to give them names identifying what they are actually doing. It feels better, but I’m not sure it’s an improvement in legibility.

The Control type derives from Element since it can draw a background, so now we have OnControlDraw. Panel does this in turn to draw another standard layer, giving us OnDrawPanel. This continues for every derived class which wishes to do drawing.

It’s confusing when we need to implement a new class that implements Draw. Not all layers in the hierarchy override draw, so if I derive from ToggleControl it’s unclear what draw method I’m supposed to override. I have to go looking through all the base classes trying to figure out what is the most recent definition of OnWhateverDraw.

This hurts legibility and maintenance. If some intervening class introduces a new drawing override you need to modify the derived classes, modifying the function name. This is a real issue for libraries since we can’t go out and modify all the users of our library; we’ve broken our API! If we had only single virtual Draw function this wouldn’t be a problem.

Note that I’ve used sealed on the first layer of OnDraw. This is done in turn on every class the implements the next layer’s draw function. This at least prevents, at compile-time, something overriding the wrong function. Without a sealed feature I don’t think this pattern is safe to use for multiple levels of inheritance.

Necessary, but no help

If code needs tight control over when the derived function is called, this pattern is valuable, if not required. The bloat and confusing naming should however dissuade us from using this pattern unless necessary. It’s inability to solve multiple layers of inheritance also prevents it from being a catch-all solution.

It’s a nice pattern but certainly not a generic solution to the mess of virtual functions.

5 replies »

  1. Rather than NVI, I would suggest avoiding concrete inheritance in general. Provide a pure virtual interface class, and inherit from that. If you only inherit from pure virtual interfaces, there is no need to worry about calls to “super()”.

    When you think you want concrete inheritance, instead, factor out the common parts of the classes, and use composition. Sometimes you can even skip the “factoring out” part.

    class stream_reader {
    public:
    virtual ~stream_reader() = 0;
    virtual void read(input &in) = 0;
    };

    class read_yaml : public stream_reader {
    public:
    void read(input &in) override;
    };

    class read_yaml_extended : public stream_reader {
    public:
    void read(input &in) override;
    private:
    read_yaml base_reader_;
    };

    • I’d be a bigger fan of composition if languages had actual support for it. But in practive this genereally means just a lot of forwarding boilerplate. That has the potential of just introducing more errors, and using up more time. I’m in favour of this for service-like objects with a small interface, but not for things with a larger API surface where derived classes truly only modify the behaviour, not implement it all.

      It’s also not a good solution for a type hierarchy.

  2. “But in practive this genereally means just a lot of forwarding boilerplate. That has the potential of just introducing more errors, and using up more time.”

    When you need the “extended” form, you do have more forwarding boilerplate. My experience is that the boilerplate tends to be really easy to write, and it is easy to get the (usually one line) code correct.

    “I’m in favour of this for service-like objects with a small interface […]”

    Great! You should generally favor small interfaces.

    “[…] but not for things with a larger API surface where derived classes truly only modify the behaviour, not implement it all.”

    This may be the crux of your problem. Having a concrete class with 30+ methods that could reasonably be virtual is a code smell on its own. Inheriting from it to override one or two methods is often troublesome too. You get a lot of coupling that often results in the brittle base-class problem. I’ve heard it referred to as the “white-out” anti-pattern. Of course, I’ve also seen it as the Gang-of-Four “Template” pattern, but I’m not a huge fan of that either.

    • Boilerplate is high on my list of things that makes code hard to maintain and harder to read.

      For services I’m in favour of small interfaces, but not everything is a service. Sometimes things are better served as polymorphic types. Both of my examples come from scenarios where there are very few virtual functions, the majority of the class is not virtual.

      Sometimes a true type hierarchy is required since the users of those types insepct the hierarchy. I have this in my Leaf compiler where the `intr_type` hierarchy is primary virtual, but produces a tree of types that the compiler needs to know about.

  3. I don’t see any use of the proposed design approach. I for example use virtual functions for “modding” of the base flow, while the abstract functions come to define parts of it the base flow. Given that I can both decide to drop the middle layer and re-implement the modding part, or also augment the middle layer, by further specification(calling before the base function call).
    I avoid defining virtual/abstract class interfaces(public virtual, public abstract) since that tends to implode when the system begins to scale and effectively breaks the functional encapsulation along with the chain of responsibility by giving too much power to the one that implements, compared to the designer of the system.
    The OOP paradigm allows that via interfaces which are basically public abstract by default,

    Using “public abstract” in a base class is normally a sign for bad design of the foundation and/or wrong entry point in regards of the overall system flow modelling.

    Using “public virtual” is simply too dangerous, I could see that happening during interface prototyping, maybe in some really abstract designs, but generally speaking a Class even a base one needs to have a function, if you need to strip it out of all functions use interfaces, it is lighter, easier to propagate through the system and allows some really neat tricks with generics that open up a number of nasty data-access/definition problems.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s