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.