Monday, December 29, 2008

Preventing NullPointerException

When a NullPointerException (NPE) is thrown, it can be hard to trace back to the bug that causes it, especially if it comes from an instance variable of a mutable class, where the execution of buggy code may have finished long before the NPE is thrown. With the wide adoption of the best practice of immutability and IoC container, it is not seen as frequently as before and most of the time it is easy to locate the bug, such as a missing setter injection in the Spring application context. Probably as a result, recently some developers seem to have relaxed on null checking as I have seem some codes in an open soure project that completely lacks both null checking and documentation of the preconditions on methods.

This week, I have seen two good practices (IMHO) of null checking and related documentation:

The first is a static method named T checkNotNull(T reference, String referenceName) in class Preconditions I found in the Activity Stream project. This class is modelled after a similar class from Google Collections API. Note that it does not share any of its source code.

This reminds me of the static methods void notNull(Object object) and void notNull(Object object, String message) from Validate class in Apache Commons Lang that are used a lot in my previous job for null checking:

import static org.apache.commons.lang.Validate.notNull;
...
public class Foo
{
private final Bar bar;

public Foo(Bar bar)
{
notNull(bar, "Bar must not be null.");
this.bar = bar;
...
}
...
}

The T checkNotNull(T reference, String referenceName) has the advantage of returning the parameter, thus it can be assigned right after the null checking:

import static com.atlassian.streams.util.Preconditions.checkNotNull;
...
public class Foo
{
private final Bar bar;

public Foo(Bar bar)
{
this.bar = checkNotNull(bar, "Bar");
...
}
...
}

Similar to null checking, quite often there is also a need to guard against empty String, blank String (containing only whitespaces, see StringUtils), empty Collection, null-containing Collection (containing a null element), empty Array, null-containing Array and empty Map, etc.

Luckily, Validate provides most of these checks:
  • void notEmpty(Collection collection, String referenceName)
  • void noNullElements(Collection collection, String referenceName)
  • void notEmpty(String string, String referenceName)
  • void notEmpty(Object[] array, String referenceName)
  • void noNullElements(Object[] array, String referenceName)
  • void notEmpty(Map map, String referenceName)

Unfortunately, they all return void and cannot be chained, and it can force you to write your own little static method to do all the checks, or use it before the proper check:

import static org.apache.commons.lang.Validate.*;
...
public class Foo extends Bar
{
public Foo(List list)
{
super(notEmptyNoNullElements(list));
}

private static notEmptyNoNullElements(Collection collection)
{
notEmpty(collection);
noNullElements(collection);
}
...
}

So I have added the following methods to Preconditions:
  • <T, C extends Collection<T>> C notEmpty(C collection, String name)
  • <T, C extends Collection<T>> C noNullElements(C collection, String name)
  • <T, C extends Collection<T>> C notEmptyNoNullElements(C collection, String name)
  • <T> T[] notEmpty(T[] array, String name)
  • <T> T[] noNullElements(T[] array, String name)
  • <T> T[] notEmptyNoNullElements(T[] array, String name)
  • <K, V> Map<K, V> notEmpty(Map<K, V> map, String name)
  • String notBlank(String text, String name)

The other example is actually in the Google Gadgets API. All the optional parameters are prefixed with "opt_", making it very obvious. So instead of just documenting in javadoc, we can also given a more intention-revealing name to parameters, such as:

/**
* Do something.
* @param bar Used to do something. Cannot be <code>null</code>. Mandatory...
* @param baz Used to do something. Can be <code>null</code>. Optional...
*/
public void foo(Bar bar, Baz optBaz)
{
...
}

Finally, Preconditions probably should not sit in streams-core. It would be more useful if it is moved to some core projects, such as atlassian-core, so that different teams do not have to reinvent the wheel.