Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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.
share|improve this question
11  
Don't think for a minute that mistakes are the domain of junior programmers. 95% of developers of all levels of experience goof something up once in awhile. The remaining 5% are lying through their teeth. –  Blrfl Nov 2 '13 at 17:09
49  
If I saw someone write code that checks for a null and silently fails, I'd want to fire them on the spot. –  Winston Ewert Nov 2 '13 at 17:10
13  
Stuff failing silently is pretty much the worst. At least when it blows up you know where the explosion occurs. Checking for null and doing nothing is just a way to move the bug on down the execution, making it far more difficult to track back to the source. –  MirroredFate Nov 3 '13 at 0:28
1  
@WinstonEwert There are cases where you want a silent fail. My current tech stack has several. Immediate fire is a bit harsh but in general programming you don't want a silent fail. Some models want it though. –  Rig Nov 3 '13 at 22:40
4  
@Rig, I'm sure that there are appropriate cases for a silent fail. But if someone puts silent fails in as general practice (unless working in a realm where it make sense), I really don't want to be working on a project they are putting code in. –  Winston Ewert Nov 3 '13 at 22:59

12 Answers 12

up vote 48 down vote accepted

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.

share|improve this answer
4  
+1 for a very succinct answer that pretty much sums up the way I feel: "it depends..." You also had the guts to say "never check for null" (if null is invalid). Placing an assert() in every member function that dereferences a pointer is a compromise that will likely placate a lot of people, but even that feels like 'speculative paranoia' to me. The list of things beyond my ability to control is infinite, and once I allow myself to start worrying about them, it becomes a really slippery slope. Learning to trust my tests, my colleagues—and my debugger—has helped me sleep better at night. –  evadeflow Nov 2 '13 at 15:29
1  
When you're reading code those asserts can be very helpful in understanding how the code is supposed to work. I've never encoutered a codebase that made me go 'I wish it didn't have all these asserts all over the place.', but I've seen plenty where I said the opposite. –  Kristof Provost Nov 2 '13 at 17:45
1  
Plain and simple, this is the correct answer to the question. –  Matthew Azkimov Nov 2 '13 at 18:55
2  
This is the closest correct answer but i can't agree with 'never check for null', always check for invalid parameters at the point they'll be assigned to a member variable. –  James Nov 2 '13 at 19:08
    
Thanks for the comments. It is better to let it crash sooner if someone doesn't read the specification and initializes it with null value when it must have something valid there. –  aaronps Nov 3 '13 at 2:31

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.

share|improve this answer
9  
The big difference between calling assert and the OP's code example is that assert is only enabled in debug builds (#define NDEBUG) When the product gets into customer hands, and debug is disabled, and a never-yested combination of code paths executes to leave the pointer null even though the function in question is executed, the assert provides you no protection. –  atk Nov 2 '13 at 12:09
14  
Presumably you'd have tested the build before actually sending it to the customer, but yes, this is a possible risk. Solutions are easy though: either just ship with asserts enabled (that's the version you tested anyway) or replace it with your own macro which asserts in debug mode and just logs, logs+backtraces, exits, ... in release mode. –  Kristof Provost Nov 2 '13 at 13:01
5  
@James - in 90% of case you won't recover from the failed NULL check as well. But in such case the code will silently fail and error may appear much later on - for example you thought that you saved to file but actually the null check sometimes fails leaving you non the wiser. You cannot recover from all situation that must not happen. You can verify that they won't happen, but I don't think you have a budget unless you write code for NASA satellite or nuclear power plant. You do need to have a way of saying "I have no idea what's going on - the program is not suppose to be in this state". –  Maciej Piechotka Nov 2 '13 at 20:14
4  
@James I disagree with throwing. That makes it look like it's something that can actually happen. Asserts are for things which are supposed to be impossible. For real bugs in the code in other words. Exceptions are for when unexpected, but possible, things happen. Exceptions are for things you could conceivable handle and recover from. Logic errors in the code you can't recover from, and you shouldn't try either. –  Kristof Provost Nov 3 '13 at 9:22
2  
@James: In my experience with embedded systems, the software and hardware are invariably designed such that is is extremely hard to render a device completely unusable. In the large majority of cases, there is a hardware watchdog that forces a complete reset of the device if the software stops running. –  Bart van Ingen Schenau Nov 3 '13 at 11:31

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.

share|improve this answer
5  
+1 for using references in preference to never-null pointers when practical. –  timday Nov 2 '13 at 11:33

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.
share|improve this answer

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.

share|improve this answer

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.

share|improve this answer

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 :)

share|improve this answer
1  
There's actually nothing wrong with using raw pointers sometimes. Std::shared_ptr is not a silver bullet and using it in an argument list is a cure for sloppy engineering although I realise it's considered 'leet' to use it whenever possible. Use java if you feel like that. –  James Nov 3 '13 at 3:22
1  
I didn't say raw pointers are bad! They definitely have their place. However, this is C++, references (and shared pointers, now) are part of the language for a reason. Use them, they will cause you to succeed. –  Seth Nov 3 '13 at 3:54
    
I like the smart pointer idea, but it's not an option in this particular gig. +1 for recommending the use of references (as @Max and a few others have). Sometimes it's not possible to inject a reference, though, due to chicken-and-egg dependency issues, i.e., if an object that would otherwise be a candidate for injection needs to have a pointer (or reference) to its holder injected into it. This can sometimes indicate a tightly coupled design; but it's often acceptable, as in the relationship between an object and its iterator, where it never makes sense to use the latter without the former. –  evadeflow Nov 3 '13 at 14:35

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.

share|improve this answer

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.)

share|improve this answer
    
That is great advice, thank you! So good, in fact, that I'm sorry I can't vote it up any higher. It's not a direct answer to the stated question, but it is a great way to eliminate any possibility of the pointer 'accidentally' becoming NULL. –  evadeflow Nov 5 '13 at 15:17
    
@evadeflow I thought "I agree with everyone else" answered the main thrust of the question. Cheers though! ;-) –  Grimm The Opiner Nov 5 '13 at 15:34
    
It would be a little impolite of me to change the accepted answer at this point, given the way the question was phrased. But I really think this bit of advice is outstanding, and I'm sorry I didn't think of it myself. With a const pointer, there's no need to assert() outside the constructor, so this answer seems even a bit better than @Kristof Provost's (which was also outstanding, but addressed the question somewhat indirectly as well.) I hope others will vote this up, since it really doesn't make sense to assert in every method when you can just make the pointer const. –  evadeflow Nov 5 '13 at 23:05
    
@evadeflow People only visit Questins for the rep, now it's been answered no-one'll look at it again. ;-) I only popped in because it was a question about pointers and I'm keeping an eye out for the bloody "Use smart pointers for everything always" brigade. –  Grimm The Opiner Nov 6 '13 at 8:42

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.

share|improve this answer
    
+1 for having the courage to support a (seemingly) unpopular position. I would've been disappointed if nobody defended my colleagues on this (they're very smart people who have been putting out a quality product for many years). I also like the idea that apps should "crash in the test environment... and fail gracefully in production." I'm not convinced that mandating 'rote' NULL checks are the way to do that, but I appreciate hearing an opinion from someone outside my workplace who's basically saying: "Aigh. Doesn't seem too unreasonable." –  evadeflow Nov 2 '13 at 14:57
    
I get annoyed when I see people testing for NULL after malloc(). It tells me they don't understand how modern OSs manage memory. –  Kristof Provost Nov 2 '13 at 18:52
2  
My guess is that @KristofProvost is talking about OSes that use overcommit, for which malloc always succeeds (but the OS may later kill your process if there isn't actually enough memory available). However, this is not a good reason to skip null checks on malloc: overcommit is not universal across platforms and even if it's enabled, there may be other reasons for malloc to fail (e.g., on Linux, a process address space limit set with ulimit). –  John Bartholomew Nov 2 '13 at 20:31
1  
What John said. I was indeed talking about overcommit. Overcommit is enabled by default on Linux (which is what pays my bills). I don't know, but suspect so, if it's the same on Windows. In most cased you're going to end up crashing anyway, unless you're prepared to write a lot of code that's never, ever going to be tested to handle errors that are almost certainly never going to happen... –  Kristof Provost Nov 2 '13 at 21:02
2  
Let me add that I've also never seen code (outside of the linux kernel, where out of memory is actuall realisticly possible) that will actually correctly handle an out-of-memory condition. All of the code that I've seen try would just crash later anyway. All that accomplished was to waste time, make the code harder to understand and hide the real problem. Null dereferences are easy to debug (if you have a core file). –  Kristof Provost Nov 2 '13 at 21:24

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.

share|improve this answer
    
As I mentioned in another comment somewhere, there are times when injecting a reference isn't possible. An example I run into a lot is where the held object itself needs a reference to the holder: –  evadeflow Nov 5 '13 at 17:33
    
@evadeflow: OK, I confess, it depends on the class size and the visibility of pointer members (that cannot be made references), whether I check before dereferencing. In cases where the class is small and plain and the pointer(s) are private, I don't... –  Wolf Nov 5 '13 at 18:50

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.