Null checking whilst navigating object hierarchies

Question

I had to implement some code which traversed a small object hierarchy to fetch a value and display it in a TextView object (this is Android / Java). I had to do this 6 times to populate 6 TextViews for various values in the object hierarchy.

My implementation was Implementation B. But upon review, my colleague disagreed and certain that Implementation A was the way to go. I believe that my version is not just cleaner, but also less error prone as I could easily miss something as a developer.

Could you provide me with your opinion, with pros and cons for both of these implementations?

Implementation A:

if (house != null && house.getLounge() != null && house.getLounge().getLetter() != null)
{
    String myValue = house.getLounge().getLetter();
    textView.setText(myValue);
}
else
{
    // Do nothing, or maybe make textView hidden.
}

Implementation B:

try
{
    String myValue = house.getLounge().getLetter();
    textView.setText(myValue);
}
catch (NullPointerException e)
{
    // Do nothing, or maybe make textView hidden.
}

Answer 1

Given the scenario you've given, I would go with implementation A.

In both implementations, the case where house is null or house.getLounge() or house.getLounge().getLetter() return null is handled.

One problem with implementation B is that it treats a NullPointerException that could happen in any of the methods called, which is an abnormal occurrence, as something normal. As commenters have pointed out, it "swallows" exceptions. It does not matter one bit if it can be demonstrated that the as of today, the methods called won't ever raise NullPointerException. Code changes, errors are introduced, what we thought was the case is in fact not the case, etc.

Implementation B also obscures the logic. When I look at implementation A, it is clear that the developer guarded against house being null, etc. I know what set of conditions will cause the "Do nothing..." branch to be taken. In implementation B what is the set of conditions that will result in a NullPointerException? Does getLounge sometimes raise a NullPointerException? I don't know without looking elsewhere.

Answer 2

Another option

import java.util.Optional;

Optional.ofNullable(house)
               .map(House::getLounge)
               .map(Lounge::getLetter)
               .ifPresent(letter -> textView.setText(letter));

This avoids using exceptions for flow control but also avoids verbose handling of each possible null. Instead that logic is part of the optional map logic.

Answer 3

Advantage of A:

You make clear, under what condition you want to do something:

house != null && house.getLounge() != null && house.getLounge().getLetter() != null

That is the condition. And you want to get a value and set another value.

String myValue = house.getLounge().getLetter();
textView.setText(myValue);

So in terms of communicating intent, this is clear. Btw. the second condition isn't necessary. If a String is null, nothing should be displayed.

Why B is wrong:

With try-catch you communicate: »I want to do x, which is somehow dangerous« - which it really isn't. Your intent isn't as clear as under A. And - in my eyes worse - you obscure an error-possibility:

String myValue = house.getLounge().getLetter();
textView.setText(myValue);

A NullPointerException is thrown a) when getLounge() returns null and b) when textView is null. Since you are catching one NPE, you do not know which one. Of course in such a simple case it should be easy to debug, but in general this is the wrong way.

I am in favour of a third way:

String myValue = (house.getLounge()!=null)?house.getLounge().getLetter:"";

Of course, there is Diskussion about the usage of the ternary Operator (some languages abandoned the concept), but in this case it fits best: you are able to write a clean oneliner, which provides a default.

Answer 4

I find both options a bit clunky, and would opt for a third:

if (viewShouldRenderForLoungeLetter(house))
{
    String myValue = house.getLounge().getLetter();
    textView.setText(myValue);
}
else
{
    // Do nothing, or maybe make textView hidden.
}

[...]

private bool viewShouldRenderForLoungeLetter(House house){
    return house != null
        && house.getLounge() != null
        && house.getLounge().getLetter() != null;
}

This makes the flow in the initial case easier to read, doesn't swallow other possible NullPointerExceptions, and allows an easy place to stow any additional logic that might crop up in determining whether the view should be rendered. It also could be moved into a separate class if there are other places in the application that use the same logic for "this should only happen if we have a non-null house with a non-null lounge with a non-null letter".

Answer 5

1. Version A is self-describing. It aims to do exactly what it says. Looking at version B taken out of context, I have no idea if the defense is supposed to be against house being null or textField being null (because, maybe, it's available on some layouts and unavailable on some other layouts - this would not be strange when dealing with android resources).

2. When one day textField is null because of some typo introduced to your layout file (which is quite probable in the long run - findViewById return null if it doesn't find ) - version A will make the error obvious and throw a helpful exception. And version B will hide the error, making an easy mistake a really obscure, well hidden bug.

3. The only thing to say for version "b" is that it's 53 characters shorter. Which is way to little to make it a good choice.

Answer 6

I'm late but want to describe why I may prefer B.

Ideally, you only want

String myValue = house.getLounge().getLetter();
textView.setText(myValue);

But there are so many ways that this code will fail. NPE is just one of the possibilities

getLounge() may throw some runtime DBException

getLetter() may return a digit that textView.setText(..) may not accept and throw an exception.

So you wrote a code that would run in case the desired code fails or will fail.

 // Do nothing, or maybe make textView hidden.

Assuming this is a production code, you want to ensure the majority of the functionality. You don't want a failure of the 2 lines of code to affect anything else. Your want to guard it. The perfect guard would be

try
{
    String myValue = house.getLounge().getLetter();
    textView.setText(myValue);
}
catch (Exception e)
{
    // Log the problem
    // Do nothing, or maybe make textView hidden.
}

Just because NPE is the most common and visible threat, you guarded against NPE only. If your intention is to guard NPE only, then you may be better off doing the null checks. Perhaps, you shouldn't have hit this logic in the first place with an appropriate null checks and caching house.getLounge(). But if your intention is to not let a problem of the textView to not get propagated out, I think you could catch Exception.