How to correct write code with java 8 stream api?

Question

I'm learning Java 8 and I want rewrite usual java code with java Stream. Ho to do it better. My code:

public Set<Product> getProductsByFilter(Map<String, List<String>>filterParams) {

    Set<Product> productsByBrand = new HashSet<Product>();
    Set<Product> productsByCategory = new HashSet<Product>();
    Set<String> criterias = filterParams.keySet();
    if (criterias.contains("brand")) {
        for (String brandName : filterParams.get("brand")) {
            for (Product product : listOfProducts) {
                if (brandName.equalsIgnoreCase(product.
                        getManufacturer())) {
                    productsByBrand.add(product);
                }
            }
        }
    }

    if (criterias.contains("category")) {
        for (String category : filterParams.get("category")) {
            productsByCategory.addAll(this.
                    getProductsByCategory(category));
        }
    }
    productsByCategory.retainAll(productsByBrand);
    return productsByCategory;
}

I don't know how to correnct reformatting code in if (criterias.contains("brand")).

Could you add the API for product? Does it contain a reference to their brand or category/categories? I assume that a product only has one brand but it might have several categories. — Valentin Ruano
– Valentin Ruano, Commented Jun 1, 2018 at 22:53
Also it looks like if any of the two criteria "brand" or "categories" is not specified then the method is supposed to return an empty set.... it more intuitive for it to assume that in case of omission either all brands or all categories are acceptable and so the result is even a bigger set. Can you clarify? — Valentin Ruano
– Valentin Ruano, Commented Jun 1, 2018 at 23:02
I have many brands and many categories. Every Product has single variable referece on the brand (google, dell, ...) and categories (laptop, tablet, smart phone, ...). This simple example for learning Java 8, Spring MVC, ... . — Alex
– Alex, Commented Jun 2, 2018 at 0:28
thanks for the clarification... what about the fact that your code would return an empty product collection if either filter criteria is not in the inputs? — Valentin Ruano
– Valentin Ruano, Commented Jun 2, 2018 at 2:33
Your code looks so weird. If brand criteria exists and category criteria does not exist then return empty. Why is productsByBrand collection ignored? Another point is, if you have access to listOfProducts in brand filter loop, why not use that list in category filter loop? You can just loop once — Mạnh Quyết Nguyễn
– Mạnh Quyết Nguyễn, Commented Jun 2, 2018 at 4:58

Ousmane D. · Accepted Answer · 2018-06-01 22:59:37Z

2

If the code works why change it? , nevertheless, here is a solution using the stream API:

public Set<Product> getProductsByFilter(Map<String, List<String>> filterParams) {

    Set<Product> productsByBrand = filterParams.containsKey("brand") ?
                filterParams.get("brand")
                         .stream()
                         .flatMap(brandName -> listOfProducts.stream()
                                            .filter(product -> brandName.equalsIgnoreCase(product.getManufacturer())))
                         .collect(Collectors.toCollection(HashSet::new)) : new HashSet<>();

   Set<Product> productsByCategory =
            filterParams.containsKey("category") ?
                  filterParams.get("category")
                               .stream()
                               .flatMap(category -> this.getProductsByCategory(category).stream())
                               .collect(Collectors.toCollection(HashSet::new)) : new HashSet<>();

        productsByCategory.retainAll(productsByBrand);
        return productsByCategory;
}

or as suggested by @shmosel, you can avoid the ternary operator with the use of getOrDefault:

Set<Product> productsByBrand = 
       filterParams.getOrDefault("brand", Collections.emptyList())
                   .stream()
                   .flatMap(brandName -> listOfProducts.stream()
                              .filter(product -> brandName.equalsIgnoreCase(product.getManufacturer())))
                   .collect(Collectors.toCollection(HashSet::new));

Set<Product> productsByCategory =
      filterParams.getOrDefault("category", Collections.emptyList())
                  .stream()
                  .flatMap(category -> this.getProductsByCategory(category).stream())
                  .collect(Collectors.toCollection(HashSet::new));

edited Jun 1, 2018 at 22:59

answered Jun 1, 2018 at 22:35

Ousmane D.

56.6k8 gold badges101 silver badges134 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

shmosel Over a year ago

You shouldn't assume toSet() returns a mutable set.

shmosel Over a year ago

Might be cleaner to reduce the scope of the ternary operator: filterParams.containsKey("brand") ? filterParams.get("brand").stream() : Stream.empty() Or avoid it entirely: filterParams.getOrDefault("brand", Collections.singleton()).stream()

Ousmane D. Over a year ago

@shmosel good shout once again although for the second example I believe you meant filterParams.getOrDefault("brand", Collections.emptyList()) as opposed to Collections.singleton().

Valentin Ruano · Accepted Answer · 2018-06-01 23:35:48Z

Trying here to makes it as lambda-stream based as possible.

First the current question code will return an empty set if either criterion (brand or category) is ommited. If this is not a bug but the intended behavior then it makes a lot of sense to check whether that is the case and return the empty set fast. So the code could start like this:

final List<String> filterBrands = filterParams.getOrDefault("brand", Collections.emptyList()));
final List<String> filterCategories = filterParams.getOrDefault("categories", Collections.emptyList()));
if (filterCategories.isEmpty() || filterCategories.isEmpty()) {
  return Collections.emptySet();
}

You could avoid to compose the second list if the first one is empty but it add a few lines of code and the gain would be most likely be super negligible.

For this point on you could use the other answer approach of creating the two criteria product set and then use the intersection. However since you can access the brand aka manufacture directly from the Product there is an alternative that would not require the instantiation of such sets explictly we might be an improvement in performance depending on the ration of the size of the return versus each criterion sets. So if the result is just a few products ... say 5 but each criterion or one of the criterion set may end up being a few 1000s you may save memory or cpu churning doing the following:

final Set<String> manufacturers = filterBrands.stream()
        .map(String::toLowerCase)
        .collect(Collectors.toSet());

return filterCategories.stream()
    .flatMap(this::getProductsByCategory)
    .distinct() // optional but might be better performance 
                // if categories share products.
    .filter(product -> manufacturers.contains(product.getManufacturer().toLowerCase()))
    .collect(Collectors.toSet());

Collectives™ on Stack Overflow

How to correct write code with java 8 stream api?

2 Answers 2

3 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

3 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related