Gracefully avoiding NullPointerException in Java

Question

Consider this line:

if (object.getAttribute("someAttr").equals("true")) { // ....

Obviously this line is a potential bug, the attribute might be null and we will get a NullPointerException. So we need to refactor it to one of two choices:

First option:

if ("true".equals(object.getAttribute("someAttr"))) { // ....

Second option:

String attr = object.getAttribute("someAttr");
if (attr != null) {
    if (attr.equals("true")) { // ....

The first option is awkward to read but more concise, while the second one is clear in intent, but verbose.

Which option do you prefer in terms of readability?

Answer 1

I've always used

if ("true".equals(object.getAttribute("someAttr"))) { // ....

because although it is a little more difficult to read it's much less verbose and I think it's readable enough so you get used to it very easily

Answer 2

I like option 1 and I would argue that it is readable enough.

Option 3 btw would be to introduce a getAttribute method that takes a default value as a parameter.

Answer 3

Util.isEmpty(string) - returns string == null || string.trim().isEmpty() Util.notNull(string) returns "" if string == null, string otherwise. Util.isNotEmpty(string) returns ! Util.isEmpty(string)

And we have a convention that for strings, Util.isEmpty(string) semantically means true and Util.isNotEmpty(string) semantically means false.

Answer 4

In the second option, you can take advantage of short-circuiting &&:

String attr = object.getAttribute("someAttr");
if (attr != null && attr.equals("true")) { // ....

Answer 5

Always aspire for shorter code, given that both are functionaly equivalent. Especially in case like this where readability is not sacrificed.

Answer 6

There are certain situations where the concise approach feels wrong to start with but effectively becomes idiomatic. This is one of them; the other is something like:

String line;
while ((line = bufferedReader.readLine()) != null) {
  // Use line
}

Side effects in a condition? Unthinkable! Except it's basically nicer than the alternatives, when you recognise the particular pattern.

This pattern is similar - it's so common in Java that I'd expect any reasonably experienced developer to recognise it. The result is pleasantly concise. (Amusingly, I sometimes see C# code which uses the same idiom, unnecessarily - the equality operator works fine with strings in C#.)

Bottom line: use the first version, and become familiar with it.

Answer 7

It's a very good question. I usually use the not graceful:

if (object.getAttribute("someAttr") != null && object.getAttribute("someAttr").equals("true")) { // ....

(and I will not use it anymore)