Is Catching a Null Pointer Exception a Code Smell?

Recently a co-worker of mine wrot开发者_JAVA技巧e 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?

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.

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

catch (NullPointerException) {
    ApplyPainfulElectricShockToProgrammer();
}

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.

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.

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.

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 wanted 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 (particularly filling the stack trace) cost a lot more than just checking data manually without an exception...

It is bad, but it could produce optimized bytecode.

If Integer i is not null most of the time, then check decreases overall performance. The check itself costs 3 instructions (0-4). The whole case then takes 7 instructions (0-14).

public class IfNotNull {

    public Integer i;

    public String getIAsString() {
        if (i != null) {
            return i.toString();
        } else {
            return "";
        }
    }
}

  public java.lang.String getIAsString();
    Code:
       0: aload_0       
       1: getfield      #2                  // Field i:Ljava/lang/Integer;
       4: ifnull        15
       7: aload_0       
       8: getfield      #2                  // Field i:Ljava/lang/Integer;
      11: invokevirtual #3                  // Method java/lang/Integer.toString:()Ljava/lang/String;
      14: areturn       // <- here we go
      15: ldc           #4                  // String 
      17: areturn 

Following EAFP approach which is common in Python world. The null case will be costly, but we need only 4 instructions (0-7) for not null case.

public class TryCatch {

    public Integer i;

    public String getIAsString() {
        try {
            return i.toString();
        } catch (NullPointerException npe) {
            return "";
        }
    }
}

  public java.lang.String getIAsString();
    Code:
       0: aload_0       
       1: getfield      #2                  // Field i:Ljava/lang/Integer;
       4: invokevirtual #3                  // Method java/lang/Integer.toString:()Ljava/lang/String;
       7: areturn       // <- here we go
       8: astore_1      
       9: ldc           #5                  // String a
      11: areturn       
    Exception table:
       from    to  target type
           0     7     8   Class java/lang/NullPointerException

Who knows, if JIT compiler can optimize this?

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 :/

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.

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

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.

What about this:

try
{
    foo.x = bar;
}
catch (NullPointerException e)
{
    // do whatever needs to be done
}

as a micro-optimization when foo may be null, but almost never is?

The idea is this:

• An explicit NULL check takes a single machine instruction

• On the other hand, the NULL check in the second version may be done by letting the NULL access occur, catching the SIGSEGV, and throwing a NullPointerException. This is free if the object is not NULL.

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 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.

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.

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.

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.

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.

If the programmer is beginner he may have a habit to catch every exception which stops him to get the final output. This should not be entertained by code reviewers.

Catching any RuntimeException is bad. But if it is really necessary then a comment in code will be really helpful for future programmers who will work on that code. If you cannot write a reasonable comment for catching them then you must avoid them. period.