3
public class Product {

    private String name;
    private List<Image> images;
    //getters and setters

      public class Images {
        private String url;
        private List<Thumbnail> thumbnails;

        // getters and setters

        public class Thumbnail{

          //getters and setters
          private String url;
        }
      }
    }
}

I have this class. I need to fetch product name, url of the first image and url of the first thumbnail. I also need to make sure product, images and thumbnails are non-empty.

This is how I am trying to do it:

Optional.ofNullable(product).ifPresent(productData -> {
  response.setProductName(productData.getName());

  Optional.ofNullable(productData.getImages()).ifPresent(images -> {
    images.stream().findFirst().ifPresent(image -> {
      response.setImageUrl(image.getUrl());

        Optional.ofNullable(image.getThumbnails()).ifPresent(thumbnails -> {
        thumbnails.stream().findFirst().ifPresent(thumbnail -> {
          response.getThumbnailUrl();
        });
      });
    });
  });
});

Is there a better way?

mufeez-shaikh
  • 179
  • 2
  • 14

4 Answers4

3

You shouldn't ever have null lists. An empty list represents the same thing without having to introduce a null check. That lets you get rid of two ifPresent checks.

Optional.ofNullable(product).ifPresent(productData -> {
  response.setProductName(productData.getName());

  productData.getImages().stream().findFirst().ifPresent(image -> {
    response.setImageUrl(image.getUrl());

    image.getThumbnails().stream().findFirst().ifPresent(thumbnail -> {
      response.getThumbnailUrl();
    });
  });
});

Optionals and streams aren't really doing you any favors. Don't use them just because they exist. Consider just using regular if statements.

if (product == null) {
    return;
}

if (!product.getImages().isEmpty()) {
    Image image = product.getImages().get(0);
    response.setImageUrl(image.getUrl());

    if (!image.getThumbnails().isEmpty()) {
        response.getThumbnailUrl();
    }
}

Alternatively, you could simulate stream().findFirst().ifPresent() with for loops that break after the first iteration.

if (product == null) {
    return;
}

for (Image image: product.getImages()) {
    response.setImageUrl(image.getUrl());

    for (Thumbnail thumbnail: image.getThumbnails()) {
        response.getThumbnailUrl();
        break;
    }

    break;
}
John Kugelman
  • 349,597
  • 67
  • 533
  • 578
3

@John's answer is the correct one. If you can, return empty lists.

It might also be that you want to distinguish between not having any items and not having a result (in your case it doesn't make much sense, but we're talking hypothetically). Then return Optional<List<T>> instead of returning null then converting it. Then @Johannes' answer is the correct one.

Another way of thinking about the problem is, if you have no control over the return values, to convert it to a stream to chain the calls:

Optional.ofNullable(possiblyNullProduct).stream()
  .peek(product -> response.setProductName(product.getName()))
  .map(Product::getImages)
  .filter(Objects::nonNull)
  .map(images -> images.stream().findFirst())
  .filter(Optional::isPresent).map(Optional::get)
  .peek(image -> response.setImageUrl(image.getUrl())
  .map(Image::getThumbnails)
  .filter(Objects::nonNull)
  .map(thumbnails -> thumbnails.stream().findFirst())
  .filter(Optional::isPresent).map(Optional::get)
  .forEach(thumbnail -> response.getThumbnailUrl());

Optional::stream was added in Java 9.

This is just another solution, by no means a better solution. I would welcome any comments on the performance.

Another option to get the first item of each list is to convert to an Optional and back into a Stream:

  .map(Product::getImages)
  .filter(Objects::nonNull)
  .flatMap(List::stream).findFirst().stream() // <- changed here
  .peek(image -> ...)

And you can also change out the last three lines in a similar way:

  .map(Image::getThumbnails)
  .filter(Objects::nonNull)
  .flatMap(List::stream).findFirst() // <- from here
  .ifPresent(thumbnail -> response.getThumbnailUrl());
Druckles
  • 3,161
  • 2
  • 41
  • 65
2

Yes, flatMap will help here:

Optional<Product> optProduct = Optional.ofNullable(product);
optProduct.ifPresent(p -> response.setProductName(p.getName()));
Optional<Image> optImage = optProduct.flatMap(p -> Optional.ofNullable(p.getImages()))
    .stream().flatMap(il -> il.stream()).findFirst();
optImage.ifPresent(i -> response.setImageUrl(i.getUrl()));
optImage.flatMap(i -> Optional.ofNullable(i.getThumbnails()))
    .stream().flatMap(tl -> tl.stream()).findFirst()
    .ifPresent(t -> response.getThumbnailUrl()); // WTF?

Node that Optional.stream() was added in Java 9.

Johannes Kuhn
  • 14,778
  • 4
  • 49
  • 73
1

I don't think the Optionals are helping much here. I would clean up the code by abstracting the iteration logic, like this:

private <T> void findFirst(List<T> list, Consumer<T> action) {
    if (list != null) {
        list.stream()
            .findFirst()
            .ifPresent(action);
    }
}

if (product != null) {
    response.setProductName(productData.getName());
    findFirst(productData.getImages(), image -> {
        response.setImageUrl(image.getUrl());
        findFirst(image.getThumbnails(), thumbnail -> response.setThumbnailUrl(thumbnail.getUrl()));
    });
}
shmosel
  • 49,289
  • 6
  • 73
  • 138