9

My problem is the following, I have quite a long Getter, i.e.,

objectA.getObjectB().getObjectC().getObjectD().getObjectE().getName();

Due to "bad" database/entity design (some things were introduced later than others) it happens that getObjectB(), getObjectC() or getObjectD() could return NULL.

Usually we use null-checks all the time, but in this case, I'd have to use

ObjectB b = objectA.getObjectB();
if (b != null) {
    ObjectC c = b.getObjectC();
    if (c != null) {
        ObjectD d = c.getObjectD();
        if (d != null)
           return d.getObjectE().getName();
    }
}
return "";

Instead it would be much easier to simply use a try-catch block

try {
   return objectA.getObjectB().getObjectC().getObjectD().getObjectE().getName();
} catch (NullPointerException e) {
   return "";
}

In this case I don't really care which object returned NULL, it's either display a name or don't. Are there any complications or is it bad design to use try-catch instead of checks?

Thanks for your input.

LordAnomander
  • 1,103
  • 6
  • 16
  • 3
    It's usually a bad design to use Exceptions for a perfectly correct case, this is, if the getter returning `null` is not an error. – Ray O'Kalahjan Aug 19 '16 at 11:21
  • Multiple if/else increases cyclomatic complexity! That's for sure! – N00b Pr0grammer Aug 19 '16 at 11:22
  • What about `if ( objectA.getObjectB() != null && objectA.getObjectB().getObjectC() != null && ...)` ? Java won't evaluate further if the first is false. – Fildor Aug 19 '16 at 11:26
  • @Fildor: That's a lot of repeated calls to methods we don't know are trivial. – T.J. Crowder Aug 19 '16 at 11:27
  • @Fildor true, but that introduces a number of duplicate method calls, which could potentially introduce bugs if they have side-effects. – Clashsoft Aug 19 '16 at 11:27
  • 1
    If your project look like a crap, and you will no re-do project architecture, try to write some kind of performance test and look for difference between null check and try-catch. – degr Aug 19 '16 at 11:27
  • 1
    also, if you will write performance test, please write your result here. – degr Aug 19 '16 at 11:28
  • @T.J.Crowder ,Clashsoft You are completely right. Of course all of your objections would have to be taken into account. I just wanted to name a possibility. Actually I'd suggest a decent refactoring so these constructs become obsolete but I know that this is not always an option. – Fildor Aug 19 '16 at 11:28
  • 1
    This is why java needs a null proof dot operator... `object?.getObjectB()?.getObjectC()...` – vikingsteve Aug 19 '16 at 11:42
  • @vikingsteve: Its already there and called `Optional`. – ST-DDT Aug 19 '16 at 11:51
  • What do you guys think about abstraction of null check ?, ask objectA to provide ObjectE 's name so objectA asks to B then B Asks to C then C asks to D and finally E , if one of the method finds null returns empty string in the end you'll just call objectA.provideObjectEname(); – Ömer Erden Aug 19 '16 at 11:57
  • Whole other approaches are probably better, if you have to use this kind of chain, you might consider Objects#requireNonNull, or Optional with Optional#map. – Joshua Taylor Aug 19 '16 at 12:34

3 Answers3

7

If it is an option to use Java 8, you can use Optional as follows:

Optional.ofNullable(objectA)
    .map(a -> a.getObjectB())
    .map(b -> b.getObjectC())
    .map(c -> c.getObjectD())
    .map(d -> d.getObjectE())
    .map(e -> e.getName())
    .orElse("");
Hoopje
  • 12,677
  • 8
  • 34
  • 50
  • 2
    @ST-DDT This works fine, and I've used it in production code - the map() method on optional will not execute the mapper function unless the Optional contains a value. – Adrian Cox Aug 19 '16 at 12:08
  • I confused Optionals with Streams. – ST-DDT Aug 19 '16 at 12:30
  • @adrian this works if the *first* object is null, but not any of the others, I think. If one of those others returns null, you don't get the empty optional,right? – Joshua Taylor Aug 19 '16 at 12:37
  • @Joshua Taylor If the function returns null, then `Optional.map` returns `Optional.empty()`. – Hoopje Aug 19 '16 at 13:15
  • 2
    @Hoopje Yes, of course, that's right; Optional#map wouldn't be of nearly as much use if it didn't. That's what I get for commenting before my first cup of coffee. :\ – Joshua Taylor Aug 19 '16 at 13:21
4

This kind of method chaining is called a "Train wreck" and is not preferred. Such a statement also violates the Law of Demeter . Let me give you an example from the book Clean code by Robert C Martin:

String scratchDirPath = ctxt.getOptions().getScratchDir().getAbsolutePath();
BufferedOutputStream bos = new BufferedOutputStream(new FileOutputStream(scratchDirPath));
//write to file...

This is similar to what you have and it is a bad practice. This could atleast be refactored to below:

Options options = ctxt.getOptions();
File scratchDir = options.getScratchDir();
String scratchDirPath = scratchDir.getAbsolutePath();
BufferedOutputStream bos = new BufferedOutputStream(new FileOutputStream(scratchDirPath));
//write to file...

This is still violating the Law of Demeter but is at least partially better. The most preferable way would be to find out why the scratchDirPath is needed and ask ctxt object to provide it to you. So it would look like -

BufferedOutputStream bos = ctxt.createScratchDirFileStream();

This way ctxt does not expose all its internals. This decouples the calling code from the implementation of ctxt.

Kiran K
  • 703
  • 4
  • 11
0

It's really a judgement call only you and your team can make, but there's (at least) one objective thing I'll call out: In the case where one of those methods returns null, your first code block will perform better than your second one. Throwing an exception is expensive relative to a simple branch. (In the case where none of the methods returns null, I think the second might be almost unmeasurably faster than the second as you avoid the branching.) In either case, if performance matters, test it in your environment. (But test it properly; micro benchmarks are hard. Use tools.)

Most of time, the difference won't matter in the real world, of course, but that's the significant difference at runtime.

Community
  • 1
  • 1
T.J. Crowder
  • 1,031,962
  • 187
  • 1,923
  • 1,875