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?

Fragile

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

Chessboard

Answer 2

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

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

Bicknell

Indeed you can, but it is optimisation favored to readibility, always a bad idea. – Fenestrated

Huh, favours optimisation? I think, on the contrary, that this is slightly more readable than option 1. – Farlee

Yes - style issues do not have definitive answers. It's what you have used to. Personally I've accustomed to this style since it also works in many other languages deriving syntax from C/C++. And if your organization/project has a code convention, follow its recommendations. – Bicknell

It's not an "optimization", nor less readable. I'd argue that having two if clauses is actually far less readable, since you end up with more nested blocks than you actually need. The double ifs also open up a window for the infamous "2ifs1else" bug: "if (a) if (b) print("a and b"); else print("not a and b"); – Izak

Answer 3

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.

Eminent

Answer 4

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.

Creak

This question is about the 'if' block so about your option 3: "You can't always rely the method will not ever return a null value – Chessboard

Of course you can ensure that it never returns null for a default value. – Creak

Option 3 would be all about moving the error handling and null checks into a method according to DRY. – Creak

I agree but what I meant is that you not always test methods you've written so you can't rely the return value won't be null – Chessboard

Answer 5

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

Whap

Answer 6

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.

Fermium

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)

Exordium

Nice! What if getAttribute() actually have side effects? Sometimes you're running it once, sometimes you're running it twice. – Supertax

I might wrote this in the "Common programming errors" question I've seen over here – Chessboard

Thanks for the comments. I knew it was no good, I am here to learn things like this one. One more question: what is the problem with this approach if getAttribute() is just a standard getter method - and so has no side-effects? – Exordium

+1 I think some people don't read the text, just the code. @question in comment: It might have a side effect at a later time. Unlikely to cause too serious damage in getters, but possible. Could result in performance loss, for example, if it returns calculated data in the next version - especially if this code is everywhere. – Soll

thanks soulmerge, I understand the problem. I think stackoverflow is great for things like this one. – Exordium

plus 1 too. Though, this is not an answer to the question. – Krucik

Answer 8

I have another answer;

List<Map<String, Object>> group = jjDatabase.separateRow(db.Select("SELECT * FROM access_user_group  WHERE user_id=1 ;"));

there is not "group_c80" as column in 'access_user_group' in my database, so in get(0).get("group_c80") null pointer exception accords. But i handled it through below code :

for (int j = 1; j < 100; j++) {
                    String rulId="0";//defult value,to privent null pointer exeption in group_c
                    try {
                        rulId = group.get(0).get("group_c" + j)).toString();
                    } catch (Exception ex) {
                        ServerLog.Print( "Handeled error in database for " + "group_c" + (j < 10 ? "0" + j : j) +"This error handeled and mot efect in program");
                        rulId = "0";
                    }}

Ooze

Answer 9

Here is my approach, needs a PropertyUtil class though, but its only written once:

/**
 * Generic method to encapsulate type casting and preventing nullPointers.
 * 
 * @param <T>          The Type expected from the result value.
 * @param o            The object to cast.
 * @param typedDefault The default value, should be of Type T.
 * 
 * @return Type casted o, of default.
 */
public static <T> T getOrDefault (Object o, T typedDefault) {
    if (null == o) {
        return typedDefault;
    }
    return (T) o;
}

Client code can do this:

PropertyUtil.getOrDefault(obj.getAttribute("someAttr"), "").equals("true");

or, for a list:

PropertyUtil.getOrDefault(
    genericObjectMap.get(MY_LIST_KEY), Collections.EMPTY_LIST
).contains(element);

Or to a consumer of List, that would reject Object:

consumeOnlyList(
    PropertyUtil.getOrDefault(
        enericObjectMap.get(MY_LIST_KEY), Collections.EMPTY_LIST
    )
)

The default might be an impl of the null object pattern https://en.wikipedia.org/wiki/Null_Object_pattern

Krucik