Duplicate Code? Replace One/Many Distinctions with Composite

After the first post on how to introduce Null Object, in this second post on how to write clean(er) code, let us look at one of the very frequent occurrence. At least something that I have seen quite often. You too would have noticed that many APIs provide two methods to do the same thing. One of the methods expects one object and the other expects a collection of objects.

Let us look at one of the APIs that we have

[sourcecode language=”java”]
…
public List selectEmployeesBy(Criteria criteria);
public List selectEmployeesBy(List criterion);
…
[/sourcecode]

If you look at the implementation class then we would see code similar to

[sourcecode language=”java”]
public List selectEmployeesBy(Criteria criteria){
…
for (Employee employee: allEmployees){
if (criteria.isSatisfiedBy(employee){
//do something
}
}
…
[/sourcecode]

and the code for the collection would look like

[sourcecode language=”java”]
public List selectEmployeesBy(List criterion){
…
for (Employee employee: allEmployees){
for (Criteria criteria: criterion){
if (criteria.isSatisfiedBy(employee){
//do something
}
}
}

…
[/sourcecode]

so as you would notice, the code becomes a little more uglier and still does a lot of the same things across the two methods. So what are the code smells that you see,

• Explicit Duplicate Code – Because the method that processes one object does the same thing as the method that processes a collection of the objects, duplicated code is often spread across the two methods.
• Non Uniform Client Code – Clients want their objects processed in one way. Yet the existence of two processing methods with different signatures forces clients to pass different kinds of data to the methods

So the first level of optimization would be something like this, the implementation for

public List selectEmployeesBy(Criteria criteria) remains the way it is, the implementation for the collection based method changes to

[sourcecode language=”java”]
public List selectEmployeesBy(List criterion){
…
for (Criteria criteria: criterion){
List partialEmployeeList = selectEmployeesBy( criteria);
}

totalEmployeeList = totalEmployeeList.addAll( partialEmployeeList);
return totalEmployeeList;
[/sourcecode]

So, in a nutshell we are calling the single argument method from our collection method and then merging the list together to return the employees.

But what are the smells that you see here?

• Subtle Duplicate Code – Though we removed the explicit duplicate code by calling one method from the other, there is implicit duplicity because there are two methods trying to do the same thing.
• Non Uniform Client Code – This, is a bigger concern. Whether they have single objects or collections of objects, clients want their objects processed in one way. Yet the existence of two processing methods with different signatures forces clients to pass different kinds of data to the methods

So for resolving the above two smells, the second level of optimization is to change the API and have only one method for the client to access the functionality. And the one method would be

[sourcecode language=”java”]
public List selectEmployeesBy(List criterion);
[/sourcecode]

The implementation would remain the same given the nature of the comparison but having just one method has the following benefits

1. Removes duplicate code associated with handling one or many objects.
2. Provides a uniform way to process one or many objects.
3. Supports richer ways to process many objects (e.g., an OR expression).

I am sure that we have covered points 1 and 2. Point 3 is an interesting point. Assume that you have a Criteria1 which is dark and six feet and Criteria2 is fair and seven feet. So assuming that one of our female colleagues checking out the employees 😉 and she is interested in OR condition. So the result should send us a list of employees meeting either of the criteria.

Using our same method I.e.

[sourcecode language=”java”]
public List selectEmployeesBy(List criterion);
[/sourcecode]

we could define a Criteria which is a composite criteria I.e the composite criteria could have a list of criteria’s

[sourcecode language=”java”]
public class CompositeCriteria {
private List<Criteria> criteria;
public CompositeCriteria(List criteria) {
this.criteria = criteria;
}
[/sourcecode]

Now, to the same method, along with rest of Criteria’s we could also pass the CompositeCriteria and for we would override the isSatisfiedBy() method on the CompositeCriteria class so that it takes all the criterion into account.

We would extend the CompositeCriteria from the Criteria and override the isSatisfiedBy mehod.

Hence, not only we would be able to take care of the two code smells of duplicate code and non-uniform client access but also make it extensible for combination of criteria’s.