Filtering a list of customers in Java based on three criteria

Question

Let's assume that you need to filter customers based on 3 criteria:

• name
• tags assigned to customer
• customer's location

For some reasons you have to do it directly in your program code, without database queries or PL/SQL for that matter.

Which of these two solutions do you find better in any terms? (e.g code readability, performance, cleanliness)

First example

public List<Customer> filterStepbyStep(List<Customer> customers, 
                                       String searchString, 
                                       List<Tag> selectedTags,
                                       List<LocationId> selectedLocations) {
    List<Customer> filteredCustomers = new ArrayList<Customer>(customers);
    boolean searchStringNotEmpty = searchString != null && !searchString.trim().isEmpty();
    boolean locationsNotEmpty = selectedLocations != null && selectedLocations.size() > 0;
    boolean selectedTagsNotEmpty = selectedTags != null && selectedTags.size() > 0;

    filteredCustomers.removeIf(customer -> {
        boolean valid = true;

        //searchString
        if (valid && searchStringNotEmpty) {
            valid = customer.name.contains(searchString);
        }

        //locations
        if (valid && locationsNotEmpty) {
            valid = selectedLocations.stream().anyMatch(locationId -> locationId.equals(customer.locationId));
        }

        //tags
        if (valid && selectedTagsNotEmpty) {
            boolean tagValid = false;
            for (Tag selectedTag : selectedTags) {
                for (Tag customerTag : customer.getTags()) {
                    if (selectedTag == customerTag) {
                        tagValid = true;
                        break;
                    }
                }
                if (tagValid) {
                    break;
                }
            }
            valid = tagValid;
        }

        return !valid;
    });

    return filteredCustomers;
}

Second example

public List<Customer> filterWithLambda(List<Customer> customers,
                                       String searchString, 
                                       List<Tag> selectedTags,
                                       List<LocationId> selectedLocations) {
    List<Customer> filteredCustomers = new ArrayList<Customer>(customers);
    boolean searchStringNotEmpty = searchString != null && !searchString.trim().isEmpty();
    boolean locationsNotEmpty = selectedLocations != null && selectedLocations.size() > 0;
    boolean selectedTagsNotEmpty = selectedTags != null && selectedTags.size() > 0;

    filteredCustomers.removeIf(customer -> {
        return (searchStringNotEmpty && !customer.name.contains(searchString))
                || (locationsNotEmpty && !selectedLocations.stream().anyMatch(locationId -> locationId.equals(customer.locationId)))
                || (selectedTagsNotEmpty && !selectedTags.stream().anyMatch(selectedTag ->
                      customer.getTags().stream().anyMatch(customerTag ->
                        selectedTag == customerTag)));
    });

    return filteredCustomers;
}

For me second example is more readable, I can immediately see that customer gets filtered out (removed) if any of 3 operands of OR operator is true.
On the other hand repeating if(valid) for each property in first example does not seem too clean to me. I also find it tiresome and tedious to follow valid variable. Having this kind of state with valid variable is IMO error-prone.
The only reason I can see, that one would prefer first example is not being familiar with lambda expressions.

I've created small repl.it to try these approaches out.

If you can think of better way of doing this, than two above examples, then please feel free to post it here.

Answer 1

As far as which is easier to read, there isn't a clearly correct answer. The second is more shorter, but more cramped. I personally prefer code with a little more whitespace, such as the first example. Others prefer code that's more succinct. Which is more readable is probably a question to ask your coworkers.

Marking variables as final will clue the reader in that they don't change once assigned.

In camelCase, the method name should be filterStepByStep.

Both methods use lambdas, so the name filterWithLambda is misleading.

In recent versions of Java, you don't need to specify the type on the right hand side of a generic assignment. You can just use <>.

You can get rid of 'valid' by returning false early.

The equals check for locations can be cleaned up using customer.locationId::equals, or, better, by just using contains.

The check on tags can be cleaned up using Set operations.

I would consider renaming the variables to matchCustomerName, matchLocation, and matchTags to make the checks read more cleanly.

If you were to make all these changes, your code might look more like:

public static List<Customer> filterStepByStep(
        final List<Customer> customers,
        final String searchString,
        final List<Tag> selectedTags,
        final List<LocationId> selectedLocations) {

    final List<Customer> filteredCustomers = new ArrayList<>(customers);
    final boolean matchCustomerName = searchString != null && !searchString.trim().isEmpty();
    final boolean matchLocation = selectedLocations != null && selectedLocations.size() > 0;
    final boolean matchTags = selectedTags != null && selectedTags.size() > 0;


    filteredCustomers.removeIf(customer -> {
        if (matchCustomerName && !customer.name.contains(searchString)) {
            return true;
        }

        if (matchLocation && !selectedLocations.contains(customer.locationId)) {
            return true;
        }

        if (matchTags) {
            final Set<Tag> matchedTags = new HashSet<>(selectedTags);
            matchedTags.retainAll(customer.getTags());
            return matchedTags.isEmpty();
        }

        return false;
    });

    return filteredCustomers;
}

public List<Customer> filterWithLambda(
        final List<Customer> customers,
        final String searchString,
        final List<Tag> selectedTags,
        final List<LocationId> selectedLocations) {

    final List<Customer> filteredCustomers = new ArrayList<>(customers);
    final boolean matchCustomerName = searchString != null && !searchString.trim().isEmpty();
    final boolean matchLocations = selectedLocations != null && selectedLocations.size() > 0;
    final boolean matchTags = selectedTags != null && selectedTags.size() > 0;

    filteredCustomers.removeIf(customer -> {
        return (matchCustomerName && !customer.name.contains(searchString))
                || (matchLocations && !selectedLocations.contains(customer.locationId))
                || (matchTags && !selectedTags.stream().anyMatch(customer.getTags()::contains));
    });

    return filteredCustomers;
}

Answer 2

Of #1 and #2, I prefer #2. But I actually dislike them both.

For every customer, you are not only testing for the desired criteria, you are also testing the searchStringNotEmpty, locationsNotEmpty and selectedTagsNotEmpty flags. If you have 1,000,000 customers, that may be up to 3,000,000 tests of unchanging boolean variables.

What you want is more like:

List<Customer> filteredCustomers = new ArrayList<Customer>(customers);

if (searchStringNotEmpty)
    filterCustomers.retainIf(c -> c.name.contains(searchString));

if (locationsNotEmpty)
    filterCustomers.retainIf(c -> locations.contains(c.locationId));

if (selectedTagsNotEmpty)
    filterCustomers.retainIf(c -> /* c contains any of the selectedTags */);

Instead of searchStringNotEmpty being tested once for each customer, it is tested once, and only if true is the filtering performed for the search string.

Except ... if you have 1,000,000 customers, you may only want to pass through the list once, (to avoid cache thrashing) and the above will pass through the list up to 3 times. So let's fix that so we pass through the list once.

Dynamic Programming

What we want to do is write a program to write a program to filter the customers.

Technically, we'll just be writing a program to assemble a program to filter the customers. Enter stream programming.

Stream<Customer> stream = customers.stream();

if (searchStringNotEmpty)
    stream = stream.filter(c -> c.name.contains(searchString));

if (locationsNotEmpty)
    stream = stream.filter(c -> locations.contains(c.locationId));

if (selectedTagsNotEmpty)
    stream = stream.filter(c -> /* c contains any of the selectedTags */);

List<Customer> filteredCustomers = stream.collect(Collectors.toList());

We start with the stream of customers. If a search string is present, we add the appropriate filter to the stream, and store the resulting stream back in our stream variable. In the same way, we can add the other two filters. Once the stream pipeline has been configured, we perform the collect() terminal operation, which actually begins the stream processing, collecting all Customer objects which pass through the filters (if any) into a list.

Note: Perhaps you just want to return the resulting stream to the caller, instead of a filteredCustomers list, to allow the caller to do additional stream processing on the result. For instance, the caller might want to take the results and partition them into a map based on location, in which case first collecting the customers into a list is unnecessary step.

Predicates

As an alternative to building up a stream pipeline, with filters, you could build a complex Predicate from individual predicates for the 3 filter conditions. The predicates would simply combine with .and() to form a larger predicate.

Predicate<Customer> filter = c -> true;

if (searchStringNotEmpty)
    filter = filter.and(c -> c.name.contains(searchString));

if (locationsNotEmpty)
    filter = filter.and(c -> locations.contains(c.locationId));

if (selectedTagsNotEmpty)
    filter = filter.and(c -> /* c contains any of the selectedTags */);

List<Customer> filteredCustomers = new ArrayList<Customer>(customers);
filteredCustomers.retainIf(filter);

You could avoid the extra c -> true filter stage, by initializing filter to null, and then either assign to filter, or execute filter = filter.and(...) depending on whether filter is null or not at each step. At the end, simply don't call .retainIf(filter) if filter is still null at the end. That would be more efficient, but the above is easier to understand.

Optimizations

Both selectedLocations and selectedTags are lists. We can improve things by turning these each into a Set for faster .contains() testing.

(Of course, this requires both LocationId and Tag to properly implement Object.hashCode() in order to function properly.)

(Unlike @EricStein's answer, these sets would be constructed once, not once per customer.)

We can even do some other optimizations based on the sizes of these lists. A list of 1 can be handled much simpler, with equals() in the location case and a simple .contains() for the tag case.

In the multiple tags case, Collections.disjoint() can be used to to test for common tags.

Stream<Customer> stream = customers.stream();

if (searchString != null && !searchString.trim().isEmpty())
    stream = stream.filter(c -> c.name.contains(searchString));

if (selectedLocations != null && selectedLocations.size() > 0) {
    if (selectedLocations.size() == 1) {
        var location = locations.get(0);
        stream = stream.filter(c -> location.equals(c.locationId));
    } else {
        var location_set = new HashSet<>(locations);
        stream = stream.filter(c -> location_set.contains(c.locationId));
    }
}

if (selectedTags != null && selectedTags.size() > 0) {
    if (selectedTags.size() == 1) {
        var tag = selectedTags.get(0);
        stream = stream.filter(c -> c.getTags().contains(tag));
    } else {
        var tag_set = new HashSet<>(selectedTags);
        stream = stream.filter(c -> !Collections.disjoint(tag_set, c.getTags()));
    }
}

List<Customer> filteredCustomers = stream.collect(Collectors.toList());