9
\$\begingroup\$

I have a Predicate which takes employee object.

Predicate<Employee> getPredicate() {
    return emp -> filter(emp);
}

Now the filter method is very complex, it calls four other methods which returns true/false if all the conditions are true, the predicate will return true.

private boolean filter(Employee employee) {
    String employeeJSONString = employeeToString(employee);
    return filterBasedOnConsistAge(employeeJSONString) && 
           filterBasedOnConsistGender(employeeJSONString) &&
           filterBasedOnConsistNationality(employeeJSONString) &&
           filterBasedOnConsistHandicap(employeeJSONString);
}

private String employeeToString(Employee employee) {
    // converts domainObject to a formatted string, it's a business requirement
}

There are five-line methods, that are linked using logical AND. But the problem here is, the chaining is looking clean. is there a way to improve this logic?

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Your function names make me fear for the worst. \$\endgroup\$
    – gnasher729
    Feb 3 '21 at 19:22
11
\$\begingroup\$

What especially jumps at me is, that you take your real data object (the Employee), convert it to a string representation and do your checks on the string.

Why? Can't you check your data object?

Apart from that, I don't see a problem with 5 and-conditions. This is clearer to read than some clever stream-through-predicates-and-reduce code. Clear. Simple. Leave it like that.

What I'd recommend is rethinking your naming:

  • getPredicate(): yes, it returns a predicate, we see that from the method signature. But what does this predicate test?
  • filter(): does some filtering, but according to which criteria?

(Sorry to pepijno, I typed this without seeing your answer... no offense meant ;-))

\$\endgroup\$
2
  • 1
    \$\begingroup\$ @mtj non taken ;) \$\endgroup\$
    – pepijno
    Mar 24 '20 at 14:09
  • \$\begingroup\$ @GovindaSakhare business requirements should not be dictating your implementation choice. What is the REST service (value-)adding into the String representation? Perhaps you should even consider deserializing that representation to some kind of EnrichedEmployee class and perform the filtering on that data object. String-based filtering is brittle and can be easily undone with data quality issues such as a stray delimiter. \$\endgroup\$
    – h.j.k.
    Mar 26 '20 at 3:23
10
\$\begingroup\$

Each one of the filterBasedOnConsist* methods look like they would be individual predicates themselves. So convert each method into a Predicate class and use the default and method to chain them together into a composite predicate:

Predicate<String> employeePredicate =
    new FilterBasedOnConsistAge()
    .and(new FilterBasedOnConsistGender())
    .and(new FilterBasedOnConsistNationality())
    .and(new FilterBasedOnConsistHandicap())

Use a more descriptive name than employeePredicate. I have no idea what you are using it for so I just put a bad generic name there.

\$\endgroup\$
3
\$\begingroup\$

One option would be to do it with Streams. This is assuming that all different filters are in the same class as the filter method:

private boolean filter(Employee employee) {
  Stream<Predicate<String>> filters = Stream.of(
    this::filterBasedOnConsistAge,
    this::filterBasedOnConsistGender,
    this::filterBasedOnConsistNationality,
    this::filterBasedOnConsistHandicap
  );

  String employeeJSONString = employeeToString(employee);
  return filters.allMatch(f -> f.test(employeeJSONString));
}

The allMatch method of Stream returns true if the condition is true for all elements in the Stream and false otherwise.

\$\endgroup\$
2
  • \$\begingroup\$ This seems like a fairly inefficient way to reimplement the Predicate.and(...) method. \$\endgroup\$ Mar 24 '20 at 14:51
  • \$\begingroup\$ @TorbenPutkonen you are probably right, I forgot about the Predicate.and(...) method \$\endgroup\$
    – pepijno
    Mar 24 '20 at 15:52

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.