Null check chain vs catching NullPointerException
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 Optional
s 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 Optional
s 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.
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.
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.
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);