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.
}

Well, implementation B silently swallows legitimate bugs manifesting as null pointer exceptions in any of the methods or on the `textView.*` access. — , Sep 05 '14 at 18:47
What about code inside the house and the lounge? While a getter should simply return a value, this is not always the case (e.g. lazy initialization). delnan's comment is quite valid in that regard. — , Sep 05 '14 at 19:09
Also: [Are exceptions as control flow considered a serious antipattern? If so, Why?](http://programmers.stackexchange.com/q/189222/22815) and [Arguments for or against using Try/Catch as logical operators](http://programmers.stackexchange.com/a/107727/22815) — , Sep 05 '14 at 19:13
@Amorgos As in `this != null`? I'm no Java guru but I think the method invocation will throw before the method is entered if `textView` is null. In case you refer to other things that might be null inside `setText`, consider that those checks too can be buggy/incomplete. — , Sep 05 '14 at 19:16
@delnan For this example, consider textView being not null. and setText internal null check being ok. — Eurig Jones, Sep 05 '14 at 19:26
I agree with @delnan, implementation B will definitely mask bugs. It also hides which methods may return null and which don't. Unfortunately I don't think you'll find a solution that's all of: 1) precise (doesn't mask unintended exceptions), 2) concise (doesn't add a ton of boilerplate), and 3) efficient (doesn't duplicate the getter calls) without access to lambdas. This is a pretty clear-cut case for [Optional.flatMap](http://docs.oracle.com/javase/8/docs/api/java/util/Optional.html#flatMap-java.util.function.Function-). — Doval, Sep 05 '14 at 19:36
I would just like to point out, that in my own code I try to avoid scenarios where I have to do null-checks as much as possible. This means *always initialising values to sensible defaults*. For my own classes this might mean designing in behaviour that does something sensible when unitialised, or using `final` for members to ensure they are. This is a *fourth way* to add to @ThomasJunk's third way below. — robert, Sep 06 '14 at 12:03
also http://stackoverflow.com/questions/3393341/what-would-we-do-without-null — robert, Sep 06 '14 at 12:04
Have you considered an alternative model entirely? “Tell, don’t ask” doesn’t remove the need for checks, but delegates the check to the owning objects. https://www.martinfowler.com/bliki/TellDontAsk.html — axl, Jun 23 '18 at 06:34

score 5 · Answer 1 · answered Sep 05 '14 at 19:31

5

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.

answered Sep 05 '14 at 19:31

Louis

217
1
5

An internal NullPointerException occurring in any of the getters accessed will not occur as they are simple POJO objects. But even if they did it would be confirmation that the value at the bottom of the tree we are trying to get to is not available. – Eurig Jones Sep 05 '14 at 19:38
Today it's a POJO, and tomorrow it becomes more complicated. As I said, code changes. You're not coding just for today. – Louis Sep 05 '14 at 19:39
1

@Amorgos If you're so sure they'll never have any logic more complicated than `return field`, there wouldn't be a need for them to have getters. By giving them getters you're acknowledging that this may not always be the case in the future. – Doval Sep 05 '14 at 19:51
@Doval But even if there were extra logic in the getters causing a potential NPE, then you would want to treat that NPE exactly the same as if the value you're trying to fetch at the bottom of the object hierarchy isn't there. – Eurig Jones Sep 05 '14 at 20:12
1

@Amorgos No; if you're expecting `null` and get a Null Pointer Exception instead, that's a bug. You can't ignore that. – Doval Sep 05 '14 at 20:15

Winston Ewert · Answer 2 · 2022-03-03T04:37:33.073

5

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.

edited Mar 03 '22 at 04:37

answered Jun 24 '18 at 21:20

Winston Ewert

24,732
12
72
103

Yes. This is the way which one (I) would choose with Java>=8. This should be the accepted answer here. Also: http://www.oracle.com/technetwork/articles/java/java8-optional-2175753.html – Thomas Junk Jun 25 '18 at 07:35
What java version? There is no fromNullable() method in version 17 – ka3ak Mar 02 '22 at 15:54
@ka3ak, it looks like it should have been `ofNullable`. `fromNullable` is the Guava equivalent method. – Winston Ewert Mar 03 '22 at 04:37

score 3 · Answer 3 · answered Sep 06 '14 at 08:04

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.

score 1 · Answer 4 · edited Mar 04 '22 at 15:09

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".

I'm 7 years late, but this should be the answer. Naming the method `showLoungeLetter` might be a better name, IMHO. Refactoring this logic into its own method makes the original question irrelevant. Additionally, you can split these null checks into multiple lines to make it easier to read. — Greg Burghardt, Mar 04 '22 at 15:12

score 0 · Answer 5 · answered Sep 05 '14 at 20:25

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).
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.
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.

score -2 · Answer 6 · answered Jun 23 '18 at 05:12

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.

I think you should rethink your solution. This is as close to "failing silently" as it gets. Of course, you have an alibi log entry. You are silencing any meaningful errors. As I said: in this sandbox it might be okay, but in general I would want to speak with the author of such code. — Thomas Junk, Jun 24 '18 at 19:56
The code will fail silently if the catch block is left empty. Maybe the textView can be set to a meaningful error message or the catch block can return false to indicate failure. What I'm more interested than the developer's experience is the user's experience. Since the context is about the TextView, I would not want its failure to propagate anywhere else. Just like JavaScript, I would want only the errored portion to fail and everything else to succeed. If the context was to develop a public API, I might have suggested otherwise. — Sgene9, Oct 06 '18 at 07:02
Now that I think of it, I should add that, if the Exception may be recovered by the callee, the it should be rethrown. I guess such situation would be the majority. I might argue that a proper OOD object should know how to fix/initialize itself, but that's just a thought. Anyways, my intention is that, at some point, when the Exception is determined to be unrecoverable and the developer would have to decide whether the user should know about this, I would prefer that the user doesn't know about it if it doesn't affect the user workflow. I agree this wasn't a suitable answer for this question. — Sgene9, Oct 06 '18 at 07:30

Null checking whilst navigating object hierarchies

Implementation A:

Implementation B:

6 Answers6