Is it reasonable to null guard every single dereferenced pointer?

Question

At a new job, I've been getting flagged in code reviews for code like this:

PowerManager::PowerManager(IMsgSender* msgSender)
  : msgSender_(msgSender) { }

void PowerManager::SignalShutdown()
{
    msgSender_->sendMsg("shutdown()");
}

I'm told that last method should read:

void PowerManager::SignalShutdown()
{
    if (msgSender_) {
        msgSender_->sendMsg("shutdown()");
    }
}

i.e., I must put a NULL guard around the msgSender_ variable, even though it is a private data member. It's difficult for me to restrain myself from using expletives to describe how I feel about this piece of 'wisdom'. When I ask for an explanation, I get a litany of horror stories about how some junior programmer, some-year, got confused about how a class was supposed to work and accidentally deleted a member he shouldn't have (and set it to NULL afterwards, apparently), and things blew up in the field right after a product release, and we've "learned the hard way, trust us" that it's better to just NULL check everything.

To me, this feels like cargo cult programming, plain and simple. A few well-meaning colleagues are earnestly trying to help me 'get it' and see how this will help me write more robust code, but... I can't help feeling like they're the ones who don't get it.

Is it reasonable for a coding standard to require that every single pointer dereferenced in a function be checked for NULL first—even private data members? (Note: To give some context, we make a consumer electronics device, not an air traffic control system or some other 'failure-equals-people-die' product.)

EDIT: In the above example, the msgSender_ collaborator isn't optional. If it's ever NULL, it indicates a bug. The only reason it is passed into the constructor is so PowerManager can be tested with a mock IMsgSender subclass.

SUMMARY: There were some really great answers to this question, thanks everyone. I accepted the one from @aaronps chiefly due to its brevity. There seems to be fairly broad general agreement that:

1. Mandating NULL guards for every single dereferenced pointer is overkill, but
2. You can side-step the whole debate by using a reference instead (if possible) or a const pointer, and
3. assert statements are a more enlightened alternative to NULL guards for verifying that a function's preconditions are met.

Answer 1

It depends on the 'contract':

If PowerManager MUST have a valid IMsgSender, never check for null, let it die sooner.

If on the other hand, it MAY have a IMsgSender, then you need to check every time you use, as simple as that.

Final comment about the story of the junior programmer, the problem is actually the lack of testing procedures.

Answer 2

I feel the code should read:

PowerManager::PowerManager(IMsgSender* msgSender)
  : msgSender_(msgSender)
{
    assert(msgSender);
}

void PowerManager::SignalShutdown()
{
    assert(msgSender_);
    msgSender_->sendMsg("shutdown()");
}

This is actually better than guarding the NULL, because it makes it very clear that the function should never be called if msgSender_ is NULL. It also makes sure that you'll notice if this happens.

The shared "wisdom" of your colleagues will silently ignore this error, with unpredictable results.

In general bugs are easier to fix if they're detected closer to their cause. In this example the proposed NULL guard would lead to a shutdown message not getting set, which may or may not result in a noticeable bug. You'd have a harder time working backwards to the SignalShutdown function than if the entire application just died, producing a handy-dandy backtrace or core dump pointing directly to SignalShutdown().

It's a little counterintuitive, but crashing as soon as anything is wrong tends to make your code more robust. That's because you actually find the problems, and tends to have very obvious causes as well.

Answer 3

If msgSender must never be null, you should put the null check only in the constructor. This is also true for any other inputs into the class - put the integrity check at the point of entry into the 'module' - class, function etc.

My rule of thumb is to perform integrity checks between module boundaries - the class in this case. In addition, a class should be small enough that it is possible to quickly mentally verify the integrity of the lifetimes of the class members - ensuring that such mistakes as improper deletes / null assignments are prevented. The null check that is performed in the code in your post assumes that any invalid usage actually assigns null to the pointer - which is not always the case. Since 'invalid usage' inherently implies that any assumptions about regular code does not apply, we cannot be sure to catch all types of pointer errors - for example an invalid delete, increment etc.

In addition - if you are certain the argument can never be null, consider using references, depending on your usage of the class. Otherwise, consider using std::unique_ptr or std::shared_ptr in lieu of a raw pointer.

Answer 4

No, it is not reasonable to check each and every pointer dereference for the pointer being NULL.

Null-pointer checks are valuable on function arguments (including constructor arguments) to ensure preconditions are met or to take appropriate action if an optional parameter is not provided, and they are valuable to check a class's invariant after you have exposed the class's internals. But if the only reason for a pointer to have become NULL is the existence of a bug, then there is no point in checking. That bug could just as easily have set the pointer to another invalid value.

If I were faced with a situation like yours, I would ask two questions:

• Why was the mistake from that junior programmer not caught before the release? For example in a code review or in the testing phase? Bringing a faulty consumer electronics device to the market can be just as costly (if you take market share and goodwill into account) as releasing a faulty device used in a safety-critical application, so I would expect the company to be serious about testing and other QA activities.
• If the null-check fails, what error handling do you expect me to put in place, or could I just write assert(msgSender_) instead of the null check? If you just put the null check in, you might have prevented a crash, but you might have created a worse situation because the software continues on the premise that an operation has taken place while in reality that operation was skipped. This might lead to other parts of the software becoming unstable.

Answer 5

This example seems to be more about object lifetime than whether or not an input parameter is null†. Since you mention that the PowerManager must always have a valid IMsgSender, passing the argument by pointer (thereby allowing the possibility of a null pointer) strikes me as a design flaw††.

In situations like this, I would prefer to change the interface so the caller's requirements are enforced by the language:

PowerManager::PowerManager(const IMsgSender& msgSender)
  : msgSender_(msgSender) {}

void PowerManager::SignalShutdown() {
    msgSender_->sendMsg("shutdown()");
}

Rewriting it this way says that PowerManager needs to hold a reference to IMsgSender for it's entire lifetime. This, in turn, also establishes an implied requirement that IMsgSender must live longer than PowerManager, and negates the need for any null pointer checking or assertions inside PowerManager.

You can also write the same thing using a smart pointer (via boost or c++11), to explicitly force IMsgSender to live longer than PowerManager:

PowerManager::PowerManager(std::shared_ptr<IMsgSender> msgSender) 
  : msgSender_(msgSender) {}

void PowerManager::SignalShutdown() {
    // Here, we own a smart pointer to IMsgSender, so even if the caller
    // destroys the original pointer, we still have a valid copy
    msgSender_->sendMsg("shutdown()");
}

This method is prefered if it is possible that IMsgSender's lifetime can not be guaranteed to be longer than PowerManager's (i.e., x = new IMsgSender(); p = new PowerManager(*x);).

† With regard to pointers: rampant null checking makes code harder to read, and does not improve stability (it improves the appearance of stability, which is much worse).

Somewhere, someone got an address for memory to hold the IMsgSender. It is that function's responsibility to make sure that the allocation succeeded (checking library return values or properly handling std::bad_alloc exceptions), so as to not pass around invalid pointers.

Since the PowerManager does not own the IMsgSender (it's just borrowing it for a while), it is not responsible for allocating or destroying that memory. This is another reason why I prefer the reference.

†† Since you're new at this job, I expect that you're hacking on existing code. So by design flaw, I mean that the flaw is in the code that you're working with. Thus, the people who are flagging your code because it's not checking for null pointers are really flagging themselves for writing code that requires pointers :)

Answer 6

Like exceptions, guard conditions are only useful if you know what to do to recover from the error or if you want to give a more meaningful exception message.

Swallowing an error (whether caught as an exception or a guard-check), is only the right thing to do when the error doesn't matter. The most common place for me to see errors being swallowed is in error logging code -- you don't want to crash an app because you were unable to log a status message.

Anytime a function is called, and it's not optional behavior, it should fail loudly, not silently.

Edit: on thinking about your junior programmer story it sounds like what happened was that a private member was set to null when that was never supposed to be allowed to happen. They had a problem with inappropriate writing and are attempting to fix it by validating upon reading. This is backwards. By the time you identify it, the error has already happened. The code-review/compiler enforceable solution for that isn't guard conditions, but instead getters and setters or const members.

Answer 7

As other have noted, this depends on whether or not msgSender can be legitimately NULL. The following assumes that it should never be NULL.

void PowerManager::SignalShutdown()
{
    if (!msgSender_)
    {
       throw SignalException("Shut down failed because message sender is not set.");
    }

    msgSender_->sendMsg("shutdown()");
}

The proposed "fix" by the others on your team violates the Dead Programs Tell No Lies principle. Bugs are really hard to find as it is. A method that silently changes its behavior based on an earlier problem, not only makes it hard to find the first bug but also adds a 2nd bug of its own.

The junior wreaked havoc by not checking for a null. What if this piece of code wreaks havoc by continuing to run in an undefined state (device is on but the program "thinks" it's off)? Perhaps another part of the program will do something that is only safe when the device is off.

Either of these approaches will avoid silent failures:

1. Use asserts as suggested by this answer, but make sure they are turned on in production code. This, of course, could cause problems if other asserts were written with the assumption that they would be off in production.

2. Throw an exception if it is null.

Answer 8

I agree with trapping the null in the constructor. Further, if the member is declared in the header as:

IMsgSender* const msgSender_;

Then the pointer cannot be changed after initialisation, so if it was fine at construction it will be fine for the lifetime of the object containing it. (The object pointed to will not be const.)

Answer 9

This is Outright Dangerous!

I worked under a senior developer in a C codebase with the shoddiest "standards" who pushed for the same thing, to blindly check all pointers for null. The developer would end up doing things like this:

// Pre: vertex should never be null.
void transform_vertex(Vertex* vertex, ...)
{
    // Inserted by my "wise" co-worker.
    if (!vertex)
        return;
    ...
}

I once tried removing such a check of the precondition one time in such a function and replacing it with an assert to see what would happen.

To my horror, I found thousands of lines of code in the codebase which were passing nulls to this function, but where the developers, likely confused, worked around and just added more code until things worked.

To my further horror, I found this issue was prevalent in all sorts of places in the codebase checking for nulls. The codebase had grown over decades to come to rely on these checks in order to be able to silently violate even the most explicitly-documented preconditions. By removing these deadly checks in favor of asserts, all the logical human errors over decades in the codebase would be revealed, and we would drown in them.

It only took two seemingly-innocent lines of code like this + time and a team to end up masking a thousand accumulated bugs.

These are the kinds of practices that make bugs depend on other bugs to exist in order for the software to work. It's a nightmare scenario. It also makes every logical error related to violating such preconditions show up mysteriously a million lines of code away from the actual site in which the mistake occurred, since all these null checks just hide the bug and hide the bug until we reach a place that forgot to hide the bug.

To simply check for nulls blindly in every place where a null pointer violates a precondition is, to me, utter insanity, unless your software is so mission-critical against assertion failures and production crashes that the potential of this scenario is preferable.

Is it reasonable for a coding standard to require that every single pointer dereferenced in a function be checked for NULL first—even private data members?

So I'd say, absolutely not. It's not even "safe". It may very well be the opposite and mask all kinds of bugs throughout your codebase which, over the years, can lead to the most horrific scenarios.

assert is the way to go here. Violations of preconditions should not be allowed to go unnoticed, or else Murphy's law can easily kick in.

Answer 10

Objective-C, for example, treats every method call on a nil object as a no-op that evaluates to a zero-ish value. There are some advantages to that design decision in Objective-C, for the reasons suggested in your question. The theoretical concept of null-guarding every method call has some merit if it's well publicized and consistently applied.

That said, code lives in an ecosystem, not a vacuum. The null-guarded behaviour would be non-idiomatic and surprising in C++, and therefore should be considered harmful. In summary, nobody writes C++ code that way, so don't do it! As a counter-example, though, note that calling free() or delete on a NULL in C and C++ is guaranteed to be a no-op.

---

In your example, it would probably be worthwhile to place an assertion in the constructor that msgSender is non-null. If the constructor called a method on msgSender right away, then no such assertion would be necessary, since it would crash right there anyway. However, since it is merely storing msgSender for future use, it would not be obvious from looking at a stack trace of SignalShutdown() how the value came to be NULL, so an assertion in the constructor would make debugging significantly easier.

Even better, the constructor should accept a const IMsgSender& reference, which cannot possibly be NULL.

Answer 11

The reason that you're asked to avoid null dereferences is to ensure that your code is robust. Examples of junior programmers long long ago are just examples. Anyone can break the code by accident and cause a null dereference - especially for globals and class globals. In C and C++ it's even more possible accidentally, with the direct memory management capability. You might be surprised, but this kind of thing happens very often. Even by very knowledgeable, very experienced, and very senior developers.

You don't need to null check everything, but you do need to protect against dereferences that have decent likelihood of being null. This is commonly when they are allocated, used and dereferenced in different functions. It is possible that one of the other functions will be modified and break your function. It is also possible that one of the other functions may be called out of order (like if you have a deallocator that can be called separately from the destructor).

I prefer the approach your coworkers are telling you in combination with using assert. Crash in the test environment so it's more obvious that there's a problem to fix and fail gracefully in production.

You should also be using a robust code correctness tool like coverity or fortify. And you should be addressing all compiler warnings.

Edit: as others have mentioned, silently failing, as in several of the code examples, is generally the wrong thing, as well. If your function cannot recover from the value being null, it should return an error (or throw an exception) to its caller. The caller is responsible for then either fixing its call order, recovering, or returning an error (or throwing an exception) to its caller, and so on. Eventually, either a function is able to gracefully recover and move on, gracefully recover and fail (such as a database failing a transaction because of an internal error for one user but not acutally exiting), or the function determines that the application state is corrupt and unrecoverable and the application exits.

Answer 12

The use of a pointer instead of a reference would tell me that msgSender is only an option and here the null check would be right. The code snippet is too slow as to decide that. Maybe there are other elements in PowerManager that are valuable (or testable)...

When choosing between pointer and reference, I thoroughly weigh both options. If I have to use a pointer for a member (even for private members), I have to accept the if (x) procedure each time I dereferencing it.

Answer 13

It depends on what you want to happen.

You wrote that you are working on "a consumer electronics device". If, somehow, a bug is introduced by setting msgSender_ to NULL, do you want

• the device to carry on, skipping SignalShutdown but continuing the rest of its operation, or
• the device to crash, forcing the user to restart it?

Depending on the impact of the shutdown signal not being sent, option 1 might be a viable choice. If the user can continue to listen to his music, but the display still shows the title of the previous track, that might be preferable to a complete crash of the device.

Of course, if you choose option 1, an assert (as recommended by others) is vital to reduce the likelihood of such a bug creeping in unnoticed during development. The if null guard is just there for failure mitigation in production use.

Personally, I also prefer the "crash early" approach for production builds, but I am developing business software that can be fixed and updated easily in case of a bug. For consumer electronics devices, this might not be so easy.