Refactoring multiple boolean conditions?

Question

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?

• gnasher729: Your function names make me fear for the worst.
• Toby Speight: Please don't change the code in your question after it's answered - it makes the answers seem nonsensical. You could post improved code as a new question, as an answer, or as a link to an external site - as described in I improved my code based on the reviews. What next?. I have rolled back the edit, so the answers make sense again.

Answer 1

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 ;-))

• pepijno: @mtj non taken ;)
• h.j.k.: @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.

Answer 2

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.

Answer 3

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.

• TorbenPutkonen: This seems like a fairly inefficient way to reimplement the Predicate.and(...) method.
• pepijno: @TorbenPutkonen you are probably right, I forgot about the Predicate.and(...) method