8
\$\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
}

filterBasedOnConsist* are five-line methods. Above several AND conditions doesn't look clean. if there a way to improve this logic?

\$\endgroup\$
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 ;-))

|improve this answer|||||
\$\endgroup\$
  • 1
    \$\begingroup\$ @mtj non taken ;) \$\endgroup\$ – pepijno Mar 24 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 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.

|improve this answer|||||
\$\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.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ This seems like a fairly inefficient way to reimplement the Predicate.and(...) method. \$\endgroup\$ – TorbenPutkonen Mar 24 at 14:51
  • \$\begingroup\$ @TorbenPutkonen you are probably right, I forgot about the Predicate.and(...) method \$\endgroup\$ – pepijno Mar 24 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.