Software developer, trainer, speaker (Java, JavaScript, Groovy, php, Cloud, DevOps, performance, architecture, motivational topics). I'm a passionate software developer. Software of best quality in the shortest possible time, that's my goal. Thomas has posted 6 posts at DZone. You can read more from them at their website. View Full User Profile

I don't like NullPointerExceptions

02.04.2012
| 5446 views |
  • submit to reddit
I don't like NullPointerExceptions. Actually I don't like the message of NullPointer Exceptions, i. e. "null". Such a message is usually not very helpful.

I created a few rules for myself how to avoid certain exceptions and actually define pre- and post-conditions for methods. Let me please give you a few examples.

In the beginning of a non-private method I usually check all method arguments.

public void foo( String x, int a, Person p ) {
  if ( x == null ) throw new IllegalArgumentException( "Argument 'x' must not be null." );
  if ( a < 0 ) throw new IllegalArgumentException( "Argument 'a' must not be less than 0." );
  if ( p == null ) throw new IllegalArgumentException( "Argument 'p' must not be null." );

There are of course libraries which do such checks, i. e. they can check if a String is not null and not empty, and if it contains only white spaces and so on.
I would recommend to choose one of those libraries and use them intensively.
Otherwise some checks like in the example might be helpful.

There are also libraries which offer annotations for methods or method arguments to check if the method arguments are valid. Those libraries are great in my opinion, but often companies do not allow the use of some libraries for whatever reasons.

You can create your own Exception type or use an existing one. I have seen very often that developers use IllegalArgumentException, IllegalStateException, or NullPointerException. In the later case with a good error message...

These checks should not only be done in methods, but also in constructors, and whereever it makes sense for you.

 

For private methods I use assert statements. Remember assertion checking can be enabled and disabled by the command line options -ea and -da.

private void gee( Person p ) {
assert p != null;

I usually do not add messages (too much work) to assertions. As we usually compile everything with the debug flag it is easy to find the line where the assertion error happened.

In the if statements from above or in such assert statements I always check only one thing. That means that the following code is not OK for me:

assert p != null && p.getName() != null && !list.contains( p );

I prefer:

assert p != null;
assert p.getName();
assert !list.contains( p );

Before I return a value in a method, I check the value (post-condition) like in:

public String concat( String a, String b ) {
  ...
	final String result = a + b;
	
	assert result != null;
	return result;

You can also add assert statements to various places within a method like:

public void addPerson( Person p ) {
  ...
  final int previousSize = list.size();
  list.add( p );
  assert list.size() == previousSize + 1;
  assert list.contains( p );	

Remember that this code is not included in the class files, if you use the -da option, so there is no performance problem in production software, but those assertions can be great during development to find problems.

And they are a very good way to document assumptions. That is actually the most important point for me: I like executable documentation...

 

In addition I always check a reference before I call a method of if:

y = x.getName().toUpperCase();

will become:

if ( x != null and x.getName() != null ) {
  y = ...

Or one can also use assertions here, that depends on the context.

For me it is not so important which technology is used for all those checks, but that those checks are done at all. I think it is a very important and actually very simple step toward increasing quality of software.

Published at DZone with permission of its author, Thomas Eichberger.

(Note: Opinions expressed in this article and its replies are the opinions of their respective authors and not those of DZone, Inc.)

Comments

Stig Christensen replied on Sun, 2012/02/05 - 8:32am

You code must be bloated with non business rules which makes it very hard to read. Personally I do non of these checks. Think about it, in how many places could it happen that a NULL is send as a parameter unexpected? Less code is better code.

Stig Christensen replied on Sun, 2012/02/05 - 8:41am

.. and if it happened it would be a bug anyway so it doesn't help much to get an IllegalArgumentException. Also you would catch this with automated tests.

Jean-Baptiste Nizet replied on Sun, 2012/02/05 - 12:04pm

Although i agree with defensive programming, especially in a public library, doing it everywhere looks  like bloat to me. I've never seen the assertion mechanism really used anywhere, because unit tests usually provide more value than assertions, IMHO.

But what I find wrong in your post is the last part:

 In addition I always check a reference before I call a method of if:

if ( x != null and x.getName() != null ) {

This, IMHO, is contradictory with the previous part. If x should not be null and the name should not be null, then the code should just fail fast with a NullPointerException if they are. Adding an if and returning an incorrect result is just wrong. Of course, you could have an else clause throwing an IllegalStateException, but it adds bloat over a plain NullPointerException.

Mason Mann replied on Sun, 2012/02/05 - 2:23pm

This is just idiotic. Whether you get a NullPointerException or an IllegalArgumentException doesn't matter. It's a bug, and you get the offending line in the stack trace (or debugger) either way. But in the latter case, you've littered a gazillion new lines of code all over the place. Code you have to maintain. Crazy. 

Erwin Mueller replied on Sun, 2012/02/05 - 3:09pm

I agree with pre checks, but totally disagree with post checks. Pre-checks are important, if it's part of the specification (the business domain). For example, a division function should check if the divisor is not zero. Or that the prize for an item is not negative.

But for post-checks you have your unit tests. There is no point in bloating your code with assert or checks if you already have your unit tests.That a concat function is working like expected, that should be tested in a unit test.

A unit test is automated and you can feed the concat function with multiple inputs to verify that the function is working under any input.

Nick Maiorano replied on Sun, 2012/02/05 - 11:18pm

I think there is some value in validating the input received in methods. Throwing an IllegalArgumentException with a description stating the reason is more valuable than a NPE with no explanation later in the stack trace because the method is telling you explicitly and quickly that nulls are invalid. This will speed-up debugging. However, one needs to be careful to not overdo it with checks because it can lead to code bloat and insanity. Consider the case where a public method validates the input then calls a series of private methods on itself passing along the same parameter every time. Would you really want to re-validate that parameter in every single method?

For me, the middle ground is to provide some checking at the border of a facade-like class and do the checks once. All classes behind the facade can safely assume someone else has done the validating and not repeat it.

Repetition leads to insanity.

Mason Mann replied on Mon, 2012/02/06 - 2:30am in response to: Nick Maiorano

"NPE with no explanation later in the stack trace because the method is telling you explicitly and quickly that nulls are invalid. This will speed-up debugging"

 

Actually it doesn't do anything but clutter the stack trace with bullshit. What I want to know is that there's an NPE, and the stack trace tells me where.

 

dieter von holten replied on Mon, 2012/02/06 - 3:53am

when you do null-pointer checking at method-entry, you detect wrong incoming values and throw an exception at your caller. Now he has a problem - he shouldnt have done so. YOU are clean.

when you dont do null-pointer checking like this, YOU have to deal with the null-pointer.

null-pointer checking (range-checks,...) for incoming arguments is a must for public methods.

these checks help findbugs to infer that a value is non-null further downstream - this reduces false warnings.

 

 

Nick Maiorano replied on Mon, 2012/02/06 - 9:45am in response to: Mason Mann

"What I want to know is that there's an NPE, and the stack trace tells me where."

The stack trace will tell you where but it will not tell you who is responsible. This is when the IllegalArgumentException can help. It will short-circuit the call flow and yield a shorter stack trace compared to the one you would have gotten had the method not validated input. So in fact, you will get less clutter. But as I said in my first comment, this approach is best when used sparingly and in key locations.

Mladen Girazovski replied on Tue, 2012/02/07 - 3:39am

A NPE ist exaclty as useful as an IllegalArguementException IME, execpt that i don't have to write extra code for it. NPE also tell me who is responsible: Usually whoever wrote the Code that called the method that  threw the NPE.

Using the built-in assertions is imho a bad idea, if they are disabled by default, whats the whole point ? I'd rather have unittest, they won't clutter up my production code like the built-in asserts do and have other advantages, too. You could also easily write your own assertions that will always be performed and have they would be more expressive than the built-in ones, thus reducing clutter and making your code more expressive.

The custom Assertions (ie. Assert.notNull(..), etc.) would be a way to express the prerequesites of a method, so i wouldn't mind throwing an IllegalArgumentException instead of an NPE, not because a IllegalArgumentException is somehow better than an NPE, but because the code that checks it is more expressive.

I don't see much value in checking the arguments in a private Method, just as i don't see much value in testing private methods (unless you're dealing with ugly legacy code).

Since NPEs are caused by bugs, best is to improve the quality of your code by writing tests IMHO.

 

 

 

Nick Maiorano replied on Tue, 2012/02/07 - 12:34pm in response to: Mladen Girazovski

"An NPE ist exaclty as useful as an IllegalArguementException IME, execpt that i don't have to write extra code for it. NPE also tell me who is responsible: Usually whoever wrote the Code that called the method that  threw the NPE."

What about a null value passed in a constructor parameter and saved in the object? That creates a timebomb that can blow up seconds/minutes/hours/days later with an NPE when that value is actually used. Finding the culprit in this scenario is much harder than with an explicit IllegalArgumentException - especially in asynchronous systems.

Comment viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.