Dtyjlui

I have a code fragment that I want to make more concise yet readable
using Java 8 features like lambdas/streams etc.

Basically, there is a list of items and each item has a list of errors.
If there is at least one item with at least one error, "failure" needs to be returned. If no items with any error, return "success".

Optional<List<Item>> optionalItemList = Optional.ofNullable(message.getItems());



if (optionalItemList.isPresent())

    for (Item item : optionalItemList.get()) {

        Optional<List<Error>> optionalErrorList = Optional.ofNullable((item.getErrors()));

        if(optionalErrorList.isPresent())

            if (!optionalErrorList.get().isEmpty()) {

                return "failure";

            }

        }

        return "success";

Optional is not meant to replace if statements, but to be used as a return value of methods. So I think you'd better not use it for this task. You can use the ternary operator along with Stream.allMatch instead:

return message.getItems() == null || 

       message.getItems().stream()

              .allMatch(i -> i.getErrors() == null || i.getErrors().isEmpty()) ?

      "success" :

      "failure";

On a side note, methods should never return null collections. The absence of elements should be expressed by returning empty collections. This would have made your code a lot easier:

return message.getItems().stream().allMatch(i -> i.getErrors().isEmpty()) ?

      "success" :

      "failure";

You can use flatMap to search a list within a list. I personally think that a List should never be null, instead it should be an empty list. If that's a guarantee, then the code could be this:

boolean hasError = message.getItems().stream()

    .flatMap(t -> t.getErrors().stream())

    .findAny()

    .isPresent();

return (hasError ? "success" : "failure");

Otherwise, the code becomes a little longer:

boolean hasError = Optional.ofNullable(message.getItems()).orElse(List.of()).stream()

    .flatMap(t -> Optional.ofNullable(t.getErrors()).orElse(List.of()).stream())

    .findAny()

    .isPresent();

return (hasError ? "success" : "failure");

Note that I could have also used .count() > 0 instead of .findAny().isPresent(). But the disadvantage of the former is that it iterates over all errors, while the latter short-circuits if any error is found.

You can use anyMatch for the iterative code as :

Optional<List<Item>> optionalItemList = Optional.ofNullable(message.getItems());

if (optionalItemList.isPresent())

    if (optionalItemList.get().stream()

            .map(item -> Optional.ofNullable((item.getErrors())))

            .filter(Optional::isPresent)

            .anyMatch(optionalErrorList -> !optionalErrorList.get().isEmpty())) {

        return "failure";

    }

return "success";

or further simplify it as :

return Optional.ofNullable(message.getItems())

        .filter(a -> a.stream()

                .map(item -> Optional.ofNullable((item.getErrors())))

                .filter(Optional::isPresent)

                .anyMatch(optionalErrorList -> !optionalErrorList.get().isEmpty()))

        .map(a -> "failure")

        .orElse("success");

To me you have made it overly complex. Here's a much simpler way of doing it. Make sure your getItems() method returns an empty list if there are no items to return, so that you can dispense with additional null checks as above. This approach is less error prone and leads to more readable code. If you can do the same for getErrors method above, you can merely dispense with the filter(Objects::nonNull) and that will further simplify the stream processing pipeline.

String errorPresent = message.getItems().stream()

    .map(Item::getErrors).filter(Objects::nonNull)

    .map(List::size).filter(s -> s > 0)

    .findAny().map(ignored -> "failure")

    .orElse("success");

Alternatively you may use the ternary operator to get this thing done.

String errorPresent = message.getItems().stream()

    .map(Item::getErrors)

    .filter(Objects::nonNull)

    .anyMatch(e -> !e.isEmpty()) ? "failure" : "success";