Null check chain vs catching NullPointerException

Question

A web service returns a huge XML and I need to access deeply nested fields of it. For example:

return wsObject.getFoo().getBar().getBaz().getInt()

The problem is that getFoo(), getBar(), getBaz() may all return null.

However, if I check for null in all cases, the code becomes very verbose and hard to read. Moreover, I may miss the checks for some of the fields.

if (wsObject.getFoo() == null) return -1;
if (wsObject.getFoo().getBar() == null) return -1;
// maybe also do something with wsObject.getFoo().getBar()
if (wsObject.getFoo().getBar().getBaz() == null) return -1;
return wsObject.getFoo().getBar().getBaz().getInt();

Is it acceptable to write

try {
    return wsObject.getFoo().getBar().getBaz().getInt();
} catch (NullPointerException ignored) {
    return -1;
}

or would that be considered an antipattern?

Solution 1

Catching NullPointerException is a really problematic thing to do since they can happen almost anywhere. It's very easy to get one from a bug, catch it by accident and continue as if everything is normal, thus hiding a real problem. It's so tricky to deal with so it's best to avoid altogether. (For example, think about auto-unboxing of a null Integer.)

I suggest that you use the Optional class instead. This is often the best approach when you want to work with values that are either present or absent.

Using that you could write your code like this:

public Optional<Integer> m(Ws wsObject) {
    return Optional.ofNullable(wsObject.getFoo()) // Here you get Optional.empty() if the Foo is null
        .map(f -> f.getBar()) // Here you transform the optional or get empty if the Bar is null
        .map(b -> b.getBaz())
        .map(b -> b.getInt());
        // Add this if you want to return null instead of an empty optional if any is null
        // .orElse(null);
        // Or this if you want to throw an exception instead
        // .orElseThrow(SomeApplicationException::new);
}

---

Why optional?

Using Optionals instead of null for values that might be absent makes that fact very visible and clear to readers, and the type system will make sure you don't accidentally forget about it.

You also get access to methods for working with such values more conveniently, like map and orElse.

---

Is absence valid or error?

But also think about if it is a valid result for the intermediate methods to return null or if that is a sign of an error. If it is always an error then it's probably better throw an exception than to return a special value, or for the intermediate methods themselves to throw an exception.

---

Maybe more optionals?

If on the other hand absent values from the intermediate methods are valid, maybe you can switch to Optionals for them also?

Then you could use them like this:

public Optional<Integer> mo(Ws wsObject) {
    return wsObject.getFoo()
        .flatMap(f -> f.getBar())
        .flatMap(b -> b.getBaz())
        .flatMap(b -> b.getInt());        
}

---

Why not optional?

The only reason I can think of for not using Optional is if this is in a really performance critical part of the code, and if garbage collection overhead turns out to be a problem. This is because a few Optional objects are allocated each time the code is executed, and the VM might not be able to optimize those away. In that case your original if-tests might be better.

Solution 2

I suggest considering Objects.requireNonNull(T obj, String message). You might build chains with a detailed message for each exception, like

requireNonNull(requireNonNull(requireNonNull(
    wsObject, "wsObject is null")
        .getFoo(), "getFoo() is null")
            .getBar(), "getBar() is null");

I would suggest you not to use special return-values, like -1. That's not a Java style. Java has designed the mechanism of exceptions to avoid this old-fashioned way which came from the C language.

Throwing NullPointerException is not the best option too. You could provide your own exception (making it checked to guarantee that it will be handled by a user or unchecked to process it in an easier way) or use a specific exception from XML parser you are using.

Solution 3

Assuming the class structure is indeed out of our control, as seems to be the case, I think catching the NPE as suggested in the question is indeed a reasonable solution, unless performance is a major concern. One small improvement might be to wrap the throw/catch logic to avoid clutter:

static <T> T get(Supplier<T> supplier, T defaultValue) {
    try {
        return supplier.get();
    } catch (NullPointerException e) {
        return defaultValue;
    }
}

Now you can simply do:

return get(() -> wsObject.getFoo().getBar().getBaz().getInt(), -1);

Solution 4

As already pointed out by Tom in the comment,

Following statement disobeys the Law of Demeter,

wsObject.getFoo().getBar().getBaz().getInt()

What you want is int and you can get it from Foo. Law of Demeter says, never talk to the strangers. For your case you can hide the actual implementation under the hood of Foo and Bar.

Now, you can create method in Foo to fetch int from Baz. Ultimately, Foo will have Bar and in Bar we can access Int without exposing Baz directly to Foo. So, null checks are probably divided to different classes and only required attributes will be shared among the classes.

Solution 5

You say that some methods "may return null" but do not say in what circumstances they return null. You say you catch the NullPointerException but you do not say why you catch it. This lack of information suggests you do not have a clear understanding of what exceptions are for and why they are superior to the alternative.

Consider a class method that is meant to perform an action, but the method can not guarantee it will perform the action, because of circumstances beyond its control (which is in fact the case for all methods in Java). We call that method and it returns. The code that calls that method needs to know whether it was successful. How can it know? How can it be structured to cope with the two possibilities, of success or failure?

Using exceptions, we can write methods that have success as a post condition. If the method returns, it was successful. If it throws an exception, it had failed. This is a big win for clarity. We can write code that clearly processes the normal, success case, and move all the error handling code into catch clauses. It often transpires that the details of how or why a method was unsuccessful are not important to the caller, so the same catch clause can be used for handling several types of failure. And it often happens that a method does not need to catch exceptions at all, but can just allow them to propagate to its caller. Exceptions due to program bugs are in that latter class; few methods can react appropriately when there is a bug.

So, those methods that return null.

• Does a null value indicate a bug in your code? If it does, you should not be catching the exception at all. And your code should not be trying to second guess itself. Just write what is clear and concise on the assumption that it will work. Is a chain of method calls clear and concise? Then just use them.
• Does a null value indicate invalid input to your program? If it does, a NullPointerException is not an appropriate exception to throw, because conventionally it is reserved for indicating bugs. You probably want to throw a custom exception derived from IllegalArgumentException (if you want an unchecked exception) or IOException (if you want a checked exception). Is your program required to provide detailed syntax error messages when there is invalid input? If so, checking each method for a null return value then throwing an appropriate diagnostic exception is the only thing you can do. If your program need not provide detailed diagnostics, chaining the method calls together, catching any NullPointerException and then throwing your custom exception is clearest and most concise.

---

One of the answers claims that the chained method calls violate the Law of Demeter and thus are bad. That claim is mistaken.

• When it comes to program design, there are not really any absolute rules about what is good and what is bad. There are only heuristics: rules that are right much (even almost all) of the time. Part of the skill of programming is knowing when it is OK to break those kinds of rules. So a terse assertion that "this is against rule X" is not really an answer at all. Is this one of the situations where the rule should be broken?
• The Law of Demeter is really a rule about API or class interface design. When designing classes, it is useful to have a hierarchy of abstractions. You have low level classes that uses the language primitives to directly perform operations and represent objects in an abstraction that is higher level than the language primitives. You have medium level classes that delegate to the low level classes, and implement operations and representations at a higher level than the low level classes. You have high level classes that delegate to the medium level classes, and implement still higher level operations and abstractions. (I've talked about just three levels of abstraction here, but more are possible). This allows your code to express itself in terms of appropriate abstractions at each level, thereby hiding complexity. The rationale for the Law of Demeter is that if you have a chain of method calls, that suggests you have a high level class reaching in through a medium level class to deal directly with low level details, and therefore that your medium level class has not provided a medium-level abstract operation that the high level class needs. But it seems that is not the situation you have here: you did not design the classes in the chain of method calls, they are the result of some auto-generated XML serialization code (right?), and the chain of calls is not descending through an abstraction hierarchy because the des-serialized XML is all at the same level of the abstraction hierarchy (right?)?

View more solutions

Comments

[Arka Ghosh]

Objects.requireNonNull eventually throws NullPointerException. So this doesn't make the situation any different than return wsObject.getFoo().getBar().getBaz().getInt()

[xenteros]

Did you consider getFoo() a very timeconsuming operation? You should store returned values in variables, however it's a waste of memory. Your method is perfect for C programming.

[Andrew Tobilko]

@ArkaGhosh, also it avoids a plenty of ifs as OP showed

[Janki Gadhiya]

but sometime is better to be 1 millisecond late then program getting crashes @xenteros..!!

[xenteros]

getFoo() might be get a value from a server located on another continent. It can last any time: mins/hours...

[NewUser]

Don't you think it's a bad idea to suppress the exception? In realtime, if we lose the trace of an exception, its real pain in the bottom to find out what the hell is going on! I would always suggest not to use chaining. The second problem I see is : This code can't grantee at point of time, which of the result was null.

[xenteros]

Nope, your Exception can have a message which would certainly point the place where it was thrown. I agree chaining isn't the best solution :)

[NewUser]

Nope, it would just say about the line number. So, any of the calls in the chain may lead to an exception.

[Janki Gadhiya]

wsObject will be containing the value returned from Webservice..!! The Service will be called already and wsObject will get a long XML data as a webservice response..!! So there is nothing like server located on another continent because getFoo() is just an element getting getter method not a Webservice call..!! @xenteros

[Marius K.]

This is the only sane solution. All others advise to use exceptions for flow control which is a code smell. On a side note: I consider the method chaining done by the OP a smell as well. If he would work with three local variables and the corresponding if's the situation would be much clearer. Also I think the problem is deeper than just working around a NPE: OP should ask himself why the getters can return null. What does null mean? Maybe some null-object would be better? Or crash in one getter with a meaningful exception? Basically everything is better than exceptions for flow control.

[Marius K.]

@xenteros waste of memory - that's certainly not true. The memory is used very good because it supports readability of the code. But I agree that the answer is flawed it the sense you mentioned.

[Janki Gadhiya]

@MariusK. . I will complete the answer by storing that values in variables but i don't know the type of values returned by these functions : getFoo, getBar, getBaz. the only thing we can get from the question is getFoo().getBar().getBaz().getInt() is an Integer.. That is the reason why i have not used variables here..!!

[Marius K.]

Well from the names of the getters I would assume they return Foo, Bar and Baz objects :P Also consider removing the mentioned double safety from your answer. I don't think it provides any real value aside the pollution of the code. With sane local variables and null checking we have done more than enough to ensure the correctness of the code. If an exception may occur it should be treated as one.

[Marius K.]

You did only half of the edit as the try-catch is still present

[DerM]

It is debatable if it's disobeyes the Law Of Demeter since the WsObject is probably only a data structure. See here: stackoverflow.com/a/26021695/1528880

[Lii]

The unconditional advice to use exceptions to signal the absence of a valid return value is not very good. Exceptions are useful when a method fails in a way which is hard for the caller to recover from and which is better handled in a try-catch-statement in some other part of the program. To simply signal the absence of a return value it is better to use the Optional class, or maybe to return a nullable Integer

[Tom]

@DerM Yes, that is possible, but since OP already has something that parses his XML file, he can also think about creating suitable model classes for required tags, so the parsing library can map them. Then these model classes contain the logic for a null check of its own sub tags.

[Landei]

Very good answer. It should be mentioned that if there are other possible failure conditions, and you need to distinguish between them, you can use Try instead of Optional. Although there is no Try in the Java API, there are many libs providing one, e.g. javaslang.io, github.com/bradleyscollins/try4j, functionaljava.org or github.com/jasongoodwin/better-java-monads

[Boris the Spider]

FClass::getBar etc would be shorter.

[Lii]

@BoristheSpider: Maybe a little. But I usually prefer lambdas to methods refs because often the class names are much longer, and I find lambdas a little easier to read.

[Boris the Spider]

@Lii fair enough, but note that a method reference may be a tiny bit faster, as a lambda may require more complex compile time constructs. The lambda will require the generation of a static method, which will incur a very minor penalty.

[Hulk]

What do you mean with "no matter if you catch it, it can propagate to other parts of the code"?

[SCouto]

If the null is in this sentence wsObject.getFoo() And in later parts of the code you run again that query or use wsObject.getFoo().getBar() (for instance) it will raise again a NullPointerException.

[VLAZ]

"If it's an error and it occurs, You can debug your code" - not in production. I'd much rather know WHAT failed when all I have is a log than trying to divine what happened to cause it to fail. With that advise (and that code), all you really know is that one of 4 things was null, but not which one or why.

[shmosel]

@Lii I actually find method references to be cleaner and more descriptive, even when they are slightly longer.

[Hulk]

That is an unusual wording for "You will have to catch the exception wherever you want to call the method (or it will propagate up the stack)." if I understand correctly. I agree with that (and that may be a problem), I just find the wording to be confusing.

[Kevin Krumwiede]

Unchecked exceptions indicate that the programmer is misusing the API. External problems like "cannot connect to database, IO Exception, Network error" should be indicated by checked exceptions.

[SCouto]

I'll fix it, sorry, English is not my first language so this may happen sometimes :) Thanks

[Lii]

@shmosel: It's an interesting question! I think the lack of explicit arguments makes it a little harder to read, there is no clue in the code about what arguments the method takes so you have to remember that. On the other hand the expression Foo::getBar immediately tells you that it's a method on a Foo, which isn't apparent with a lambda. But on the third hand getBar could be a static method, or could take more than one argument, etc, those things are clear with a lambda but not with a meth-ref. I haven't been able to form a strong opinion about which way is best.

[Hoàng Long]

It really depends on the need of the caller. Checked exception help because it force you to process the error. However, in other cases, it is not necessary and may pollute the code. For example, you have an IOException in your Data layer, will you throw it to Presentation layer? That'd mean you have to catch the exception and re-throw at every caller. I'd prefer wrap the IOException by a custom BusinessException, with a relevant message, and let it pop through the stacktrace, until a global filter catch it and display the message to user.

[Kevin Krumwiede]

Callers don't have to catch and re-throw checked exceptions, just declare them to be thrown.

[Jeroen Mostert]

Three extra seconds on one million requests in the worst case is measurable, but it would rarely be a deal breaker, even in "mission critical applications". There are systems where adding 3.2 microseconds to a request is a big deal, and if you have such a system, by all means think carefully about exceptions. But calling a web service and deserializing its output, per the original question, probably takes much longer than that, and worrying about the performance of exception handling is beside the point there.

[NewUser]

@JeroenMostert: 3 Seconds per check/Million. So, the number of checks will increase the cost

[Jeroen Mostert]

True. Even with that I'd still consider it a case of "profile first", though. You'd need over 300 checks in one request before the request takes a full millisecond extra. Design considerations would weigh on my soul far sooner than that.

[NewUser]

@JeroenMostert: :) Agreed! I would like to leave it up to the programmer with the result and let them take a call!

[Hoàng Long]

@KevinKrumwiede: you are correct, we only need to declare the exception to be thrown. We still need to declare though. Edit: Taking a 2nd look at it, there are quite a lot of debates about checked vs unchecked exception usages (eg: programmers.stackexchange.com/questions/121328/…).

[Developer102938]

This is far less readable in my opinion. It litters the method with a whole bunch of null-checking logic which is completely irrelevant from the actual logic of the method.

[Philippe Gioseffi]

return get(() -> wsObject.getFoo().getBar().getBaz().getInt(), ""); does not give an error in compile time which could be problematic.

[likejudo]

@Lii what would flatmap do in the second case you mention? I don't understand.

[Lii]

@likejudo: The second example is intended to illustrate how the code would look IF the getXXX methods themselves returned Optionals, instead of nullable objects. In that case you have to use flatMap instead of map.

[likejudo]

@Lii I dont understand why we have to use flatmap instead of map.

[Lii]

@likejudo: I think that if you examine the documentation and types of map and flatMap, copy the code a source file and try it out for yourself then you will understand.