A Null Reference Exception bug is reported. You check the code and find the problem happened because a public field wasn't set. You avoid the bug by modifying the code to check for a null. And you make it home in time for dinner.
Original Code:
myClass.MyField.MyMethod(); // Gets NullReferenceException on MyField
“Corrected” Code:
// Avoids NPE
if ( myClass.MyField != null ) { myClass.MyField.MyMethod();
}
Seems like a reasonable solution. But guess what–this is nearly always the wrong solution!
There are several reasons why this would be the wrong solution:
- Why is MyField null in the first place? Perhaps the proper fix is to find out why it wasn't set to a real value and fix that case.
- If MyField can legitimately be null, then you need to check the entire code base to make sure MyField is always checked for null. And in a sufficiently large code base, your code is very brittle because you can never be sure that you found all the places. And more importantly, in the future, will a new hire know to do the null check first? Likely not.
Your code is more complicated than it needs to be and is therefore prone to bugs.
What to do? Before putting in a null-check of a public field, always ask yourself two questions:
- Is there a good reason why this field is null? If so, see "Case 2: The Field can be null" (below). If it can't, then research the bug some more and fix the real problem.
- What's the right thing to do if it is null? Don't ignore this case. You may be able to recover, or you may have to throw an exception. But proactively do something. Don't just Assert, because that is a debug-only check. You need to specify what the production code should do. [See my earlier post: CodeGaffe: Clueless Comments - Never Say Never]
Let's look at a good way to solve both issues:
Case 1: The Field should never be null
If this field should never be null, then the null-check is trying to fix the bug in the wrong place. And it will come back to haunt you.
First of all, if MyField shouldn't be null, then putting in defensive code to check for null is bad because you'll need to add an ELSE to it. What should the code do if the pointer is null? There's often nothing good you can accomplish, and what usually ends up happening is the code HIDES the null reference exception. This makes it harder to diagnose future failures because hiding the exception will likely cause another failure much later in the code (the symptom is far from the cause).
Secondly, if MyField shouldn't be null–go find out why it was null and fix that problem. Make sure the field is properly set or you never reach this code when it is null.
Case 2: The Field can be null
If the field can be null, then you need to centralize the null-check. That's the real issue you need to deal with. There are a couple of ways to do this, each appropriate for different situations.
Solution A: Make MyField private
If the only reason MyField is exposed is to call its method, then you should hide MyField and create a wrapper method. That follows the Law of Demeter (http://en.wikipedia.org/wiki/Law_of_Demeter). The Law of Demeter reduces dependencies, and even reduces bugs. Stated another way, to obey the Law of Demeter means resisting the temptation to use multiple dots in our code. So, for example, instead of using myClass.MyField.MyMethod() we add a pass-thru method to MyClass so we can simply say myClass.MyMethod(). The definition of our pass-thru method looks like this:
public string MyMethod() { if ( MyField != null ) { return MyField.MyMethod();
}
else { // Do the right thing when MyField is null.
return "";
}
}
Since MyField can be null, there must be something reasonable we should be doing when it is. By consolidating it in the MyMethod() wrapper, we don't have to worry about people forgetting this check – we'll always do the right thing when it's null.
And, by making MyField private, you don't have to worry about a new hire coming along and writing the original code all over again without a null check, namely: myClass.MyField.MyMethod()
If MyField exposes a few methods/properties, write wrappers for all of them (you'll need that to make MyField private). Each one encapsulates the correct behavior for the null case.
However, if MyField must be a public property on MyClass, then you should consider this next solution instead. This might be the case if its value is often set externally, or if it has many methods – too many to write wrappers for, practically speaking.
Solution B: Convert MyField to a property and use the null-object pattern
Instead of having MyField be a public field, we convert it to a property. By doing this, we don't have to change any of the calling code. And, no one can get to MyField directly. Since they must go thru the property, we can guarantee that the right thing will be done – even in the future by a new person who joins the coding team.
Code Was:
Code Now is:
private Foo _MyField;
public Foo MyField
{ get
{ if ( _MyField != null )
{ return _MyField;
}
else
{ // Do the right thing when MyField is null.
return EmptyFoo;
}
}
set
{ // don't add Set until you actually need it _MyField = value;
}
}
This is the obvious choice when MyField is something like a string, because it may be perfectly appropriate to return "" when it's null. It's also the right choice if MyField must remain public.
If it's not a basic type, like string or int, then you'll have to return a null-object version of the object when MyField is null. What's the null-object pattern? It's an instance of MyField that responds to all its methods appropriately for the case where MyField would have been null. It is a way to encapsulate the correct behavior when MyField would be null – without having to do null-reference checks.
Some references for the null object pattern:
· Ward Cunningham: http://c2.com/cgi/wiki?NullObject
· Martin Fowler: http://www.refactoring.com/catalog/introduceNullObject.html
Summary
When making fixes, always take an extra minute and look ahead. Ask yourself:
· "Will this fix the real problem or is it just treating the symptom?"
· "Can I fix this in such a way that the issue won't come up again in some other form?"
Object Oriented programming is very powerful. Go enjoy your dinner.
More CodeGaffes: http://geekswithblogs.net/dchestnutt