Erroneously empty code paths

Created 10 April 2002, last updated 28 August 2004

When writing code, if you want to leave a code path empty (for example, because it’s a placeholder for more code to be written in the future, or because you think it isn’t a case that can arise, or you just don’t feel like writing it), don’t leave it empty.

Don’t do nothing if nothing isn’t the correct thing to do

An empty code path should only exist for cases where truly nothing should happen. If nothing isn’t the right thing, then don’t leave the code path empty. I’ll state it again, as a strict rule:

Your code should only do nothing if nothing is the correct thing to do.

The reason is this: it is very hard to debug a problem if the root cause is that nothing happened when something should have.

Rather than leave an empty code path, write an assert that expresses your intentions. For example, if you simply haven’t written the code yet, then instead of this:

if (IsThisATrickyFutureCase()) {
    // todo: write some code...
}

do this:

if (IsThisATrickyFutureCase()) {
    // todo: write some code...
    ASSERTMSG(FALSE, "Not yet implemented");
}

(ASSERTMSG is a macro with two arguments: an expression which must be true, and a message to display if somehow the expression is not true. Using an assert expression of FALSE means the message will always be displayed if execution reaches that point.)

Now when the tricky future case finally arrives, instead of nothing happening, and possibly making your whole system behave mysteriously, an assert will be raised, and it will be very clear that some code needs to be written.

Overly defensive coding

I’ve seen this kind of code too much:

CFoo * pFoo = CreateNewFoo();
if (pFoo != NULL) {
    pFoo->DoSomethingWithFoo();
}

In this case, CreateNewFoo() is a function that never returns NULL. The coder was torturing himself with “what-if” scenarios. By only using pFoo if it is not NULL, he avoided a crash in the dereferencing operator. But he hasn’t done anything useful about the problem of pFoo being NULL: nothing happens, no message is displayed, and the code continues on. Who knows how far it will continue before a symptom finally appears?

In this case, the bogusly-empty path is the missing else clause. But rather than add the else clause, let’s choose a different way to express our paranoid concern that somehow pFoo might be NULL.

Much better would be:

CFoo * pFoo = CreateNewFoo();
ASSERT(pFoo != NULL);
pFoo->DoSomethingWithFoo();

Now if pFoo is NULL, a failed assertion is raised. And the next developer reading this code understands what is going on: pFoo should never be NULL. In the earlier code, the next reader of the code will be led to believe (erroneously) that sometimes pFoo might be NULL.

Too much?

Some would claim that leaving these asserts all over is too compulsive. It may be, and there will be plenty of them that are never triggered, or that are removed (to be replaced by real code) a short time later. But, it is not hard to put these asserts in, especially once you get in the habit, and if even one of them fires, it will save you fifteen minutes (or more) of head scratching to figure out how everything got into some “impossible” state.

And if you provide yourself with a rich set of tools ahead of time:

#define NOT_YET_IMPLEMENTED() ASSERTMSG(FALSE, "Not yet implemented")
#define SHOULD_NOT_HAPPEN()   ASSERTMSG(FALSE, "Should not happen")
#define CANT_BE_NULL(p)       ASSERTMSG((p) != NULL, "NULL pointer!")

you can have expressive, strict code with very little trouble. These examples are in C++, where the preprocessor helps a bit, but the same is true in any language: your code should only do nothing if nothing is the correct thing to do.

Other examples

There are plenty of opportunities for bad empty code paths.

Switch defaults

Consider a switch over an enumeration. If you have an explicit case label for every defined value of the enumeration, what should the default clause be?

switch (type) {
    case kTypeA:
        HandleTypeA();
        break;
    case kTypeB:
        HandleTypeB();
        break;
    default:
        SHOULD_NOT_HAPPEN();
        break;
}

The default clause of a “complete” switch is a classic example of an empty code path (and a tricky one, since it is an invisible code path if there is no explicit “default” clause). But by putting a SHOULD_NOT_HAPPEN in the default clause, you have an early warning system for incorrect changes to the code.

We’ve all worked on systems that have grown to the point that to add a new “thing”, you have to modify ten different places to add the new thing to the list of cases. If you miss one, how will the system behave? How will you discover that you missed one? You could write a document that explained how to add a new thing, and having that document is unquestionably a Good Thing. But the document falls out of date, or is lost, or is simply not trusted by the code-hardened developer. By plugging the empty code path holes with asserts, missed cases become immediately obvious.

Stub implementations

When working with abstract interfaces, there may be many implementations of a single interface. These implementations may be owned by different groups, or require different skills to extend. What happens when a new method is added to the interface?

If an implementation of the interface is missing the new method, then that implementation will not compile. So the creator of the new method has to provide stub code in all of the implementations, or leave the code in an uncompilable state. What’s the right way to stub out a method you can’t yet implement?

void
CNotMyImplementation::NewMethod()
{
    // Didn't know how to write the code, Joe will have to do it.
    NOT_YET_IMPLEMENTED();
}

This way the missing method announces itself (loudly) so that it won’t go forgotten.

Comments

Bill T 3:11 PM on 27 Jan 2006

Adding a default: case may actually make finding places that need new code harder. Good compilers (eg, gcc) have an option that tells the compiler to generate a warning if a switch on an enum type is missing one of the enum instances. Combined with "all warnings are errors", this approach lets you detect the error as soon as you add something to the enum, rather than months later when your default branch just happens to executer.

Pedro 10:36 AM on 19 Nov 2007

Hi Bill T
To me, the best way to detect every impact of an enum is not de compilation. It this that tolls like LXR (linux cross reference) do the job. It is a little hard to set up but is it the ultimate tool for impact detection.

Pierre 9:40 AM on 30 Jan 2008

I really believe in these kind of approach. It allows to detect the sources of problems and bugs. In addition it is possible to build a set of macro that does a check raise an assertion on failure and then 'return' the function, 'continue' or 'break' a loop.

For instance I use:
CHK_BOOL_AND_RET(boolean_expected_true)
CHK_BOOL_AND_RET_VAL(boolean_expected_true, return_value)
CHK_BOOL_AND_CONT(boolean_expected_true)
CHK_BOOL_AND_BREAK(boolean_expected_true)

I provides a one-statement error notification and management.