5
\$\begingroup\$

There is A LOT of information online stating that you should NEVER catch a NullPointerException. Generally I agree, but I am wondering about this one case.

I have inherited code that requires me to access data that I need in the following way

context.getGrandParent().getParent().getChild().isRequired()

There is no guarantee that any of the objects in this hierarchy will not be null. I have to enter a block if isRequired() returns true. First, and what I initially wrote, with null checks:

if(context != null
   && context.getGrandParent() != null
   && context.getGrandParent().getParent() != null
   && context.getGrandParent().getParent().getChild() != null 
   && context.getGrandParent().getParent().getChild().isRequired()
){
   // continue with business logic
} else {
   LOG.error("Unable to determine if processing is required.");
}    
// continue with other inherited code

Setting aside that I could refactor this, perhaps for readability, wouldn't it make more sense to do the following?

boolean isRequired = false;
try {
   isRequired = context.getGrandParent().getParent().getChild().isRequired();
} catch (NullPointerException npe) {
   LOG.error("Unable to determine if processing is required.");
}

if(isRequired){
   // continue with business logic
}
// continue with other inherited code
\$\endgroup\$
  • 1
    \$\begingroup\$ some notes on null propagation: oracle.com/technetwork/articles/java/… \$\endgroup\$ – dfhwze May 30 '19 at 21:40
  • \$\begingroup\$ yes i like the version with an exception. You should not make a code where normal funtionality is that some variable is present or not. You sould not make if-else branches instead of exception cases ( like in AJNeufeld answer) in error log howewer you must determine what exactly place is absent) \$\endgroup\$ – user8426627 May 30 '19 at 22:24
  • \$\begingroup\$ I agree, use the exception, guidelines are for the obedience of fools and the guidance of wise men. This is a case were clarity reigns. \$\endgroup\$ – Martin Spamer May 31 '19 at 0:15
  • \$\begingroup\$ @dfhwze: Thanks for the link. It gives a good overview of Optional that AJNeufeld recommends. \$\endgroup\$ – TCCV May 31 '19 at 14:35
  • \$\begingroup\$ .getParent().getChild() wait what? Doesn't that go back to where we came from? \$\endgroup\$ – Simon Forsberg Aug 20 '19 at 14:16
6
\$\begingroup\$

The problem with catching NullPointerException is “which one did you catch?” A null can be returned from getGrandParent(), and using that return value without checking will cause the exception. OR a bug in getGrandParent() might cause an exception while trying to find the parent’s parent, and you are obscuring the bug by assuming the NullPointerException results from a properly returned null value.

You can use Optional to properly capture the null and not call subsequent function.

Optional<Boolean> isRequired = Optional.ofNullable(context)
                          .map(Context::getGrandParent)
                          .map(GrandParent::getParent)
                          .map(Parent::getChild)
                          .map(Child::isRequired);

if (!isRequired.isPresent()) {
    LOG.error("Unable to determine if processing is required.");
} else if (isRequired.get()) {
    // continue with business logic
}

The Context::, GrandParent::, Parent::, Child:: class types are, of course, WAG's. You'll need to supply the corrent types based on the type returned by the previous stage.

Alternately, you could use .getOrElse(Boolean.FALSE) at the end of the .map() chain.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ This may be the only case where creating an Optional to avoid an explicit null check would be a good idea. \$\endgroup\$ – TorbenPutkonen May 31 '19 at 7:06
  • \$\begingroup\$ Thank you for this. I think I will go this route. For academic reasons, would you have the same opinion if all of the getters are actually just getters with no logic? In that case there would be no bug (it would just return a reference to a member or null). \$\endgroup\$ – TCCV May 31 '19 at 14:24
  • \$\begingroup\$ @TCCV Absolutely. For what might be a getter with no logic today may become a computation or delegation method tomorrow. \$\endgroup\$ – AJNeufeld May 31 '19 at 15:20
2
\$\begingroup\$

The solution of @AJNeufeld is elegant and solves your immediate problem. However, I want to make a case for doing things the verbose way: check for each bad condition individually, log an error if the value is null, and continue onward otherwise. This has two advantages.

First, it allows custom error messages depending on which value is null. If a missing field is a serious error in your data, your error log should have tons of information about it! Simply giving up and logging a generic error does not help you fix the database.

Second, it allows you to assign each object to a variable. Presumably, you need to use at least some of these objects inside of the business logic, so having variables bound to each will avoid extra calls to the getters later on.

The resulting code would look something like this.

if (context == null) {
    LOG.error("Cannot process: context is null.");
    return;
}

GrandParent grandParent = context.getGrandParent();
if (grandParent == null) {
    LOG.error(String.format("Cannot process: context %s has null grandParent.", context));
    return;
}

Parent parent = grandParent.getParent();
if (parent == null) {
    LOG.error(String.format("Cannot process: grandParent %s has null parent.", grandParent));
    return;
}

Child child = parent.getChild();
if (child == null) {
    LOG.error(String.format("Cannot process: parent %s has null child.", parent));
    return;
}

if (child.isRequired()) {
    // business logic
}

The code is admittedly quite verbose, but it is readable and clear. In production, useful error messages like these can be much more important than concise code.

| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ It also avoids a possible concurrent execution logic problem in the second code block of the OP's post, where context.getGrandParent() != null evaluates to true, but by the time && context.getGrandParent().getParent() != null is evaluated, getGrandParent() now returns a null and results in the NullPointerException anyway. Keeping each object, as it is retrieved, in a local variable prevents the item you just tested from changing just after you tested it. \$\endgroup\$ – AJNeufeld May 31 '19 at 15:27

Not the answer you're looking for? Browse other questions tagged or ask your own question.