Is Catching a Null Pointer Exception a Code Smell?

Question

Recently a co-worker of mine wrote in some code to catch a null pointer exception around an entire method, and return a single result. I pointed out how there could've been any number of reasons for the null pointer, so we changed it to a defensive check for the one result.

However, catching NullPointerException just seemed wrong to me. In my mind, Null pointer exceptions are the result of bad code and not to be an expected exception in the system.

Are there any cases where it makes sense to catch a null pointer exception?

Answer 1

In general, I think it is a code smell; it seems to me that defensive checks are better. I would extend that to cover most unchecked exceptions, except in event loops, etc. that want to catch all errors for reporting/logging.

The exception I can think of would be around a call to a library that can't be modified and which may generate a null pointer exception in response to some assertion failure that is difficult to proactively check.

Answer 2

Yes, catching any RuntimeException is almost always a code smell. The C2 Wiki seems to agree.

An exception would probably be some specially defensive pieces of code which run pretty much random code from other modules. Examples for such defensive structures would be the EDT, ThreadPools/Executors and plugin system.

Answer 3

It depends.

How experienced this co-worker is? Is he doing this for ignorance/laziness or is there a real good reason for that? ( like this is the main thread above everything else and should never ever die? )

90% of the times catching a runtime exception is wrong, 99% catching a NullPointerException is wrong ( if the reason is "I was getting a lot of them..." then the whole programmer is wrong and you should look take care for the rest of the code he's doing )

But under some circumstances catching a NullPointerException may be acceptable.

Answer 4

It certainly is.

Most of the time, your variables shouldn't be null to begin with. Many new languages are coming out with builtin support for non-nullable reference types -- that is, types which are guaranteed to never be null.

For the times when your incoming value is allowed to be null, you need to do a check. But exceptions are definitively a bad way to do this.

An if statement takes perhaps three instructions to perform and is a local check (meaning, you make the check in the same place as you need the guarantee).

Using an exception, on the other hand, may take many more instructions -- the system attempts to look up the method, fails, looks through the exception table for the appropriate exception handler, jumps there, executes the handler, and jumps again. Furthermore, the check is potentially non-local. If your code is something like this:

try
  return contacts.find("Mom").getEmail()
catch (NullPointerException e)
  return null

You don't know whether the NPE was thrown in 'getEmail' or in 'find'.

A technical worse solution to a very, very common pattern written in a more obfuscated way? It isn't rank, but it definitely smells bad :/

Answer 5

I try to guarantee results from my interfaces, but if some library or someones code can produce null as a result and im expecting a guarantee catching it might be viable. Ofcourse what you do once you catch it is up to you. Sometimes it just doesnt make sense to check for null, and if you catch it you have some other method of solving the problem that might not be as good but gets the job done.

What im saying is use exceptions for what you can, its a pretty good language feature.

Answer 6

I have had to catch nullpointer exception sometimes because of a bug in third part library. The library we used threw that exception, and it was nothing we could do about it.

In that case it is OK to catch it, otherwise not.

Answer 7

The only place you should catch a NullPointerException (or specifically, just any Throwable) is at some top-level or system boundary so that your program doesn't completely crash and can recover. For example, setting up an error page in your web.xml provides a catch-all so that a web application can recover from an exception and notify the user.

Answer 8

Catching a NullPointerException can be useful if your method calls an external interface (or a SOAP API) and there is a possibility that the value returned might be Null. Other than that, there is not a huge benefit to catching these exceptions.

Answer 9

It really depends on the interface definition. Unstructured NPE handling is as bad as catching Exception or Throwable.

Nulls are useful for identifying an uninitialized state rather than using an empty string or max_int or whatever. Once place where I use null regularly is in places where a callback object is not relevant.

I really like the @Nullable annotation provided by Guice.

http://code.google.com/docreader/#p=google-guice&s=google-guice&t=UseNullable

To eliminate NullPointerExceptions in your codebase, you must be disciplined about null references. We've been successful at this by following and enforcing a simple rule:

Every parameter is non-null unless explicitly specified. The Google Collections library and JSR-305 have simple APIs to get a nulls under control. Preconditions.checkNotNull can be used to fast-fail if a null reference is found, and @Nullable can be used to annotate a parameter that permits the null value.

Guice forbids null by default. It will refuse to inject null, failing with a ProvisionException instead. If null is permissible by your class, you can annotate the field or parameter with @Nullable. Guice recognizes any @Nullable annotation, like edu.umd.cs.findbugs.annotations.Nullable or javax.annotation.Nullable.

Answer 10

I can think of exactly one use for ever catching a NullPointerException:

catch (NullPointerException)
    ApplyPainfulElectricShockToProgrammer();

Answer 11

Long ago I had one use. A particularly stupid library would throw NullPointerException when asked for an object in a collection by key and the object was not found. There was no other way to look up than by key and no way to check if the object existed.

Some time later we started booted the vendor and started modifying the library. Now the library throws a better exception (my change) and has a check function (somebody else's change).

Of course I'd always end up with exactly one line inside the try block. Any more and I myself would be guilty of bad code.

Answer 12

Yes in Java there is a need to check for a NullPointerException.

Thrown when an application attempts to use null in a case where an object is required. These include:

Calling the instance method of a null object. Accessing or modifying the field of a null object. Taking the length of null as if it were an array. Accessing or modifying the slots of null as if it were an array. Throwing null as if it were a Throwable value.

Applications should throw instances of this class to indicate other illegal uses of the null object.

NullPointerException in other languages when reading text files (i.e. XML), whose records have not been validated to the correct ASCII character and record format.

Answer 13

Catching a NULL pointer exception really depends on the context ... one should strive to avoid strict absolute rules ... rules should be applied in context - want to trap this exception and put the entire software in some STABLE state - doing nothing or almost next to nothing. All such coding rules should be well understood

At this point you then look at your software AUDIT TRACE ... which you should be doing and discover the SOURCE of this exception.

The idea that a NULL Pointer Exception Shall Never Occur must be verifiable. First do a static analysis ... (which is harder if 3rd party code/components come in) and then do an exhaustive state space search using relevant tools.

x

Answer 14

And what if it was not the object that is null but the method called that throw the nullpointer exception?

Not really fan of catching only nullpointers, but i suppose lazy programmers may use it to avoid checking null objects, but it could be the same for many (all?) runtimes exception...

But i don't really understand why we shouldn't catch runtime exceptions? At some level it can be really useful to catch all runtimes and throw a checked exception no? It can be also useful to catch it, log it, and stop there if we want the app to continue working but noticing the dev that there may be a problem... It can be also usefull to catch exceptions on an interceptor on a webapp to redirect to technical error webpage... I see many cases but we generally catch as well checked and unchecked exceptions in the same time...

Don't also forget that sometimes you don't have the choice, you have to deal with your mates work, deal with a poor design, deal with a module you haven't developped, deal with time... we sometimes have to do bad code because we just can't do better at this time and we can't rewrite everything, refactor, or modify interfaces (yes, sometimes you have to throw a runtime exception)...

If you are not on a very specific case, i think you'd rather not catch a runtime exception just by lazyness of checking data...

Answer 15

Catching NPEs (any RTEs in fact) can be necessary to cleanly terminate a Swing-GUI based application.

edit : in this case, it is usually done via a UncaughtExceptionHandler though.

Answer 16

Funny

I just found something that shouldn't be done at work:

public static boolean isValidDate (final String stringDateValue) {
        String exp = "^[0-9]{2}/[0-9]{2}/[0-9]{4}$";
        boolean isValid = false;
        try {
            if (Pattern.matches(exp, stringDateValue)) {   
                String [] dateArray = stringDateValue.split("/");
                if (dateArray.length == 3) {
                    GregorianCalendar gregorianCalendar = new GregorianCalendar();    
                    int annee = new Integer(dateArray[2]).intValue();
                    int mois = new Integer(dateArray[1]).intValue();
                    int jour = new Integer(dateArray[0]).intValue();        

                    gregorianCalendar = new GregorianCalendar(annee, mois - 1, jour);
                    gregorianCalendar.setLenient(false);
                    gregorianCalendar.get(GregorianCalendar.YEAR);
                    gregorianCalendar.get(GregorianCalendar.MONTH);
                    gregorianCalendar.get(GregorianCalendar.DAY_OF_MONTH);
                    isValid = true;
                }
            } 
        } catch (Exception e) {
            isValid = false;
        }
        return isValid;

    }

baaad :)

The developper wait the calendar to raise exceptions of this kind: java.lang.IllegalArgumentException: DAY_OF_MONTH at java.util.GregorianCalendar.computeTime(GregorianCalendar.java:2316) at java.util.Calendar.updateTime(Calendar.java:2260) at java.util.Calendar.complete(Calendar.java:1305) at java.util.Calendar.get(Calendar.java:1088)

to invalidate the values...

Yes it works but it's not a really good practice...

Raising exception (particullary filling the stacktrace) cost a lot more than just checking data manually without exception...