72

I have some (simplified) code that uses Java Optionals:

Optional<User> maybeTarget = userRepository.findById(id1);
Optional<String> maybeSourceName = userRepository.findById(id2).map(User::getName);
Optional<String> maybeEventName = eventRepository.findById(id3).map(Event::getName);

maybeTarget.ifPresent(target -> {
    maybeSourceName.ifPresent(sourceName -> {
        maybeEventName.ifPresent(eventName -> {
            sendInvite(target.getEmail(), String.format("Hi %s, $s has invited you to $s", target.getName(), sourceName, meetingName));
        }
    }
}

Needless to say, this looks and feels bad. But I can't think of another way to do this in a less-nested and more readable way. I considered streaming the 3 Optionals, but discarded the idea as doing a .filter(Optional::isPresent) then a .map(Optional::get) feels even worse.

So is there a better, more 'Java 8' or 'Optional-literate' way of dealing with this situation (essentially multiple Optionals all needed to compute a final operation)?

17
  • 7
    I think it's a good question for Stack Overflow, although you may also try your luck at Code Review. Personally I don't see a real problem with your current syntax, but if the code is simplified by a lot and the end result is a lot messier, then indeed you may want to look for an alternative design.
    – Mena
    Jan 25, 2018 at 15:47
  • 9
    Don't forget that your design could be bad from get go, and the Optional mess is just a symptom of that. It's hard to evaluate a proper solution based on simplified code though.
    – Kayaman
    Jan 25, 2018 at 15:54
  • 6
    I think to stream the three optionals is an overkill here why not the simple if (maybeTarget.ifPresent() && maybeSourceName.ifPresent() && maybeEventName.ifPresent()) Jan 25, 2018 at 15:55
  • 11
    Then start by not issuing the three calls to the DB at the beginning. Issue the 1st query, if the result is present, issue the 2nd one, and if the result is also present, issue the 3rd query. Finally, if the last result is present, send the email. This avoids extra calls to the DB when either one of the first 2 results is not present. Now extract each DB call to its own private method and chain them with Optional.ifPresent, accumulating the arguments until the last method, which receives the 3 args needed to send the email.
    – fps
    Jan 25, 2018 at 17:52
  • 7
    @FedericoPeraltaSchaffner It would be good to see that in an answer. Jan 25, 2018 at 19:00

13 Answers 13

63

I think to stream the three Optionals is an overkill, why not the simple

if (maybeTarget.isPresent() && maybeSourceName.isPresent() && maybeEventName.isPresent()) {
  ...
}

In my eyes, this states the conditional logic more clearly compared to the use of the stream API.

12
  • 2
    @SharonBenAsher this one is lazy indeed, or you can do it just with java-8 features i guess stackoverflow.com/a/48451409/1059372
    – Eugene
    Jan 25, 2018 at 20:53
  • 14
    @hughjdavey It's not that Optional.isPresent and Optional.get are bad, per se, it's just that many cases are better expressed using map or ifPresent or other combinations of methods on Optional. This case is complicated enough that alternatives aren't necessarily obvious, so use of isPresent and get might be called for here. Jan 25, 2018 at 22:04
  • 2
    This alternative seems structurally identical to doing a bunch of null checks. Assuming OP could go back in time and rewrite the methods to return non-optional values, would this be okay? Or are optionals always better than returning an unwrapped null? Jan 26, 2018 at 0:43
  • 3
    @jeremysprofile Using an Optional wrapped null lets other people who use your code know that you intend for null to be an acceptable value that the code handles. "Always better" is a hard statement to match, because some people would probably argue that in certain optimization cases it's better not to, but generally it doesn't hurt to be more clear and discourage people from shooting themselves in the foot by not considering the null case when it is expected as a valid input.
    – mezzoEmrys
    Jan 26, 2018 at 6:09
  • 4
    Some people say that using isPresent+get is bad, because by allowing then you then have to prove that your programs are safe: you can always forget an isPresent check or make a typo (e.g. missing braces in the if etc). If you always use ifPresent to unwrap the value, or use orElse/orElseGet/orElseThrow you know for sure that an NPE wont happen.
    – Bakuriu
    Jan 26, 2018 at 20:19
33

Using a helper function, things at least become un-nested a little:

@FunctionalInterface
interface TriConsumer<T, U, S> {
    void accept(T t, U u, S s);
}

public static <T, U, S> void allOf(Optional<T> o1, Optional<U> o2, Optional<S> o3,
       TriConsumer<T, U, S> consumer) {
    o1.ifPresent(t -> o2.ifPresent(u -> o3.ifPresent(s -> consumer.accept(t, u, s))));
}

allOf(maybeTarget, maybeSourceName, maybeEventName,
    (target, sourceName, eventName) -> {
        /// ...
});

The obvious downside being that you'd need a separate helper function overload for every different number of Optionals

3
  • 1
    I quite like this as it's at a higher level of abstraction and feels more 'functional' than the other answers... admittedly it is quite a specialized helper interface but all the same. Thanks for your response!
    – hughjdavey
    Jan 25, 2018 at 15:58
  • @JornVernee I like this the most, sort of has the same idea as mine (sort of being the keyword)
    – Eugene
    Jan 25, 2018 at 20:57
  • 4
    I really like this approach as well, however the calls to the DB are all eager, even if the first optional is empty. I think you could easily add the needed laziness if the o2 and o3 were Supplier<Optional<U> and Supplier<Optional<S> respectively. Then you would invoke get() on each supplier from within the lambda of each ifPresent.
    – fps
    Jan 25, 2018 at 21:27
28

How about something like this

 if(Stream.of(maybeTarget, maybeSourceName,  
                        maybeEventName).allMatch(Optional::isPresent))
  {
   sendinvite(....)// do get on all optionals.
  }

Having said that. If your logic to find in database is only to send mail, then if maybeTarget.ifPresent() is false, then there is no point to fetch the other two values, ain't it?. I am afraid, this kinda logic can be achieved only through traditional if else statements.

3
  • Thank you for answering! This certainly makes the whole thing cleaner - the only thing is that we have to use Optional#get in the if block which I have always been advised not to do, but I think perhaps it is still the best thing to do
    – hughjdavey
    Jan 25, 2018 at 15:52
  • 3
    doing a get on optional without the check (isPresent) is a bad idea. But since we have done that in the allMatch check , It is safe to do. Also i don't think the option to use collect and check the size is a good idea, because you are creating a list of at max 3 objects unnecessarily instead of using just a boolean check
    – pvpkiran
    Jan 25, 2018 at 15:54
  • 2
    @pvpkiran thing is, this will execute all the 3 queries even if the first one would be enough to answer the question. read this stackoverflow.com/a/48451409/1059372
    – Eugene
    Jan 25, 2018 at 20:55
28

Since the original code is being executed for its side effects (sending an email), and not extracting or generating a value, the nested ifPresent calls seem appropriate. The original code doesn't seem too bad, and indeed it seems rather better than some of the answers that have been proposed. However, the statement lambdas and the local variables of type Optional do seem to add a fair amount of clutter.

First, I'll take the liberty of modifying the original code by wrapping it in a method, giving the parameters nice names, and making up some type names. I have no idea if the actual code is like this, but this shouldn't really be surprising to anyone.

// original version, slightly modified
void inviteById(UserId targetId, UserId sourceId, EventId eventId) {
    Optional<User> maybeTarget = userRepository.findById(targetId);
    Optional<String> maybeSourceName = userRepository.findById(sourceId).map(User::getName);
    Optional<String> maybeEventName = eventRepository.findById(eventId).map(Event::getName);

    maybeTarget.ifPresent(target -> {
        maybeSourceName.ifPresent(sourceName -> {
            maybeEventName.ifPresent(eventName -> {
                sendInvite(target.getEmail(), String.format("Hi %s, %s has invited you to %s",
                                                  target.getName(), sourceName, eventName));
            });
        });
    });
}

I played around with different refactorings, and I found that extracting the inner statement lambda into its own method makes the most sense to me. Given source and target users and an event -- no Optional stuff -- it sends mail about it. This is the computation that needs to be performed after all the optional stuff has been dealt with. I've also moved the data extraction (email, name) in here instead of mixing it with the Optional processing in the outer layer. Again, this makes sense to me: send mail from source to target about event.

void setupInvite(User target, User source, Event event) {
    sendInvite(target.getEmail(), String.format("Hi %s, %s has invited you to %s",
               target.getName(), source.getName(), event.getName()));
}

Now, let's deal with the optional stuff. As I said above, ifPresent is the way to go here, since we want to do something with side effects. It also provides a way to "extract" the value from an Optional and bind it to a name, but only within the context of a lambda expression. Since we want to do this for three different Optionals, nesting is called for. Nesting allows names from outer lambdas to be captured by inner lambdas. This lets us bind names to values extracted from the Optionals -- but only if they're present. This can't really be done with a linear chain, since some intermediate data structure like a tuple would be necessary to build up the partial results.

Finally, in the innermost lambda, we call the helper method defined above.

void inviteById(UserId targetId, UserId sourceID, EventId eventId) {
    userRepository.findById(targetId).ifPresent(
        target -> userRepository.findById(sourceID).ifPresent(
            source -> eventRepository.findById(eventId).ifPresent(
                event -> setupInvite(target, source, event))));
}

Note that I've inlined the Optionals instead of holding them in local variables. This reveals the nesting structure a bit better. It also provides for "short-circuiting" of the operation if one of the lookups doesn't find anything, since ifPresent simply does nothing on an empty Optional.

It's still a bit dense to my eye, though. I think the reason is that this code still depends on some external repositories on which to do the lookups. It's a bit uncomfortable to have this mixed together with the Optional processing. A possibility is simply to extract the lookups into their own methods findUser and findEvent. These are pretty obvious so I won't write them out. But if this were done, the result would be:

void inviteById(UserId targetId, UserId sourceID, EventId eventId) {
    findUser(targetId).ifPresent(
        target -> findUser(sourceID).ifPresent(
            source -> findEvent(eventId).ifPresent(
                event -> setupInvite(target, source, event))));
}

Fundamentally, this isn't that different from the original code. It's subjective, but I think I prefer this to the original code. It has the same, fairly simple structure, although nested instead of the typical linear chain of Optional processing. What's different is that the lookups are done conditionally within Optional processing, instead of being done up front, stored in local variables, and then doing only conditional extraction of Optional values. Also, I've separated out data manipulation (extraction of email and name, sending of message) into a separate method. This avoids mixing data manipulation with Optional processing, which I think tends to confuse things if we're dealing with multiple Optional instances.

2
  • 7
    Great answer, you've done the refactoring much better than me. Extracting the lookup and sendInvite methods was the way to go, while letting the nested ifPresents. I've learned a good refactoring lesson today, thank you very much.
    – fps
    Jan 26, 2018 at 2:40
  • 3
    @FedericoPeraltaSchaffner Thanks! I think one of the takeaways here is that this isn't only about Optional, but it's also about several small things all taken together, such as names, types, lambdas, and other adjustments and refactorings. Jan 26, 2018 at 7:46
10

I think you should consider taking another approach.

I'd start by not issuing the three calls to the DB at the beginning. Instead, I'd issue the 1st query and only if the result is present, I'd issue the 2nd one. I'd then apply the same rationale with regard to the 3rd query and finally, if the last result is also present, I'd send the invite. This would avoid unnecessary calls to the DB when either one of the first two results is not present.

In order to make the code more readable, testable and maintainable, I'd also extract each DB call to its own private method, chaining them with Optional.ifPresent:

public void sendInvite(Long targetId, Long sourceId, Long meetingId) {
    userRepository.findById(targetId)
        .ifPresent(target -> sendInvite(target, sourceId, meetingId));
}

private void sendInvite(User target, Long sourceId, Long meetingId) {
    userRepository.findById(sourceId)
        .map(User::getName)
        .ifPresent(sourceName -> sendInvite(target, sourceName, meetingId));
}

private void sendInvite(User target, String sourceName, Long meetingId) {
    eventRepository.findById(meetingId)
        .map(Event::getName)
        .ifPresent(meetingName -> sendInvite(target, sourceName, meetingName));
}

private void sendInvite(User target, String sourceName, String meetingName) {
    String contents = String.format(
        "Hi %s, $s has invited you to $s", 
        target.getName(), 
        sourceName, 
        meetingName);
    sendInvite(target.getEmail(), contents);
}
0
9

You can use the following if you want to stick to Optional and not commit to consuming the value immediately. It makes use of Triple<L, M, R> from Apache Commons:

/**
 * Returns an optional contained a triple if all arguments are present,
 * otherwise an absent optional
 */
public static <L, M, R> Optional<Triple<L, M, R>> product(Optional<L> left,
        Optional<M> middle, Optional<R> right) {
    return left.flatMap(l -> middle.flatMap(m -> right.map(r -> Triple.of(l, m, r))));
}

// Used as
product(maybeTarget, maybeSourceName, maybeEventName).ifPresent(this::sendInvite);

One could imagine a similar approach for two, or multiple Optionals, although java unfortunately doesn't have a general tuple type (yet).

1
  • Great example! Using left, middle and right is very clear. THX A LOT.
    – Allen Kerr
    Nov 13, 2019 at 13:19
8

The first approach is not perfect (it does not support laziness - all 3 database calls will be triggered anyway):

Optional<User> target = userRepository.findById(id1);
Optional<String> sourceName = userRepository.findById(id2).map(User::getName);
Optional<String> eventName = eventRepository.findById(id3).map(Event::getName);

if (Stream.of(target, sourceName, eventName).anyMatch(obj -> !obj.isPresent())) {
    return;
}
sendInvite(target.get(), sourceName.get(), eventName.get());

The following example is a little bit verbose, but it supports laziness and readability:

private void sendIfValid() {
    Optional<User> target = userRepository.findById(id1);
    if (!target.isPresent()) {
        return;
    }
    Optional<String> sourceName = userRepository.findById(id2).map(User::getName);
    if (!sourceName.isPresent()) {
        return;
    }
    Optional<String> eventName = eventRepository.findById(id3).map(Event::getName);
    if (!eventName.isPresent()) {
        return;
    }
    sendInvite(target.get(), sourceName.get(), eventName.get());
}

private void sendInvite(User target, String sourceName, String eventName) {
    // ...
}
0
2

Well I took the same approach of Federico to only call the DB when needed, it's quite verbose too, but lazy. I also simplified this a bit. Considering you have these 3 methods:

public static Optional<String> firstCall() {
    System.out.println("first call");
    return Optional.of("first");
}

public static Optional<String> secondCall() {
    System.out.println("second call");
    return Optional.empty();
}

public static Optional<String> thirdCall() {
    System.out.println("third call");
    return Optional.empty();
}

I've implemented it like this:

firstCall()
       .flatMap(x -> secondCall().map(y -> Stream.of(x, y))
              .flatMap(z -> thirdCall().map(n -> Stream.concat(z, Stream.of(n)))))
       .ifPresent(st -> System.out.println(st.collect(Collectors.joining("|"))));
2
  • 7
    This is way more complicated than the original solution - Streams are not the way to go here.
    – Hulk
    Jan 26, 2018 at 12:02
  • @Hulk I know... I did it for fun mainly
    – Eugene
    Jan 26, 2018 at 12:08
2

You can create an infrastructure to handle a variable amount of inputs. For this to be a good design though, your inputs should not be Optional<?>; but Supplier<Optional<?>> so you can short-circuit the unnecessary evaluation of Optionals while trying to determine whether or not all are present.

Because of this, it'd be better to create a utility wrapper around your Optionals that provides transparent access to the evaluated value using a singleton pattern, like the following:

    class OptionalSupplier {

        private final Supplier<Optional<?>> optionalSupplier;
        private Optional<?> evaluatedOptional = null;

        public OptionalSupplier(Supplier<Optional<?>> supplier) {
            this.optionalSupplier = supplier;
        }

        public Optional<?> getEvaluatedOptional() {
            if (evaluatedOptional == null)
                evaluatedOptional = optionalSupplier.get();

            return evaluatedOptional;
        }
    }

Then you can create another class that handles a List of these wrappers and provides a programmatic API to execute a Function that takes as parameters the evaluated values of the actual optionals, hiding further the users involvement in the process. You can overload the method to execute a Consumer with the same parameters. Such class would look something like this:

    class OptionalSemaphores {

        private List<OptionalSupplier> optionalSuppliers;
        private List<Object> results = null;
        private boolean allPresent;

        public OptionalSemaphores(Supplier<Optional<?>>... suppliers) {
            optionalSuppliers = Stream.of(suppliers)
                    .map(OptionalSupplier::new)
                    .collect(Collectors.toList());

            allPresent = optionalSuppliers.stream()
                    .map(OptionalSupplier::getEvaluatedOptional)
                    .allMatch(Optional::isPresent);

            if (allPresent)
                results = optionalSuppliers.stream()
                        .map(OptionalSupplier::getEvaluatedOptional)
                        .map(Optional::get)
                        .collect(Collectors.toList());
        }

        public boolean isAllPresent() {
            return allPresent;
        }

        public <T> T execute(Function<List<Object>, T> function, T defaultValue) {
            return (allPresent) ? function.apply(results) : defaultValue;
        }

        public void execute(Consumer<List<Object>> function) {
            if (allPresent)
                function.accept(results);
        }
    }

Finally all you have left to do is to create objects of this class (OptionalSemaphores) using Suppliers of your Optionals (Supplier<Optional<?>>) and invoking any of the overloaded execute methods to run (IF all Optionals are present) with a List containing the corresponding evaluated values from your Optionals. The following is a full working demo of this:

public class OptionalsTester {

    public static void main(String[] args) {
        Supplier<Optional<?>> s1 = () -> Optional.of("Hello");
        Supplier<Optional<?>> s2 = () -> Optional.of(1L);
        Supplier<Optional<?>> s3 = () -> Optional.of(55.87);
        Supplier<Optional<?>> s4 = () -> Optional.of(true);
        Supplier<Optional<?>> s5 = () -> Optional.of("World");
        Supplier<Optional<?>> failure = () -> Optional.ofNullable(null);
        Supplier<Optional<?>> s7 = () -> Optional.of(55);

        System.out.print("\nFAILING SEMAPHORES: ");
        new OptionalSemaphores(s1, s2, s3, s4, s5, failure, s7).execute(System.out::println);

        System.out.print("\nSUCCESSFUL SEMAPHORES: ");
        new OptionalSemaphores(s1, s2, s3, s4, s5, s7).execute(System.out::println);
    }

    static class OptionalSemaphores {

        private List<OptionalSupplier> optionalSuppliers;
        private List<Object> results = null;
        private boolean allPresent;

        public OptionalSemaphores(Supplier<Optional<?>>... suppliers) {
            optionalSuppliers = Stream.of(suppliers)
                    .map(OptionalSupplier::new)
                    .collect(Collectors.toList());
            
            allPresent = optionalSuppliers.stream()
                    .map(OptionalSupplier::getEvaluatedOptional)
                    .allMatch(Optional::isPresent);

            if (allPresent)
                results = optionalSuppliers.stream()
                        .map(OptionalSupplier::getEvaluatedOptional)
                        .map(Optional::get)
                        .collect(Collectors.toList());
        }

        public boolean isAllPresent() {
            return allPresent;
        }

        public <T> T execute(Function<List<Object>, T> function, T defaultValue) {
            return (allPresent) ? function.apply(results) : defaultValue;
        }

        public void execute(Consumer<List<Object>> function) {
            if (allPresent)
                function.accept(results);
        }
    }

    static class OptionalSupplier {

        private final Supplier<Optional<?>> optionalSupplier;
        private Optional<?> evaluatedOptional = null;

        public OptionalSupplier(Supplier<Optional<?>> supplier) {
            this.optionalSupplier = supplier;
        }

        public Optional<?> getEvaluatedOptional() {
            if (evaluatedOptional == null)
                evaluatedOptional = optionalSupplier.get();

            return evaluatedOptional;
        }
    }
}

Complete code on GitHub

Hope this helps.

0

If you treat Optional just as a marker for method return values, the code becomes very simple:

User target = userRepository.findById(id1).orElse(null);
User source = userRepository.findById(id2).orElse(null);
Event event = eventRepository.findById(id3).orElse(null);

if (target != null && source != null && event != null) {
    String message = String.format("Hi %s, %s has invited you to %s",
        target.getName(), source.getName(), event.getName());
    sendInvite(target.getEmail(), message);
}

The point of Optional is not that you must use it everywhere. Instead, it serves as a marker for method return values to inform the caller to check for absentness. In this case, the orElse(null) takes care of this, and the calling code is fully concious about the possible nullness.

1
  • Whole point of using Optional is to not rely on a null value. A value exists or it doesn't. Might as well got back to just ternary expressions.
    – Big Kahuna
    Aug 5, 2019 at 11:24
0
return userRepository.findById(id)
                .flatMap(target -> userRepository.findById(id2)
                        .map(User::getName)
                        .flatMap(sourceName -> eventRepository.findById(id3)
                                .map(Event::getName)
                                .map(eventName-> createInvite(target, sourceName, eventName))))

First of all you return an Optional as well. It's better to have a method first that creates an invite, which you can call and then send if it's not empty.

Among other things, it's easier to test. Using flatMap you also get the benefit of laziness, since if the first result is empty, nothing else will be evaluated.

When you want to use multiple optionals, you always should use a combination of map and flatMap.

I'm also not using target.getEmail() and target.getName(), those should be safely extracted in createInvite method, since I don't know if they can be nulls or not.

0

Keeping in mind that Exceptions should not be used in this fashion, for conciseness you can consider as well:

try {
  doSomething( optional1.get(), optional2.get(), optional3.get() );
} catch( NoSuchElementException e ) {
  // report, log, do nothing
}
0

Remember, you can define Classes and Records inline to keep the state explicit and flattened vs. nested using callbacks/closures. It might seem a bit overkill for a small example like this, but it really helps when each nested 'chain' ends up doing work.

For example, given your 3 Optionals using lombok:

    @Value @With class Temp {User target; String source; String eventName;}

    maybeTarget
        .map(target -> new Temp(target, null, null))
        .flatMap(tmp -> maybeSourceName.map(tmp::withSource))
        .flatMap(tmp -> maybeEventName.map(tmp::withEventName))
        .ifPresent(tmp -> System.out.printf("Hi %s, %s has invited you to %s%n", tmp.target.getName(), tmp.source, tmp.eventName));

You can do the same w/records but you'll have to do a bit more work since you have to copy everything by hand:

    record TempRecord(User target, String source, String eventName) {}

    maybeTarget
        .map(target -> new TempRecord(target, null, null))
        .flatMap(tmp -> maybeSourceName.map(source -> new TempRecord(tmp.target, source, null)))
        .flatMap(tmp -> maybeEventName.map(eventName -> new TempRecord(tmp.target, tmp.source, eventName)))
        .ifPresent(tmp -> System.out.printf("Hi %s, %s has invited you to %s%n", tmp.target.getName(), tmp.source, tmp.eventName));

I've tried to keep the data immutable and functions pure.

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.