27

There are some patterns for checking whether a parameter to a method has been given a null value.

First, the classic one. It is common in self-made code and obvious to understand.

public void method1(String arg) {
  if (arg == null) {
    throw new NullPointerException("arg");
  }
}

Second, you can use an existing framework. That code looks a little nicer because it only occupies a single line. The downside is that it potentially calls another method, which might make the code run a little slower, depending on the compiler.

public void method2(String arg) {
  Assert.notNull(arg, "arg");
}

Third, you can try to call a method without side effects on the object. This may look odd at first, but it has fewer tokens than the above versions.

public void method3(String arg) {
  arg.getClass();
}

I haven't seen the third pattern in wide use, and it feels almost as if I had invented it myself. I like it for its shortness, and because the compiler has a good chance of optimizing it away completely or converting it into a single machine instruction. I also compile my code with line number information, so if a NullPointerException is thrown, I can trace it back to the exact variable, since I have only one such check per line.

Which check do you prefer, and why?

Roland Illig
  • 40,703
  • 10
  • 88
  • 121

14 Answers14

23

Approach #3: arg.getClass(); is clever, but unless this idiom see widespread adoption, I'd prefer the clearer, more verbose methods as opposed to saving a few characters. I'm a "write once, read many" kind of programmer.

The other approaches are self-documenting: there's a log message you can use to clarify what happened - this log message is use when reading the code and also at run-time. arg.getClass(), as it stands, is not self-documenting. You could use a comment at least o clarify to reviewers of the code:

arg.getClass(); // null check

But you still don't get a chance to put a specific message in the runtime like you can with the other methods.


Approach #1 vs #2 (null-check+NPE/IAE vs assert): I try to follow guidelines like this:

http://data.opengeo.org/GEOT-290810-1755-708.pdf

  • Use assert to check parameters on private methods
    assert param > 0;

  • Use null check + IllegalArgumentException to check parameters on public methods
    if (param == null) throw new IllegalArgumentException("param cannot be null");

  • Use null check + NullPointerException where needed
    if (getChild() == null) throw new NullPointerException("node must have children");


HOWEVER, since this is question may be about catching potential null issues most efficiently, then I have to mention my preferred method for dealing with null is using static analysis, e.g. type annotations (e.g. @NonNull) a la JSR-305. My favorite tool for checking them is:

The Checker Framework:
Custom pluggable types for Java
https://checkerframework.org/manual/#checker-guarantees

If its my project (e.g. not a library with a public API) and if I can use the Checker Framework throughout:

  • I can document my intention more clearly in the API (e.g. this parameter may not be null (the default), but this one may be null (@Nullable; the method may return null; etc). This annotation is right at the declaration, rather than further away in the Javadoc, so is much more likely to be maintained.

  • static analysis is more efficient than any runtime check

  • static analysis will flag potential logic flaws in advance (e.g. that I tried to pass a variable that may be null to a method that only accepts a non-null parameter) rather than depending on the issue occurring at runtime.

One other bonus is that the tool lets me put the annotations in a comment (e.g. `/@Nullable/), so my library code can compatible with type-annotated projects and non-type-annotated projects (not that I have any of these).


In case the link goes dead again, here's the section from GeoTools Developer Guide:

http://data.opengeo.org/GEOT-290810-1755-708.pdf

5.1.7 Use of Assertions, IllegalArgumentException and NPE

The Java language has for a couple of years now made an assert keyword available; this keyword can be used to perform debug only checks. While there are several uses of this facility, a common one is to check method parameters on private (not public) methods. Other uses are post-conditions and invariants.

Reference: Programming With Assertions

Pre-conditions (like argument checks in private methods) are typically easy targets for assertions. Post-conditions and invariants are sometime less straighforward but more valuable, since non-trivial conditions have more risks to be broken.

  • Example 1: After a map projection in the referencing module, an assertion performs the inverse map projection and checks the result with the original point (post-condition).
  • Example 2: In DirectPosition.equals(Object) implementations, if the result is true, then the assertion ensures that hashCode() are identical as required by the Object contract.

Use Assert to check Parameters on Private methods

private double scale( int scaleDenominator ){
 assert scaleDenominator > 0;
 return 1 / (double) scaleDenominator;
}

You can enable assertions with the following command line parameter:

java -ea MyApp

You can turn only GeoTools assertions with the following command line parameter:

java -ea:org.geotools MyApp

You can disable assertions for a specific package as shown here:

java -ea:org.geotools -da:org.geotools.referencing MyApp

Use IllegalArgumentExceptions to check Parameters on Public Methods

The use of asserts on public methods is strictly discouraged; because the mistake being reported has been made in client code - be honest and tell them up front with an IllegalArgumentException when they have screwed up.

public double toScale( int scaleDenominator ){
 if( scaleDenominator > 0 ){
 throw new IllegalArgumentException( "scaleDenominator must be greater than 0");
 }
 return 1 / (double) scaleDenominator;
}

Use NullPointerException where needed

If possible perform your own null checks; throwing a IllegalArgumentException or NullPointerException with detailed information about what has gone wrong.

public double toScale( Integer scaleDenominator ){
 if( scaleDenominator == null ){
 throw new NullPointerException( "scaleDenominator must be provided");
 }
 if( scaleDenominator > 0 ){
 throw new IllegalArgumentException( "scaleDenominator must be greater than 0");
 }
 return 1 / (double) scaleDenominator;
}
mernst
  • 7,437
  • 30
  • 45
Bert F
  • 85,407
  • 12
  • 106
  • 123
  • 1
    I'm a fan of the static verification approach. How do you deal with the problem of third-party libraries that provide no such annotations? Everything nullable by default? Everything not-null by default? Can you make specific overrides of this default? – R. Martinho Fernandes Jan 25 '11 at 16:19
  • 1
    @Martinho - Two approaches: (1) ignore it because the default works for most parameters/methods, and use can use `SuppressWarnings` to handle the rest, and (2) the tool provides a way to annotate a 3rd party library separately. – Bert F Jan 25 '11 at 16:23
12

Aren't you optimizing a biiiiiiiiiiiiiiit too prematurely!?

I would just use the first. It's clear and concise.

I rarely work with Java, but I assume there's a way to have Assert only operate on debug builds, so that would be a no-no.

The third gives me the creeps, and I think I would immediately resort to violence if I ever saw it in code. It's completely unclear what it's doing.

Community
  • 1
  • 1
Willem van Rumpt
  • 6,490
  • 2
  • 32
  • 44
  • 5
    EXACTLY this. There is no way in the world you'll ever find a performance problem with null checking. Even if the JIT compiler doesn't optimize this, and it probably will, the time is negligible. Compare the micro or nano seconds this takes with the milliseconds of network or disk access. There are bigger fish to fry. – rfeak Jan 25 '11 at 16:22
  • When you don't have network or disk access (which I don't), and when your code gets called billionstrillions of times, even a single machine instruction matters. That's why I'm asking. – Roland Illig Mar 08 '11 at 22:09
  • When you saw a `for` loop for the first time ever, did you resort to violence, too? Still, using this weird syntax has become common knowledge, and many people don't have to think about it when they see it now. Personally, I got used to the third variant, and I don't have to think about it anymore. It's like seeing `(void) argv` in C code, which at first looks like complete nonsense. – Roland Illig Mar 08 '11 at 22:13
  • 4
    @Roland Illig: As I said, I work rarely with Java. Apparently lots of people use it (although this apparently only became clear quite recently, since you yourself said "I haven't seen it in wide use"). If a single machine instruction matters, don't use Java, and I'm not some "native language" disciple, far from it. For regular Java code, I still perceive this as an awkward (apparently generally accepted) hack. If a null check is the performance bottleneck in your Java source, chances are you should evaluate switching to a compiled language (if only for that particular part). – Willem van Rumpt Mar 09 '11 at 16:51
  • That view on Java remembers me of the old times, when Java was an interpreted language. Nowadays the HotSpot compiler is really great at producing efficient code, especially for long-running applications. To me, one of the beauties of Java is that I can translate the source code into equivalent machine instructions easily, so I know what performance to expect from the code. And yes, I already looked at the generated machine code, and it was what I expected, or often even better than that. So, for performance, Java is like C or C++, except that you don't run into undefined behavior so often. – Roland Illig Mar 09 '11 at 23:25
9

You can use the Objects Utility Class.

public void method1(String arg) {
  Objects.requireNonNull(arg);
}

see http://docs.oracle.com/javase/7/docs/api/java/util/Objects.html#requireNonNull%28T%29

räph
  • 3,634
  • 9
  • 34
  • 41
6

You should not be throwing NullPointerException. If you want a NullPointerException, just dont check the value and it will be thrown automatically when the parameter is null and you attempt to dereference it.

Check out the apache commons lang Validate and StringUtils classes.
Validate.notNull(variable) it will throw an IllegalArgumentException if "variable" is null.
Validate.notEmpty(variable) will throw an IllegalArgumentException if "variable" is empty (null or zero length".
Perhaps even better:
String trimmedValue = StringUtils.trimToEmpty(variable) will guarantee that "trimmedValue" is never null. If "variable" is null, "trimmedValue" will be the empty string ("").

DwB
  • 37,124
  • 11
  • 56
  • 82
  • 5
    Nitpick: A NullPointerException is not automatically thrown when a parameter is null. It is only thrown when you attempt to dereference a null reference (i.e. call something on it). – R. Martinho Fernandes Jan 25 '11 at 16:21
  • What Martinho said. Having said that, I like that you mentioned the Apache lang package. I myself prefer to use their custom illegal and null argument exceptions as opposed to directly launching a standard NPE. – luis.espinal Mar 11 '11 at 11:19
4

In my opinion, there are three issues with the third method:

  1. The intent is unclear to the casual reader.
  2. Even though you have line number information, line numbers change. In a real production system, knowing that there was a problem in SomeClass at line 100 doesn't give you all the info you need. You also need to know the revision of the file in question and be able to get to that revision. All in all, a lot of hassle for what appears to be very little benefit.
  3. It is not at all clear why you think the call to arg.getClass can be optimized away. It is a native method. Unless HotSpot is coded to have specific knowledge of the method for this exact eventuality, it'll probably leave the call alone since it can't know about any potential side-effects of the C code that gets called.

My preference is to use #1 whenever I feel there's a need for a null check. Having the variable name in the error message is great for quickly figuring out what exactly has gone wrong.

P.S. I don't think that optimizing the number of tokens in the source file is a very useful criterion.

NPE
  • 486,780
  • 108
  • 951
  • 1,012
3

The first method is my preference because it conveys the most intent. There are often shortcuts that can be taken in programming but my view is that shorter code is not always better code.

Bnjmn
  • 1,973
  • 1
  • 20
  • 34
  • 2
    I don't do much Java, so I'll have to ask: should you be throwing around NullPointerExceptions? Isn't that sort of "reserved" for the runtime? Shouldn't it be something more along the lines of IllegalArgumentException? – R. Martinho Fernandes Jan 25 '11 at 15:46
  • @Martinho Fernandes It's okay to throw an `NPE`, if it matches the error condition, you should almost never catch an `NPE` though. – biziclop Jan 25 '11 at 15:47
  • It is perfectly proper to throw a NPE in the case of a null you weren't expecting. Things like OOM should be reserved for the runtime. – gub Jan 25 '11 at 15:49
  • NPE vs IAE is oft debated with no winner (AFAIK) (http://stackoverflow.com/questions/3881/illegalargumentexception-or-nullpointerexception-for-a-null-parameter). Side IAE argues its more consistent with other parameter checks, while side NPE argues that NPE is more consistent with what the the core API does. – Bert F Jan 25 '11 at 16:20
2

x==null is super fast, and it can be a couple of CPU clocks (incl. the branch prediction which is going to succeed). AssertNotNull will be inlined, so no difference there.

x.getClass() should not be faster than x==null even if it uses trap. (reason: the x will be in some register and checking a register vs an immediate value is fast, the branch is going to be predicted properly as well)

Bottom line: unless you do something truly weird, it'd be optimized by the JVM.

bestsss
  • 11,796
  • 3
  • 53
  • 63
1

While I agree with the general consensus of preferring to avoid the getClass() hack, it is worth noting that, as of OpenJDK version 1.8.0_121, javac will use the getClass() hack to insert null checks prior to creating lambda expressions. For example, consider:

public class NullCheck {
  public static void main(String[] args) {
    Object o = null;
    Runnable r = o::hashCode;
  }
}

After compiling this with javac, you can use javap to see the bytecode by running javap -c NullCheck. The output is (in part):

Compiled from "NullCheck.java"
public class NullCheck {
  public NullCheck();
    Code:
       0: aload_0
       1: invokespecial #1    // Method java/lang/Object."<init>":()V
       4: return

  public static void main(java.lang.String[]);
    Code:
       0: aconst_null
       1: astore_1
       2: aload_1
       3: dup
       4: invokevirtual #2    // Method java/lang/Object.getClass:()Ljava/lang/Class;
       7: pop
       8: invokedynamic #3, 0 // InvokeDynamic #0:run:(Ljava/lang/Object;)Ljava/lang/Runnable;
      13: astore_2
      14: return
}

The instruction set at "lines" 3, 4 and 7 are basically invoking o.getClass(), and discarding the result. If you run NullCheck, you'll get a NullPointerException thrown from line 4.

Whether this is something that the Java folks concluded was a necessary optimization, or it is just a cheap hack, I don't know. However, based on John Rose's comment at https://bugs.openjdk.java.net/browse/JDK-8042127?focusedCommentId=13612451&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13612451, I suspect that it may indeed be the case that the getClass() hack, which produces an implicit null check, may be ever so slightly more performant than its explicit counterpart. That said, I would avoid using it unless careful benchmarking showed that it made any appreciable difference.

(Interestingly, the Eclipse Compiler For Java (ECJ) does not include this null check, and running NullCheck as compiled by ECJ will not throw a n NPE.)

Ian Robertson
  • 1,312
  • 1
  • 13
  • 14
1

The first option is the easiest one and also is the most clear.

It's not common in Java, but in C and C++ where the = operator can be included in a expression in the if statement and therefore lead to errors, it's often recommended to switch places between the variable and the constant like this:

if (NULL == variable) {
   ...
}

instead of:

if (variable == NULL) {
   ...
}

preventing errors of the type:

if (variable = NULL) { // Assignment!
   ...
}

If you make the change, the compiler will find that kind of errors for you.

Marcelo
  • 2,232
  • 3
  • 22
  • 31
  • 1
    Are you recommending Yoda conditionals in Java? If not, why are you rambling about C and C++ in a Java question? – R. Martinho Fernandes Jan 25 '11 at 16:01
  • 1
    I don't consider it to be off topic. I answered the question and then I tried to contribute with a little suggestion that it's not that far from Java after all. Sorry if I violated a rule (although I don't think I did.) – Marcelo Jan 25 '11 at 16:41
  • I give a +vote to this, as I'm one of the Java developers who write (null == x) and I can see the reason why Marcelo brought this up. – yclian Jan 27 '12 at 10:03
0

I'd use the built-in Java assert mechanism.

assert arg != null;

The advantage of this over all the other methods is that it can be switched off.

biziclop
  • 48,926
  • 12
  • 77
  • 104
  • I included this in my own answer. Elaboration: asserts prevent other developers from passing nulls, so anyone faultily passing nulls to a method that does not accept nulls will get slapped on the wrist. Always remember though that the assert keyword is and should be used as a developer-only tool - production machines will /not/ have asserts enabled. – cthulhu Jan 25 '11 at 15:54
  • @Cthulhu And how is that different from what I wrote? Unlike all the other solutions, `assert` doesn't slap on your wrist, as it can be switched off. Even better, in can be switched off and on selectively without recompiling your code. – biziclop Jan 25 '11 at 15:57
  • The only reason I can think to switch off these checks is performance. I can't fathom a scenario where your bottleneck becomes the non-null assertions everywhere, though. – R. Martinho Fernandes Jan 25 '11 at 16:00
  • @Martinho Fernandes As far as I can gather, the OP was worrying about performance. Which, to be fair, I don't agree with, but that was the question. – biziclop Jan 25 '11 at 16:07
0

I prefer method 4, 5 or 6, with #4 being applied to public API methods and 5 / 6 for internal methods, although #6 would be more frequently applied to public methods.

/**
 * Method 4.
 * @param arg A String that should have some method called upon it. Will be ignored if
 * null, empty or whitespace only.
 */
public void method4(String arg) {
   // commons stringutils
   if (StringUtils.isNotBlank(arg) {
       arg.trim();
   }
}

/**
 * Method 5.
 * @param arg A String that should have some method called upon it. Shouldn't be null.
 */
public void method5(String arg) {
   // Let NPE sort 'em out.
   arg.trim();
}

/**
 * Method 6.
 * @param arg A String that should have some method called upon it. Shouldn't be null.
 */
public void method5(String arg) {
   // use asserts, expect asserts to be enabled during dev-time, so that developers
   // that refuse to read the documentations get slapped on the wrist for still passing
   // null. Assert is a no-op if the -ae param is not passed to the jvm, so 0 overhead.

   assert arg != null : "Arg cannot be null"; // insert insult here.
   arg.trim();
}

The best solution to handle nulls is to not use nulls. Wrap third-party or library methods that may return nulls with null guards, replacing the value with something that makes sense (such as an empty string) but does nothing when used. Throw NPE's if a null really shouldn't be passed, especially in setter methods where the passed object doesn't get called right away.

cthulhu
  • 3,142
  • 2
  • 23
  • 32
0

There is no vote for this one, but I use a slight variation of #2, like

erStr += nullCheck (varName, String errMsg); // returns formatted error message

Rationale: (1) I can loop over a bunch of arguments, (2) The nullCheck method is tucked away in a superclass and (3) at the end of the loop,

if (erStr.length() > 0)
    // Send out complete error message to client
else
    // do stuff with variables

In the superclass method, your #3 looks nice, but I wouldn't throw an exception (what is the point, somebody has to handle it, and as a servlet container, tomcat will ignore it, so it might as well be this()) Regards, - M.S.

Manidip Sengupta
  • 3,573
  • 5
  • 25
  • 27
  • 1
    erStr + =... adds tons of overhead – bestsss Jan 25 '11 at 17:28
  • I know, I know, should have used a StringBuffer - will fix it some day! – Manidip Sengupta Jan 25 '11 at 17:49
  • 1
    no, no. There won't be any huge gains w/ StringBuffer/Builder. My point is that: if you have to do client validation, the code is ok. But if you do a normal null check, the code adds really a lot of overhead. – bestsss Jan 25 '11 at 18:37
  • You may be right, sometimes we felt it was redundant, because the calls come from a client (our code) and in the client it is checked too. But then we keep it as a safety measure - in case some developer forgets to do the necessary check at the client end, etc. Defensive style, if you will. Now, why would you say its a lot of overhead? Would you sugest an alternative? – Manidip Sengupta Jan 25 '11 at 19:56
  • 2
    it's overhead b/c the erStr+=... is executed always, creates new String(s), even if empty (or null), it's still far from no-op. If the code is an entry point, you can live w/ that and no need to think of performance, if you perform it somewhere deep-inside (and often called) you have to move the check somewhere at the upper level(s) and if you wish to be paranoid use 'assert' deep inside. – bestsss Jan 25 '11 at 21:53
0

First method. I would never do the second or the third method, not unless they are implemented efficiently by the underlying JVM. Otherwise, those two are just prime examples of premature optimization (with the third having a possible performance penalty - you don't want to be dealing and accessing class meta-data in general access points.)

The problem with NPEs is that they are things that cross-cut many aspects of programming (and my aspects, I mean something deeper and more profound that AOP). It is a language design problem (not saying that the language is bad, but that it is one fundamental short-coming... of any language that allows null pointers or references.)

As such, it is best to simply deal with it explicitly as in the first method. All other methods are (failed) attempts to simplify a model of operations, an unavoidable complexity that exists on the underlying programming model.

It is a bullet that we cannot avoid to bite. Deal with it explicitly as it is - in the general case that is - the less painful down the road.

luis.espinal
  • 10,331
  • 6
  • 39
  • 55
-1

I believe that the fourth and the most useful pattern is to do nothing. Your code will throw NullPointerException or other exception a couple of lines later (if null is illegal value) and will work fine if null is OK in this context.

I believe that you should perform null check only if you have something to do with it. Checking to throw exception is irrelevant in most cases. Just do not forget to mention in javadoc whether the parameter can be null.

AlexR
  • 114,158
  • 16
  • 130
  • 208
  • 3
    I disagree. It could be "a couple of lines later" or arbitrarily far away, since you're letting the nulls pass unscathed through every single method. If you pre-emptively refuse null arguments, the exception is thrown as close to the culprit as possible. – R. Martinho Fernandes Jan 25 '11 at 15:49
  • @Martinho Fernandes - +1 for fail fast: http://stackoverflow.com/questions/2807241/what-does-the-expression-fail-early-mean-and-when-would-you-want-to-do-so/2807375#2807375 – Bert F Jan 25 '11 at 16:17