88.

About This Bug Pattern

Signaling exceptional conditions by returning null values is not a good idea. Doing so limits control flow to the ordinary paths of method invocation and return, and buries evidence of where an exceptional condition occurred.

As you might have guessed, I call this pattern the Null Flag bug pattern because it is caused by the inconsistent use of null pointers as flags for exceptional conditions.

The Symptoms

A code block that uses null pointers as flags for exceptional conditions signals a NullPointerException.

The Cause

Let's consider the following simple bridge class from BufferedReaders to Iterators:

Listing 10-1: A Simple Bridge Class

 import java.io.BufferedReader; import java.io.InputStreamReader; import java.io.IOException; import java.util.Iterator; public class BufferedReaderIterator implements Iterator {   private BufferedReader internal;   public BufferedReaderIterator(BufferedReader _internal) {     this.internal = _internal;   }   public boolean hasNext() {     try {       boolean result = true;       // Let's suppose that lines in the underlying input stream are known       // to be no greater than 80 characters long.       internal.mark(80);       if (this.next() == null) {         result = false;       }       internal.reset();       return result;     }     catch (IOException e) {       System.err.println(e.toString());       return false;     }   }   public Object next() {     try {       return internal.readLine();     }     catch (IOException e) {       System.err.println(e.toString());       return null;     }   }   public void remove() {      // This iterator does not support the remove operation.      throw new UnsupportedOperationException();   } } 

Because this class serves as a bridge implementation of the Iterator interface, the code must catch IOExceptions from BufferedReader. Each of the methods handles IOExceptions by returning some default value. In the case of hasNext(), the value false is returned. This is reasonable: if an IOException is thrown, the client should not expect that another element can be retrieved from the Iterator.

On the other hand, next() simply returns null, both in the case of an IOException and-because it relies on the return value of internal.readLine()-in the case in which internal is empty. But this is not what the client of an Iterator object would expect.

Normally, when next() is called on an Iterator with no more elements, it throws a NoSuchElementException. If a client of our Iterator is relying on this behavior, it may likely attempt to dereference a null value returned from a call to next(), resulting in a NullPointerException.

Whenever a NullPointerException occurs, check for scenarios like the one described previously. Occurrences of this bug pattern are common.

Cures and Prevention

Despite the fact that it happens so frequently, the use of null flags is often unwarranted. Let's rewrite next() so that it throws NoSuchElementException, as expected:

Listing 10-2: Rewriting to Throw the Proper, Expected Exception

 public Object next() {   try {    String result = internal.readLine();    if (result == null) {     throw new NoSuchElementException();    }    else {     return result;    }   }   catch (IOException e) {   // The original exception is included in the message to notify the   // client that an IOException has occurred.   throw new NoSuchElementException(e.toString());  } } 

Notice that to make the rest of the code work with this altered method, we would also have to

1. Import java.util.NoSuchElementException.

2. Fix hasNext() so that it no longer calls next() to test. The simplest way to do this would be to simply call internal.readLine() directly.

An alternative to our handling of IOExceptions would be to catch them and throw RuntimeExceptions in their stead. Base your decision to do this on an assessment of how frequently you expect IOExceptions are expected on your target platform. If they will be common, then you might want to try recovering from them.

Any code that calls this new next() method may have to deal with thrown NoSuchElementExceptions. Of course, the code may simply choose to ignore them and allow the program to abort. If so, the resulting error message and the place where the exception was thrown would be much more informative than a NullPointerException thrown by the original code.

If the exception thrown were a checked exception (such as IOException), it would be even more helpful, because no client code for the class would compile unless it handled the exception. In this way, we could eliminate even the possibility of certain errors occurring before the program is ever run.

But, in this example, we couldn't throw such a checked exception without violating the Iterator interface. So we've sacrificed some static checking for the sake of reusing code that operates on Iterators. Trade-offs such as these, between the goals of static checking and reuse, are quite common.