8

I often see java SourceCode where a null as value for a method or constructor is not allowed. A Typical implementation of this looks like

public void someMethod(Object someObject){

    if(someObject == null) throw new NullPointerException()

    someObject.aMethodCall()

}

I see no sense for myself in that at all, because if I attempt to call a Method on a nullPointer the NullPointerException is thrown anyways. I would see a sense if this method would throw an IllegalArgumentException or some other custom-made exception. Can someone clearify , why this check seems to makes sense (as I see it very often I'm assuming, that there has to be sense behind that), or why it's complete nonsense

4
  • This is useless if the correct behaviour is to null pointer (in fact it's worse since the check takes a small amount of time).
    – Jim
    Jan 26, 2012 at 13:13
  • Possible duplicate: stackoverflow.com/questions/32280/passing-null-to-a-method I prefer assertions for parameter goodness checking, just like preconditions are used in Ada.
    – vulkanino
    Jan 26, 2012 at 13:15
  • Explicit null-checks are as cheap as implicit null-checks after JIT'ing, so I wouldn't worry about performance here.
    – Chris Vest
    Jan 26, 2012 at 13:17
  • 3
    I'd argue it's the wrong exception to be thrown. If anything, you should throw an IllegalArgumentException as you are enforcing the precondition of what a method needs before anything is done. That's really what you want to do. You are right, this exception is the wrong exception, but it doesn't mean you shouldn't perform the check or throw the proper exception.
    – casperOne
    Jan 26, 2012 at 13:34

9 Answers 9

9

The code you posted makes absolutely no sense at all. It looks like a strong case of cargo cult programming. Most likely, somebody implemented a useful test to check for pre-conditions once and somebody else adapted the test to look like this.

6

There is a corner case where something like that would make sense. If you don't use someObject immediately short-circuiting the error case to the start of the function can be useful.

public void someMethod(Object someObject){

    if(someObject == null) throw new NullPointerException();

    expensiveOperationNotUsingSomeObject();

    someObject.aMethodCall();
}
1
  • 1
    +1 a check and explicit throw can be useful. Think fail fast, fail early.
    – Qwerky
    Jan 26, 2012 at 13:38
5

No, that doesn't make much sense as it stands, but only because throwing an NPE doesn't add any useful info.

You could make the error nicer by throwing (for example) an IllegalStateException:

if (someObject == null) throw new IllegalStateException("someObject was null");

But that doesn't add a great deal of value either - other than being specific about what is null (which can be useful in more complex methods)

1
  • And the possible exception should be declared in the throws clause and documented. Jan 26, 2012 at 13:29
5

Of course you want to check for null. This is a pre-condition for your method, part of its contract with clients. If your method cannot accept null input, you need to enforce that.

The choice of exception could be better. I usually go with an IllegalArgumentException.

If it's the brevity of the method that bothers you, I have to say I agree. No new info there.

2
  • I don't believe IllegalArgumentException is better choice. NPE is more specific. Also you can consult item 60 of Effective Java about it. Jan 26, 2012 at 13:30
  • I'll give it a look, but I don't know if I agree. The message in IAE can make it clear enough. It can also flag other conditions (e.g. String that can be neither blank nor null). Your interpretation is very narrow, but correct for this problem.
    – duffymo
    Jan 26, 2012 at 13:38
4

This depends on who is responsible to recover from this situation. If it's the caller, throw a NPE. If it's the called method, test for null and do anything that is necessary to resolve the situation.

Edit: Don't throw a NPE explicitly but just let it bubble up. Sorry for the vague wording.

11
  • But in the case the example the OP gave, it is clear that the method itself isn't recovering from the exception...
    – cdeszaq
    Jan 26, 2012 at 13:14
  • True, but I took this only as a MWE, not as real code.
    – user647772
    Jan 26, 2012 at 13:15
  • Throwing unchecked exception is bad programming practice... Jan 26, 2012 at 13:24
  • 1
    @Alex And giving an illegal parameter to an exception is not a programming error?
    – Voo
    Jan 26, 2012 at 14:40
  • 1
    @Voo Yes it is! But best practice and common sense tells that methods should throw Cheked exceptions (define custom exception that encapsulate your program bad use-cases) and these exception should be handled by the caller. Unchecked exception will only be visible at runtime and they can cause a lot of pain after deployments. This is not the case with test cases of course... Jan 26, 2012 at 16:05
2

The situation you describe may be useful under one of the following assumptions:

1.) If you have to perform some operations which are either costly or do change some global state prior to the execution someObject.aMethodCall() you can prevent rollback code and waste of execution cycles.

2.) If someObject is stored within a data structure, you may create a possible pitfall if at some later point the data is retrieved from the data structure. I think, I remember some classes in the java collection framework, which throw rather a NPE than allowing a null into the storage structure.

1
  • An example from the Java collections: Older versions of an empty TreeSet allowed null to be added, only to throw a NPE when any second element was to be added. The current version of TreeSet.add explicitly checks for null. Jul 11, 2012 at 20:00
2

Personally the reason why I'd never throw a NullPointerException as part of a public API method is because it makes it much harder for the programmer to distinguish between a programming error on their side and a bug in my code (ie did I deliberately allow the NPE to be thrown or not?).

Throwing a different exception makes the intention much clearer and also helps the programmer to find out what's wrong easier. So I'd go with IllegalArgumentException there.

If it's some internal method? I'd use assertions or just let it crash. No sense in explicitly throwing a NPE imo.

0

I think doing this type of check makes sense if a method does processing which takes a lot of time or is hard to undo before it actually triggers the NPE. In the example given I don't think it makes actual sense.

On the other hand: using assert or throwing a more specific exception like IllegalArgumentException might make more sense.

0

In C# exists an ArgumentNullException. This exception takes the name of the argument in the constructor. So the caller could see, which argument maybe is illicity NULL. The IllegalArgumentException is almost the Java-equivalent to that.

Not the answer you're looking for? Browse other questions tagged or ask your own question.