Returning CompletableFuture in a conditional function

Question

I have an use case where I have to return a CompletableFuture<Void> from a function that composes 2 Completable Futures based on a condition.

Below is what I have right now -

private CompletableFuture<Void> processSomething(final SomeEvent event) {
    final CompletableFuture<PaginatedQueryList<Detail>> detail = dao.getData(event.getOrderId());
    return detail.thenApply(n -> n.stream()
        .filter(i -> i.getBusinessType().equals(BusinessType.AWESOME))
        .findFirst()
        .filter(i -> i.getLastUpdateEventTimestamp() <= event.getCreationTime())) // This returns Optional<Detail>
        .thenCompose(i -> i
           .map(o -> deleteItem(event,o))
           .orElse(CompletableFuture.completedFuture(null))); // deleteItem is a async call that returns CompletableFuture<Void>
}

Can the community check and and suggest any other approach ?
I particularly do not like returning explicitly CompletableFuture.completedFuture(null).

For me this code is perfectly fine. When you have to return a `CompletableFuture` but you only get one conditionally, your only option is to return an already completed future in the alternative case, but it is perfectly fine. Note that it might be a better fit for [codereview.se] since your code works already. — Didier L, Dec 07 '19 at 12:38
@DidierL Thanks for your comments, here and in below suggested answer. Also, thanks for the Code Review website, I did not know about this, but now I do. Thanks. — dipamchang, Dec 07 '19 at 23:44

score 2 · Answer 1 · answered Dec 09 '19 at 11:53

There is nothing wrong with returning an already completed future in a function for thenCompose. As mentioned in this answer, you can also use CompletableFuture.allOf() to denote an empty list of tasks, to the same result.

But you may use .orElseGet(() -> …) to avoid constructing the completed future in advance even when unneeded.

Further, you may replace your chain of thenApply(…).thenCompose(…) with a single thenCompose(…):

private CompletableFuture<Void> processSomething(final SomeEvent event) {
    CompletableFuture<PaginatedQueryList<Detail>> detail = dao.getData(event.getOrderId());
    return detail.thenCompose(n -> n.stream()
        .filter(i -> i.getBusinessType().equals(BusinessType.AWESOME))
        .findFirst()
        .filter(i -> i.getLastUpdateEventTimestamp() <= event.getCreationTime())
        .map(o -> deleteItem(event, o))
        .orElseGet(() -> CompletableFuture.completedFuture(null)));
// or   .orElseGet(() -> CompletableFuture.allOf()));
// or   .orElseGet(CompletableFuture::allOf));

}

Unknown · Accepted Answer · 2019-12-07T13:23:09.347

0

One of the way to achieve it would be, simply make two changes.

Pass Optional<Detail> as argument to the delete method and make necessary checks inside it before performing actual logic.
Change .thenCompose() to .thenAccept(). It will return CompletionStage<Void> which you can cast to CompletableFuture<Void> before returning at the top of method return.

private CompletableFuture<Void> processSomething(final SomeEvent event) {
    final CompletableFuture<PaginatedQueryList<Detail>> detail = dao.getData(event.getOrderId());
    return (CompletableFuture<Void>)detail.thenApply(n -> n.stream()
        .filter(i -> i.getBusinessType().equals(BusinessType.AWESOME))
        .findFirst()
        .filter(i -> i.getLastUpdateEventTimestamp() <= event.getCreationTime())) // This returns Optional<Detail>
        .thenAccept(i -> deleteItem(event, i).join())); // deleteItem is a async call that returns CompletableFuture<Void>
}

private CompletableFuture<Void> deleteItem(Optional<Detail> optionalDetail){
     if(optionalDetail.isPresent()){
       // Your existing logic
     }
}

Suppose if you don't want to change deleteItem() method signature, then simply do it as,

.thenAccept(i -> 
     {
      if(i.isPresent()){
         deleteItem(event, i.get());
      }
     });

edited Dec 07 '19 at 13:23

answered Dec 07 '19 at 07:08

Unknown

531
5
18

If you use `thenAccept()` instead of `thenCompose()` the returned future will be completed before `deletItem()` has completed. – Didier L Dec 07 '19 at 12:35
You could stall the current thread inside `thenAccept()` by calling `join()` on `deleteItem()` until it completes right? Or should it not be? – Unknown Dec 07 '19 at 12:52
Well, that would defeat the purpose a little because it means you would consume 2 threads instead of one. Note that `Optional` is not designed to be used as a method parameter. Also, `isPresent()` followed by `get()` is also an anti-pattern or at least a code smell (sometimes you can't avoid it, at least in Java 8). Just use `ifPresent()` instead. You will also notice that your new `deleteItem()` method still needs to return a `completedFuture(null)` in the else case. – Didier L Dec 07 '19 at 15:32
@DidierL Yeah, I agree that it would consume 2 threads. And regarding anti-pattern, I think I learned something new today. Thanks for that information. – Unknown Dec 07 '19 at 16:52
What makes you say that `thenAccept()` returns a `CompletionStage` and must be cast? https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/CompletableFuture.html – daniu Dec 09 '19 at 11:58
@daniu : Sorry for the misconception. Yeah I know that too. Actually, I always prefer to work with interfaces(`CompletionStage`) rather than concrete types(`CompletableFuture`). This confusion is bcoz, in my example that I worked out, `dao.getData()` returns `CompletionStage<>` so, chained `thenAccept` will return `PreviousMethodReturnType`. And I thought of suggesting to use the Interfaces instead of Concrete types but missed that piece of info. – Unknown Dec 09 '19 at 12:56

Returning CompletableFuture in a conditional function

2 Answers2