4
\$\begingroup\$

I have the following concerns:

  1. Is Set a good choice for getting products? What are drawbacks of Set for this purpose?

  2. Is it better to do something like common.retainAll() or to get products through a request with WHERE to the database. For now I do three requests in: getProductsByCategory, getProductsByManufacturer, getProductsByPrice. So maybe it would be better to make just one?

    @RequestMapping("/{category}/{price}")
    public String filterProducts(@PathVariable("category") String productCategory,
            @MatrixVariable(pathVar = "price") Map<String, List<String>> priceParams,
            @RequestParam("manufacturer") String manufacturer, Model model) {
    
        Set<Product> productsByCategory = service.getProductsByCategory(productCategory);
        Set<Product> productsByManufacturer = service.getProductsByManufacturer(manufacturer);
    
        BigDecimal low;
        BigDecimal high;
    
        low = new BigDecimal(priceParams.get("low").get(0));
        System.out.println(low);
        high = new BigDecimal(priceParams.get("high").get(0));
        System.out.println(high);
    
        Set<Product> productsByPrice = service.getProductsByPrice(low, high);
    
        Set<Product> common = new HashSet<Product>(productsByCategory);
    
        common.retainAll(productsByManufacturer);
        common.retainAll(productsByPrice);
    
        model.addAttribute("products", common);
    
        return "products";
    
    }
    
\$\endgroup\$
4
\$\begingroup\$

OK, so I assume this is the code from your @Controller, there's a @Service that it calls, and the Service further calls a @Repository (the DAO layer). Furthermore, I shall assume you're using the latest version of Java, i.e. Java 8.

Ultimately I find it all depends on the number of products you have. Given not too many, you could do all the filtering in Java. Given thousands, you'd better do it in the database.

One method in the database layer

In any case, one database call is definitely much better than three, so you could just create a generic enough database search method to which you can pass all the search parameters. When you create the method, you could also create a data transfer object named, say, SearchParameters that contains all the search parameters. That way your method signatures don't grow to ridiculous lengths even when you search with several parameters. The interface will also be easy to change, because in order to add a new search parameter the method signature won't have to change, just the data transfer object.

Personal experience

The way I've solved a similar issue, with just a couple of dozen products, was that my DAO layer had a trivial public List<Product> findAll() method with a @Cacheable annotation so that after the first call to the DB the further ones would only access a fast cache in the memory.

The Service would then have the methods you described: getProductsByCategory, getProductsByManufacturer and getProductsByPrice, but they'd all just call the findAll() method of the DAO (quickly getting the cached response) and they'd filter the results based on their parameters.

For example (assuming the Product class has a getManufacturer() method that returns a String:

public Set<Product> getProductsByManufacturer(final String manufacturer) {
    return dao.findAll().stream()
        .filter(product -> Objects.equals(product.getManufacturer(), manufacturer)
        .collect(Collectors.toSet());
}

And that's it using Java 8's streams. The filter only includes those products whose manufacturer matches the given parameter.

One method in the service layer

However, since you need an intersection of three sets, you might as well create one method to do all of your filtering. Either to the DAO layer like I proposed in the beginning, or do it in your service layer if the amount of data is not too big, or databases aren't your strong suit. ;)

So, your business code would boil down to something like this:

New SearchParameters class:

import java.util.Optional;

public class SearchParameters {
    // Here with just one search parameter for brevity, but you'll get the idea
    String manufacturer;

    public boolean manufacturerMatches(final Product product) {
        return Optional.ofNullable(manufacturer).map(m -> m.equals(product.getManufacturer())).orElse(false);
    }
}

A method in your Service class:

public Set<Product> getProducts(final SearchParameters params) {
    return dao.findAll().stream()
        .filter(params::manufacturerMatches)
        .collect(Collectors.toSet());
}

Then just add more of those matcher methods to the SearchParameters class and deploy them as filter lines in your Service -- you can chain filters, so it's easy and really concise.

In case you're not familiar with Java 8's streams, .filter(params::manufacturerMatches) is a short-hand notation for .filter(product -> params.manufacturerMatches(product)), which in turn compares all the products against the manufacturerMatches method and only allows a product to go through if the method returns true.

Answers

So, to answer your questions...

  1. Sets are basically good for holding those Products of yours in case you are not interested in the order of the products. If that matters, you need to use some List instead. But that's not really what you asked or wanted to know, so:

    The issue with Sets in your code is that you duplicate or triplicate a lot of data and operate on many more objects than you need to in order to achieve your objective. Comparing big product objects is also more expensive (in terms of CPU cycles) than simple Strings or other such parameters, which would be enough for you. You could have all of the objects (Products) in one collection and just filter that. Also when you compare your objects in Sets, you need to be sure your equals and hashCode methods for the Products are always up to date -- if you forget to update them when you add a new field, you will get wrong results.

  2. Yes, it's best to do just one DB call -- and even that one call can sometimes be omitted by using a cache. You can achieve that "one call only" by creating a bit more complicated DB query to account for all the search parameters or by handling the searching (filtering) in Java. Which way should you do it, depends on the factors I've listed above, i.e. mainly about the number of products in your database and your personal preference. But I wouldn't really use the Sets, as (like I said) they multiply the amount of objects you have, and also because I just don't find that .retainAll(...) kind of code very readable.

|improve this answer|||||
\$\endgroup\$
2
\$\begingroup\$
System.out.println(low);
System.out.println(high);

I hope these are just "debugging" statements, and you should 'de-clutter' them as early as possible... :)

return "products";

Really? The output of this method to filter products is just to return a String with the word "products"? Depending on the subsequent usage, you can consider returning the model itself so that callers can perform operations on it immediately, for example:

filterProducts("category", priceParams, "Maker", model)
    .addAttribute("attributeX", anotherSet);

At the very least, a boolean return value may also make sense too indicating whether the model was indeed filtered or not, for example:

boolean isFiltered = filterProducts(category, priceParams, "Maker", model);
if (isFiltered) {
    // a logging framework, e.g. SLF4J
    logger.info("Filtered for '{}' products: {}", category, 
                    getDescriptions(model.getAttribute("products"));
}

Is it better to do something like common.retainAll() or to get products through a request with WHERE to the database.

I will always suggest defaulting to running more efficient SELECT statements on the database, mainly for two important reasons:

  1. Minimize network traffic to transfer the results from the database to your application, which tends to be the source of significant latencies (disk/network speed is far slower than doing computations in-memory).
  2. Most good databases can efficiently optimize these kinds of filtering for you in the background (either by tuning how it handle the queries, or even to cache subsets of data), so there is literally less code to write, optimize and maintain.

Anyways, going back to your code, you may also want to consider inlining your service calls to make it more streamlined:

// Assuming you are on at least Java 7 too, for '<>' generic type declaration to work
@RequestMapping("/{category}/{price}")
public boolean filterProducts(@PathVariable("category") String productCategory,
        @MatrixVariable(pathVar = "price") Map<String, List<String>> priceParams,
        @RequestParam("manufacturer") String manufacturer, Model model) {   
    // actually, any special reason why this is wrapped in a new HashSet?
    Set<Product> common = new HashSet<>(service.getProductsByCategory(productCategory));
    boolean result = common.retainAll(service.getProductsByManufacturer(manufacturer)) & 
                        common.retainAll(service.getProductsByPrice(
                            getPrice(priceParams, "low"), getPrice(priceParams, "high")));
    model.addAttribute("products", common);
    return result;
}

private static BigDecimal getPrice(Map<String, List<String>> priceParams, String param) {
    return new BigDecimal(priceParams.get(param).get(0));
}
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ That "streamlined" version with the inlined service calls looks quite horrible to me... Code should primarily be readable, and the original version is much more so than your "streamlined" version. There's no shame in creating intermediary variables with descriptive names. \$\endgroup\$ – ZeroOne Oct 16 '15 at 23:35
  • \$\begingroup\$ @ZeroOne that's a fair point. :) \$\endgroup\$ – h.j.k. Oct 17 '15 at 5:16

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.