8

I'm working with a legacy application where I often have to access properties deeply nested like that:

a.getB().getC().getD().getE().getF()

The problem is that it's possible that in any depth, the value could be null. Of course I could check each depth like that:

public Optional<F> retrieveValue(A a) {
  if(a != null && a.getB() != null && a.getB().getC() != null && 
     a.getB().getC().getD() != null && 
     a.getB().getC().getD().getE() != null && 
     a.getB().getC().getD().getE().getF() != null) {
    return Optional.of(a.getB().getC().getD().getE().getF());
  } else {
    return Optional.empty();
  }
}

F existingOrCreated = retrieveValue(a).orElse(new F());

But I have to do this in many places for many different objects so the code gets bloated with these checks. Mostly the objects are non null but there are some rare cases where objects are null. Is there a way to do it in a more concise way? I'm using Java 8 but I can't update the legacy code provided.

Christian
  • 22,585
  • 9
  • 80
  • 106
  • Funnily enough, I was confronted with a similar problem. After a few unsuccessful attempts I got annoyed and postponed it to Christmas, where there is little going on at work and I have more time. I will try your approach soon. Thanks for sharing. – Eritrean Sep 10 '19 at 20:22
  • @ChristianWörz on OleksandrPyrohov's answer: actually, you can store any intermediate object as a `Function` and continue building up the chain after applying the function (if it's not too much for you) ;) – Andrew Tobilko Sep 10 '19 at 21:49
  • @OleksandrPyrohov I was going to upvote, but I didn't catch it... I was thinking of something like `reduce(a, [A::getB, B::getC,...], () -> new F())` but it wouldn't be *that* nice and concise with Java... – Andrew Tobilko Sep 10 '19 at 22:06
  • 2
    There is a Java feature that a lot of people seem to miss: *local variables*. They allow to store a value temporarily, to avoid repeating the expression which produced the value. Besides that, `return Optional .ofNullable(a).map(A::getB).map(B::getC).map(C::getD).map(D::getE).map(E::getF);`… is that really that bad (compared to the problem of needing such a chain)? – Holger Sep 11 '19 at 07:22

2 Answers2

8

Before going into my answer I'd like to point out that these chains should be avoided in the first place. (Check the Law of Demeter).

Now to the problem. Because Java, unlike other languages like Kotlin does not provide simpler checking for nulls, until Java 8 you had only the choice to check each level of your object.

But since Java 8 offers function passing as parameters, I started using this feature to build my own "Safe Calls" in Java:

public static <T> Optional<T> retrieveValue(Supplier<T> getter) {
  try {
    return Optional.ofNullable(getter.get());
  } catch (NullPointerException e) {
    return Optional.empty();
  }
}

Let's first see how this method is called in your given case:

F f = retrieveValue(() -> a.getB().getC().getD().getE().getF()).orElse(new F());

As you can see, we just pass a Supplier to retrieveValue and call it in a try-catch block where we catch the possible NullPointerException and return an Optional.empty() in that case.

A small disadvantage of this try-catch approach is, that it's slower than simple null check in the code if a NullPointerException occurs during checking.

Another advantage of this approach is, that this method is reusable for any other null checking in any depth and does not depend on a specific object structure.

Node node = retrieveValue(() -> root.getNode().getNode()).orElse(new Node());
Naman
  • 27,789
  • 26
  • 218
  • 353
Christian
  • 22,585
  • 9
  • 80
  • 106
  • 1
    Sorry, I don't see how the solution that suggests catching a NPE could be a good one – Andrew Tobilko Sep 10 '19 at 20:57
  • 2
    @AndrewTobilko I see your point but IMO in this case catching a NPE is acceptable because it's in a controlled environment where a NPE is expected. Of course it would be better to not have to do that at all but having to do these nested checks everywhere in the code is unnecessary bloating the code. – Christian Sep 10 '19 at 20:59
  • 1
    I mean `a.getB().getC().getD().getE().getF()` looks like **the** problem, why would someone be digging so deep from `a` to `f`? shouldn't they be given an access only to `e`? (I guess it's called "the principle of least knowledge" as the guys pointed out) – Andrew Tobilko Sep 10 '19 at 21:04
  • Often also `a` or `a.getB()` is used in the same method but sure, the call isn't optimal. – Christian Sep 10 '19 at 21:06
  • You can skip the `Optional` wrapper by taking a default value as an argument to `retrieveValue`. – jaco0646 Sep 10 '19 at 21:09
  • If you were to catch a `NullPointerException`, you could have done it while accessing the attribute as `try { return Optional.of(a.getB().getC().getD().getE().getF()); } catch (NullPointerException e) { return Optional.empty(); }`, why use a `Supplier` for that? Besides that, [don't ever catch a NullPointerException](https://stackoverflow.com/questions/18265940/when-is-it-ok-to-catch-nullpointerexception) for you might just be covering up a bug. – Naman Sep 11 '19 at 04:06
  • The problem is not that it is slower if a `NullPointerException` occurs, the problem is that you don’t know whether the exception was truly caused by one of the elements of the chain being `null` or by a bug inside one of the `get…` methods. – Holger Sep 11 '19 at 07:16
0

Is there any chance that you could change the design such that A could simply have a getF method and the chaining is hidden in the other classes?

Something like:

public class E {
    ...
    public F getF() {
        return f; // F can be null
    }
    ...
}

public class D {
    ...
    public F getF() {
        e == null ? null : e.getF();
    }
    ...
}

public class C {
    ...
    public F getF() {
        d == null ? null : e.getF();
    }
    ...
}

public class B {
    ...
    public F getF() {
        c == null ? null : e.getF();
    }
    ...
}

public class A {
    ...
    public F getF() {
        b == null ? null : e.getF();
    }
    ...
}

This way the null just gets propagated through the classes. You only need to check if a is null, or if a.getF() is null at the end, which will look way cleaner.

arcadeblast77
  • 560
  • 2
  • 12
  • 1
    No in the given scenario the pojos are set and cannot be changed. That is the main point why I provided that solution. It's a well known problem that some parts of the system cannot be changed. – Christian Sep 11 '19 at 00:24