No, it’s not the control flow. Sure, we could replace the conditional with a guard clause and get rid of some indentation, but the problem is more subtle than that. Most people are blind to it because it’s in their face every day.

The problem is this line:

if (flags != null)

Spurious null checks are a symptom of bad code.

That’s not to say that null checks are wrong. If a vendor gives you a library that can return null, you’re obliged to check for null. And, if people are passing null all over the place in your code, it makes sense to keep putting some null checks in, but, you know what? That just means that you’re dealing with bad code: you're dealing with code where people are actively making extra work for themselves and making code brittle in the process.

I’ve gotten more diplomatic over the years, but in one my early programming jobs, the guy who hired me called me over to his office and showed me a scheme that he’d created for our classes. Every method that we called could set a flag if it determined that it was passed a bad parameter. After a client called a method, it could check the flag and determine whether the call was successful or not. I said “Oh, cool, we can call it the ‘was I stupid flag!’” He gave me a withering look I’ll never forget, but he saw the point despite my youthful obnoxiousness: it was kind of silly to check afterwards if you could check before. Why check for an error afterwards if you can avoid it to begin with?

Passing null in production code is a very bad idea. It’s an example of what I call offensive programming – programming in way that encourages defensive programming.
The better way to code is to deal with nulls as soon as they happen and translate them to null objects or exceptions. Unfortunately, this isn’t common knowledge.

Let’s look at an example.

Here’s a method that builds a set of parameters from an array of integers:

If we don’t have any parameters, we return an empty array. The empty array is an example of a null object.. an object which gracefully handles an exceptional case. Now that we're always returning a valid array, there's one less possible error.

When we program we always have the choice to program in a way which forces us to check for errors later or not. The alternative is finesse the problem, to design in a way that makes errors impossible. Making errors impossible is far better than handling them.

What I think is good about your example is that it shows something that I often find to be true: that often the 'correct' way to write the code is simpler than the offensive version.

At my current employer I find that when I maintain a class, after cleaning up the formatting, I delete at least 10% of the members and method code because it's unecessary and very often causes problems.

Another example of this issue in Java is when a method returns a collection type and there several concerns:

The caller needs a modifiable collection for their use. Trying to modify the returned collection could unintentionally modify the state of the source of the collection. If the source has wrapped the collection in a unmodifiable wrapper trying to modify it will result in a runtime exception.

The collection may be 'live' meaning changes to the source could modify the state of the returned collection. This can be true even if the returned collection is unmodifiable to the caller.

A quick solution to all of these is to take the returned collection and all all the elements to a new collection. The question is whether you do this in a defensive way or if the source should do this upfront. If one way is not chosen, you can end up doing it on both ends.

> What I think is good about your example is that it shows> something that I often find to be true: that often the> 'correct' way to write the code is simpler than the> offensive version.

Yeah, it seems that way to me too, but so much of it seems to be insight. When we have it, we can see the simpler thing. Without it, it all becomes brute force.

> At my current employer I find that when I maintain a> class, after cleaning up the formatting, I delete at least> 10% of the members and method code because it's unecessary> and very often causes problems.> > Another example of this issue in Java is when a method> returns a collection type and there several concerns:> > Iteration requires explicit synchronization which when> done can cause contention issues.> > The caller needs a modifiable collection for their use.> Trying to modify the returned collection could> d unintentionally modify the state of the source of the> collection. If the source has wrapped the collection in a> unmodifiable wrapper trying to modify it will result in a> runtime exception.> > The collection may be 'live' meaning changes to the source> could modify the state of the returned collection. This> can be true even if the returned collection is> unmodifiable to the caller.> > A quick solution to all of these is to take the returned> collection and all all the elements to a new collection.> The question is whether you do this in a defensive way or> r if the source should do this upfront. If one way is not> chosen, you can end up doing it on both ends.

I just implicitly recoil from the idea of returning live collections of my innards. It can work if everyone understands that it is happening, but the potential for confusion is great.

I have this theory that the thing that differentiates good code bases from bad code bases is the fact that the good code bases have rules. Often they're not explicit, but they are easy to discern. I think it all goes back to the principle of least astonishment.

> I just implicitly recoil from the idea of returning live> collections of my innards. It can work if everyone> understands that it is happening, but the potential for> confusion is great.

Yeah, I usually return a copy of the collection or if there's a really good reason to return a live collection, make it unmodifiable. One of the nice things about generics is that you can actually make the immutablity of your collection part of the API definition. This still doesn't solve the iterator synchronization issue though. Really, I think that's a result of a poor API design. The iterator should not be returned, rather a handler for each Object should be passed to the collection.

> I have this theory that the thing that differentiates good> code bases from bad code bases is the fact that the good> code bases have rules. Often they're not explicit, but> they are easy to discern. I think it all goes back to the> principle of least astonishment.

The problem I find with this is getting the rules together. My experience is that it's hard to get people to agree on the rules. I've also had the experience of coming into an organization where a lot of the rules were just wrong. For example, the coding standard said 'always use import package.*;' and that you could never import specific classes.

So I agree that rules help, It's getting the rules in place that seems to be the trouble.

You gave an example of a private method but let's assume it is a public API method called all over the place. Is it really better that each client check the parameter before calling the method, instead of the method generically checking the input parameter? Certainly not! This method is eactly the right place to make the null check because it knows best what to do. It can throw an exception to tell the caller that they have made a serious mistake, or it can simply tolerate the null if it doesn't matter. Which btw is very often the case. I don't agree that null pointers should never be passed around. Null is a legitimate object. I agree however that excessive null checks stink. I prefer a coding style that concentrates null checks in as few places as possible. By far the most common null check is for strings, and usually it doesn't matter whether a string is null or empty. I absolutely hate seeing

Btw i would like to have a compiler-enforced not-null qualifier, as in Nice, for the cases where null is deemed illegal. As long as this doesn't exist, relying on clients to always pass valid parameters is a dangerous strategy. As Rusty Harold put it: "Classes are responsible for enforcing their class invariants, not client programmers. Nobody who uses the public methods of a class should be assumed to understand what is and is not legal."

> You gave an example of a private method but let's assume> it is a public API method called all over the place. Is it> really better that each client check the parameter before> calling the method, instead of the method generically> checking the input parameter?

I agree with you to a point but I think what Michael is saying is that neither would check for null.

I agree that nulls can be meaningful but in the case of collections, this is rarely the case.

One of the ideas I've had for a OO language (that may already exist) would be a syntax for when 'this == null' allowing a behavior to be defined for when a method is called against a null but of course this wouldn't be polymorphic...

My theory is that code like this is often a symptom of the 'single return' philisophy that is often espoused by university professors. Instead of checking the parameters up front and aborting if they are bad, the whole method is contorted to fufill this ideological requirement.

.> > Btw i would like to have a compiler-enforced not-null> qualifier, as in Nice, for the cases where null is deemed> illegal. As long as this doesn't exist, relying on clients> to always pass valid parameters is a dangerous strategy.> As Rusty Harold put it: > "Classes are responsible for enforcing their class> ass invariants, not client programmers. Nobody who uses> the public methods of a class should be assumed to> understand what is and is not legal."> > http://www.cafeconleche.org/XOM/designprinciples.xhtml

I disagree with that advice because I don't see all design as API design. I think it's unfortunate that much of the design advice that we see is geared towards API design and not application design.

In this specific case, I think that for most programming we should be able to assume that people aren't going to pass null parameters. To the degree that we can't, the application is messed up. It's just a matter of sanity.

> You gave an example of a private method but let's assume> it is a public API method called all over the place. Is it> really better that each client check the parameter before> calling the method, instead of the method generically> checking the input parameter? Certainly not! This method> is eactly the right place to make the null check because> it knows best what to do. It can throw an exception to> tell the caller that they have made a serious mistake, or> it can simply tolerate the null if it doesn't matter.> Which btw is very often the case. I don't agree that null> pointers should never be passed around. Null is a> legitimate object.

It's a sick little object, in my opinion. It makes programs go boom. NULL is even worse in C and C++.

> I agree however that excessive null> checks stink. I prefer a coding style that concentrates> null checks in as few places as possible.

I think that, again, it comes down to whether you're writing an API or not. If clients of your code are under your control, you can establish a safe subset coding convention: pass only non-null objects.

> "Making errors impossible is far better than handling> them." Well said, but please tell us how to "make errors> impossible"!

I don't think there is any algorithm for this. The thing I did above removes a possible error. The same thing can be said for using collection iterators rather than arrays, or using closures rather than collection iterators. I think that the key thing is to think of possible errors and wonder why they are there. Sometimes this involves going all the way back to requirements.

I was working on some code recently where I had to do some error checking because of an invalid state and I realized that it could be traced back to a particular requirement. If that requirement was altered a bit the error would be impossible. It didn't work out in that case, but when you do have control over the feature set, in something like new product development, you can often finesse away possible errors, so it's worth looking at.

In Cocoa / Objective-C programming, I still prefer an empty string over a null pointer, or an empty container-object over a null object, but the language does have a key difference: calling object methods on null-pointers does not normally crash! (But sometimes method parameters being null can cause a crash.)

Foo* aFoo = nil;Bar* aResult = [aFoo someMethod: anArgument ];

calling someMethod (as above) on a nil object DOES NOTHING (it does not crash or throw an exception) and returns nil.

The Cocoa UI frameworks take advantage of this. For example, the objects that implements a Button or a Menu item would have two members variables: a pointer to an object, and a "selector" that identifies what method to call on the object. The method that handles a click (or whatever) would be written something like this:

No need to check for a null targetObject. There would be a need to check for a null-selector (in the performSelector:withArgument: method), since the selector isn't actually an object.

People have objected that null acting like the "null object pattern" hides bugs, but too many times have I gotten web-apps sending me messages that a "null pointer exception has occurred" so I assert that null-pointer exceptions are not helping people find bugs as well as test-driven development, or thorough code-reviews and testing, would do. I expect that if null had the "null object" behavior, the web-app would have returned an partial or empty result rather than a crashing message, and that would be fine for a most users.

If the "null = null object pattern" behavior is an expected and documented part of the language, it can work quite well. Cocoa and NextStep programmers are, and have been, very productive using a language with this behavior.

Null objects and similar cases are definitely a problem in code as it leads to too many checks. Certainly, in your own code you should strive to check arguments at the public method level and keep it sane inside.

But equally often I find it detrimental to work with basic objects like String, int, etc. since in most applications (unless it is basic API design) we should be dealing with domain-specific objects, which replace primitive objects like String, int, etc. Each value objects should have a defined domain of values, which normally would exclude null in almost all cases (or wrap it into a special value object a la NullObject pattern).

As soon as these values are agreed, you can forget all null values since null values are now clearly programming errors and should never (famous last words :) make it into production. At least that's would I try to strive for in code in my projects.

I generally agree with the idea of trapping mistakes as early as possible. In your example code I see this as a logic error rather than a data error. In other words, it is illogical to pass a null object to this function; the function has the right to expect that only reasonable items will be passed to it. Of course, that situation would be different if the data being passed came directly from user-entered values, but in this case it comes from some other function that ought to be validating data prior to passing it along.

However, even though your example code is a private member, some form of defensive coding could be warranted in order to catch your own mistakes. Those languages that implement some form of Design by Contract (e.g. Eiffel and D) can help here.

the in block is only executed in non-release editions of the application which allows you to perform the defensive code during in-house testing and to leave it out when the release edition of the application is built (using the -release switch).

> <p>What’s wrong with it?</p>> > <p>No, it’s not the control flow. Sure, we could replace> the conditional with a guard clause and get rid of some> indentation, but the problem is more subtle than that.> Most people are blind to it because it’s in their face> e every day.</p>> > <p>The problem is this line:</p>> > <pre>if (flags != null)</pre>> > <p>Spurious null checks are a symptom of bad code.</p>

No, the problem with nulls is not their existence, but that there are not separate types. In functional programming languages, there is not a 'null' value, but there is 'nil', and the typechecker/pattern matching can catch that and create separate versions of code. For example, a maybe type can be defined like this:

type maybe T = T | nil

And it can be used like this:

fun doSomething T = bla bla handle case | nil = bla bla handle null

So the property of 'maybe something exists' is encoded in the type system, and thus there is no need to do null checks or anything: if a pattern fails to match at compile time, the compiler will display an error.

It also demonstrates the fact that values are types and not only groups of values. For example, the factorial function has two types:

fun factorial n = n * factorial (n - 1) | 0 = 1

The above makes it clear that when it comes to factorial, number 0 is a different type than the rest of the numbers.

Many bugs in programs in the above mentioned languages are a result of values not being types. The knowledge of what values are allowed is carried within the head of the programmer, never given to the compiler...and thus compilers silently ignore many obvious errors.

If you had a rule "no nulls allowed", wouldn't you just be, in many cases, replacing logic like:

if(x != null)

with:

if(x.length() > 0)

or

if(x.length > 0)

or

if(x.size() > 0)

?

Certainly in some cases you are just iterating through the elements of the collection or array and if it's empty your code still works. But not always. Many times an empty collection/array means something special, and checking for it is no different than checking for null. Many times a zero-length string means the same as a null string, and requires special handling.

Another issue I see is that nulls go "all the way down". If you create an object with instance members, those members default to null. If you construct them, their members default to null. Either you are initializing all objects with all their instance members created as empty objects with default values (and zero-length strings), or you have to deal with null at some point. What about lazy instantiation/initialization?