6
\$\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\$

closed as off-topic by 200_success, yuri, Toby Speight, IEatBagels, esote Jun 1 at 19:15

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – 200_success, yuri, Toby Speight, esote
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 1
    \$\begingroup\$ some notes on null propagation: oracle.com/technetwork/articles/java/… \$\endgroup\$ – dfhwze May 30 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 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 at 0:15
  • \$\begingroup\$ @dfhwze: Thanks for the link. It gives a good overview of Optional that AJNeufeld recommends. \$\endgroup\$ – TCCV May 31 at 14:35
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.

\$\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 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 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 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.

\$\endgroup\$
  • 1
    \$\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 at 15:27

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