3

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.
}
  • 7
    Well, implementation B silently swallows legitimate bugs manifesting as null pointer exceptions in any of the methods or on the textView.* access. – user7043 Sep 5 '14 at 18:47
  • textView.setText() has an internal null check. – Eurig Jones Sep 5 '14 at 18:51
  • 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. – user22815 Sep 5 '14 at 19:09
  • 1
  • @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. – user7043 Sep 5 '14 at 19:16
4

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.

  • 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 5 '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 5 '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 5 '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 5 '14 at 20:12
  • @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 5 '14 at 20:15
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.

3

Another option

import java.util.Optional;

Optional.fromNullable(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.

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

-2

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.

  • 2
    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 6 '18 at 7: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 6 '18 at 7:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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