How much null checking is enough?

Question

What are some guidelines for when it is not necessary to check for a null?

A lot of the inherited code I've been working on as of late has null-checks ad nauseam. Null checks on trivial functions, null checks on API calls that state non-null returns, etc. In some cases, the null-checks are reasonable, but in many places a null is not a reasonable expectation.

I've heard a number of arguments ranging from "You can't trust other code" to "ALWAYS program defensively" to "Until the language guarantees me a non-null value, I'm always gonna check." I certainly agree with many of those principles up to a point, but I've found excessive null-checking causes other problems that usually violate those tenets. Is the tenacious null checking really worth it?

Frequently, I've observed codes with excess null checking to actually be of poorer quality, not of higher quality. Much of the code seems to be so focused on null-checks that the developer has lost sight of other important qualities, such as readability, correctness, or exception handling. In particular, I see a lot of code ignore the std::bad_alloc exception, but do a null-check on a new.

In C++, I understand this to some extent due to the unpredictable behavior of dereferencing a null pointer; null dereference is handled more gracefully in Java, C#, Python, etc. Have I just seen poor-examples of vigilant null-checking or is there really something to this?

This question is intended to be language agnostic, though I am mainly interested in C++, Java, and C#.

---

Some examples of null-checking that I've seen that seem to be excessive include the following:

---

This example seems to be accounting for non-standard compilers as C++ spec says a failed new throws an exception. Unless you are explicitly supporting non-compliant compilers, does this make sense? Does this make any sense in a managed language like Java or C# (or even C++/CLR)?

try {
   MyObject* obj = new MyObject(); 
   if(obj!=NULL) {
      //do something
   } else {
      //??? most code I see has log-it and move on
      //or it repeats what's in the exception handler
   }
} catch(std::bad_alloc) {
   //Do something? normally--this code is wrong as it allocates
   //more memory and will likely fail, such as writing to a log file.
}

---

Another example is when working on internal code. Particularly, if it's a small team who can define their own development practices, this seems unnecessary. On some projects or legacy code, trusting documentation may not be reasonable... but for new code that you or your team controls, is this really necessary?

If a method, which you can see and can update (or can yell at the developer who is responsible) has a contract, is it still necessary to check for nulls?

//X is non-negative.
//Returns an object or throws exception.
MyObject* create(int x) {
   if(x<0) throw;
   return new MyObject();
}

try {
   MyObject* x = create(unknownVar);
   if(x!=null) {
      //is this null check really necessary?
   }
} catch {
   //do something
}

---

When developing a private or otherwise internal function, is it really necessary to explicitly handle a null when the contract calls for non-null values only? Why would a null-check be preferable to an assert?

(obviously, on your public API, null-checks are vital as it's considered impolite to yell at your users for incorrectly using the API)

//Internal use only--non-public, not part of public API
//input must be non-null.
//returns non-negative value, or -1 if failed
int ParseType(String input) {
   if(input==null) return -1;
   //do something magic
   return value;
}

Compared to:

//Internal use only--non-public, not part of public API
//input must be non-null.
//returns non-negative value
int ParseType(String input) {
   assert(input!=null : "Input must be non-null.");
   //do something magic
   return value;
}

Answer 1

One thing to remember that your code that you write today while it may be a small team and you can have good documentation, will turn into legacy code that someone else will have to maintain. I use the following rules:

1. If I'm writing a public API that will be exposed to others, then I will do null checks on all reference parameters.

2. If I'm writing an internal component to my application, I write null checks when I need to do something special when a null exists, or when I want to make it very clear. Otherwise I don't mind getting the null reference exception since that is also fairly clear what is going on.

3. When working with return data from other peoples frameworks, I only check for null when it is possible and valid to have a null returned. If their contract says it doesn't return nulls, I won't do the check.

Answer 2

NULL checking in general is evil as it's add a small negative token to the code testability. With NULL checks everywhere you can't use "pass null" technique and it will hit you when unit testing. It's better to have unit test for the method than null check.

Check out decent presentation on that issue and unit testing in general by Misko Hevery at http://www.youtube.com/watch?v=wEhu57pih5w&feature=channel

Answer 3

If I receive a pointer that is not guaranteed by language to be not null, and am going to de-reference it in a way that null will break me, or pass out put my function where I said I wouldn't produce NULLs, I check for NULL.

It is not just about NULLs, a function should check pre- and post-conditions if possible.

It doesn't matter at all if a contract of the function that gave me the pointer says it'll never produce nulls. We all make bugs. There's a good rule that a program shall fail early and often, so instead of passing the bug to another module and have it fail, I'll fail in place. Makes things so much easier to debug when testing. Also in critical systems makes it easier to keep the system sane.

Also, if an exception escapes main, stack may not be rolled up, preventing destructors from running at all (see C++ standard on terminate()). Which may be serious. So leaving bad_alloc unchecked can be more dangerous than it seems.

Fail with assert vs. fail with a run time error is quite a different topic.

Checking for NULL after new() if standard new() behavior has not been altered to return NULL instead of throwing seems obsolete.

There's another problem, which is that even if malloc returned a valid pointer, it doesn't yet mean you have allocated memory and can use it. But that is another story.

Answer 4

Lower level code should check use from higher level code. Usually this means checking arguments, but it can mean checking return values from upcalls. Upcall arguments need not be checked.

The aim is to catch bugs in immediate and obvious ways, as well as documenting the contract in code that does not lie.

Answer 5

I don't think it's bad code. A fair amount of Windows/Linux API calls return NULL on failure of some sort. So, of course, I check for failure in the manner the API specifies. Usually I wind up passing control flow to an error module of some fashion instead of duplicating error-handling code.

Answer 6

It's widely known that there are procedure-oriented people (focus on doing things the right way) and results-oriented people (get the right answer). Most of us lie somewhere in the middle. Looks like you've found an outlier for procedure-oriented. These people would say "anything's possible unless you understand things perfectly; so prepare for anything." For them, what you see is done properly. For them if you change it, they'll worry because the ducks aren't all lined up.

When working on someone else's code, I try to make sure I know two things.
1. What the programmer intended
2. Why they wrote the code the way they did

For following up on Type A programmers, maybe this helps.

So "How much is enough" ends up being a social question as much as a technical question - there's no agreed-upon way to measure it.

(It drives me nuts too.)