5

Instead of multiple null checks in below case, I am planning to add something readable code instead. May be with the help of java 8 streams/maps. Can somebody help me with this

private String getRailsServiceClass(IRailsComponent railsComponent) {
        String serviceClass = "";
        if (railsComponent != null && railsComponent.getRailOffer() != null && railsComponent.getRailOffer().getRailProducts().get(0).getRailProduct() != null && railsComponent.getRailOffer().getRailProducts().get(0).getRailProduct().getFareBreakdownList() != null &&
                railsComponent.getRailOffer().getRailProducts().get(0).getRailProduct().getFareBreakdownList().get(0).getPassengerFareList() != null && railsComponent.getRailOffer().getRailProducts().get(0).getRailProduct().getFareBreakdownList().get(0).getPassengerFareList().get(0).getPassengerSegmentFareList() != null &&
                railsComponent.getRailOffer().getRailProducts().get(0).getRailProduct().getFareBreakdownList().get(0).getPassengerFareList().get(0).getPassengerSegmentFareList().get(0).getCarrierServiceClassDisplayName() != null) {
            return railsComponent.getRailOffer().getRailProducts().get(0).getRailProduct().getFareBreakdownList().get(0).getPassengerFareList().get(0).getPassengerSegmentFareList().get(0).getCarrierServiceClassDisplayName();
        }
        return serviceClass;
    }
Rocker rock
  • 155
  • 2
  • 11
  • I think this is related: https://stackoverflow.com/questions/271526/avoiding-null-statements?rq=1 – Endery Jun 16 '17 at 07:30
  • Maybe this article will be helpful http://www.baeldung.com/java-getters-returning-null?utm_content=buffer97745&utm_medium=social&utm_source=linkedin.com&utm_campaign=buffer – Alexey Semenyuk Jun 16 '17 at 07:41
  • 5
    There is something called “local variables” which can hold the result of an expression evaluation, eliminating the need to repeat that expression over and over again… – Holger Jun 16 '17 at 08:36

4 Answers4

31

You can use Optional for your purpose.

String serviceClass = Optional.ofNullable(railsComponent)
           .map(IRailsComponent::getRailOffer)
           .map(RailOffer::getRailProducts)
           ...
           .orElse("");
Mykola Yashchenko
  • 5,103
  • 3
  • 39
  • 48
  • 5
    It’s the first time I’ve seen a good use of `Optional` for something else than the return value from a method. Bravo. Still better, of course, is those methods (or at least some of them) could be modified to return an `Optional`. – Ole V.V. Jun 16 '17 at 09:07
  • @Nikolay Thanks for sharing this. But I am getting error - CannotResolvemethod 'getRailProduct' in the 3rd case of mapping. – Rocker rock Jun 16 '17 at 09:31
  • You should use exactly your methods, of course. I write just example of using because I haven't seen your classes. An error that you mentioned means that you written the name of method which doesn't exist in your class. – Mykola Yashchenko Jun 16 '17 at 09:33
  • I have value inside getRailProducts().get(0).getRailProduct(), what about in this case ? – Rocker rock Jun 16 '17 at 09:37
  • 1
    I'm not sure, because I don't know your interfaces but something like that: ...map(RailOffer::getRailProducts).filter(item -> !item.isEmpty()).map(item -> item.get(0)).map(item -> item.getRailProduct()); – Mykola Yashchenko Jun 16 '17 at 09:41
5

Your code is bad because on each check you had to get the items in the lists again and again. That's a lot of I/O to do.

Without using any API (so pre-Java 8 solution) You can clean you code by checking each item one after all (depends on the accessiblity of each class but here is a fully developped condition

RailComponent rc = getRailComponent();
    if (rc != null) {
        RailOffer ro = rc.getRailOffer()
        if (ro != null) {
            RailProduct rp = ro.getRailProducts().get(0).getRailProduct();
            if (rp != null) {
                List<FareBreakDown> fbList = rp.getFareBreakdownList();
                if (fbList != null) {
                    List<PassengerFare> pfList = fb.get(0).getPassengerFareList();
                    if (pfList != null) {
                        List<PassengerSegmentFare> psfList = pfList.get(0).getPassengerSegmentFareList();
                        if (psfList != null) {
                            String carrierServiceClassDisplayName = psfList.get(0).getCarrierServiceClassDisplayName();
                            if (carrierServiceClassDisplayName != null) {
                                return carrierServiceClassDisplayName;
                            }
                        }
                    }
                }
            }
        }
    }

You see that's not THAT much of check once you reduce the verbosity of your code

AxelH
  • 14,325
  • 2
  • 25
  • 55
3

inspired by kotlin safe call operator ?. & elivs opreator ?:, you can chain a custom SafeCallable. for example:

String serviceClass = SafeCallable.of(railsComponent)
                                  .then(IRailsComponent::getRailOffer)
                                  .then(RailOffer::getRailProducts)
                                  .then(products -> products.get(0))
                                  .then(...)
                                  .orElse("");

SafeCallable

interface SafeCallable<T> {
    T call();

    static <T> SafeCallable<T> of(T value) {
        return () -> value;
    }

    // ?. operator
    default <R> SafeCallable<R> then(Function<T, R> step) {
        return then(step, () -> null);
    }

    // ?: operator
    default T orElse(T value) {
        return then(identity(), () -> value).call();
    }

    default <R> SafeCallable<R> then(Function<T, R> step, Supplier<R> otherwise) {
        T value = call();
        return value == null ? otherwise::get : () -> step.apply(value);
    }
}
holi-java
  • 29,655
  • 7
  • 72
  • 83
  • 1
    I can't really tell why you would need the two-argument `then` at all here. If you removed it, you could simplify this `default SafeCallable then(Function step) {Objects.requireNonNull(step); T t = call(); return t == null ? () -> null : () -> step.apply(t);}` and also `default T orElse(T value) { T t = call(); return t == null ? value : t; }` – Eugene Jun 16 '17 at 11:31
  • 1
    and then it looks a lot like `Optional` *already*; and the two argument `then` looks a lot like `map.orElseGet`... – Eugene Jun 16 '17 at 11:34
  • @Eugene hi, sir. the `then(..., ...)` can remove duplication between `then(..)` and `orElse(...)`. just as jdk-9 [Optional.ifPresentOrElse](http://download.java.net/java/jdk9/docs/api/java/util/Optional.html#ifPresentOrElse-java.util.function.Consumer-java.lang.Runnable-). :) – holi-java Jun 16 '17 at 11:36
  • not really; `then.orElse` will return `T`; while `then(...,...)` will return a `SafeCallable` – Eugene Jun 16 '17 at 11:43
  • @Eugene I don't put the whole code in the answer. Indeed, I have another one: `SafeCallable orElse(R value)`. the answer I have inline it to avoiding call as `orElse(foo).call()` . – holi-java Jun 16 '17 at 11:45
1

If any of those nulls are rare, i would use a try catch block:

private String getRailsServiceClass(IRailsComponent railsComponent) {
  try {
    return railsComponent.getRailOffer().getRailProducts().get(0)
           .getRailProduct().getFareBreakdownList().get(0).getPassengerFareList().get(0)
           .getPassengerSegmentFareList().get(0).getCarrierServiceClassDisplayName();
  } catch (NullPointerException e) {
    return "";
  }
}
Usagi Miyamoto
  • 6,196
  • 1
  • 19
  • 33
  • 1
    You should at least add logging of an exception. Without logging user of your code can't understand why method returns empty String in some cases. – Mykola Yashchenko Jun 16 '17 at 07:36
  • 2
    Original did not log the possible null values, too. – Usagi Miyamoto Jun 16 '17 at 07:38
  • Okay, I see. It's just suggestion. It's not required to add :) – Mykola Yashchenko Jun 16 '17 at 07:39
  • 4
    I don't think it's a bad idea because of lack of logging, but rather because exceptions should only be used for exceptional circumstances, not merely rare ones. Unless it is truly a logic error for these things to be null, I wouldn't use try/catch to handle it. – David Conrad Jun 16 '17 at 07:42
  • 6
    @David Conrad: the original code is a big code smell anyway, considering all these `List`s which are either `null` (instead of empty) or have only a single element (of interest)… – Holger Jun 16 '17 at 08:40
  • 1
    @Holger Oh, I agree. – David Conrad Jun 16 '17 at 10:31