Duplication in a codebase is problematic for several reasons. First, if the code really should be the same, it makes it easy to introduce bugs if you need to change it. The more places the same snippet of code exists, the more chances you have to forget to implement changes. Perhaps more importantly, if some parts of the code are intentionally different, it can become hard to tell whether the differences are intentional.

Today, I ran across a code snippet that looked something like this. The comment was there already – as much as I wish otherwise, I didn’t add it.

final Class queryClass = queryType.getTypeClass();
// TODO Yuck, this could use some refactoring
if (queryClass.equals(Project.class)) {
Workspace workspace = RallyRequestContext.getCurrent().getWorkspace();
Collection<Project> accessibleProjs = filter(workspace.getProjects(), accessibleTo(user));
Collection<Long> oids = transform(accessibleProjs, toOids());
criteria.add(new InstancesForOidsCriterion(oids));
} else if (queryClass.equals(Scope.class)) {
List<Long> oids = newArrayList();
Workspace workspace = RallyRequestContext.getCurrent().getWorkspace();
oids.add(workspace.getOID());
Collection<Project> accessibleProjs = filter(workspace.getProjects(), accessibleTo(user));
for (Project project : accessibleProjs) {
oids.add(project.getOID());
}
criteria.add(new InstancesForOidsCriterion(oids));
} else if(queryClass.equals(AttributeConfiguration.class)) {
List<Long> oids = newArrayList();
Workspace workspace = RallyRequestContext.getCurrent().getWorkspace();
oids.add(workspace.getOID());
Collection<Project> accessibleProjs = filter(workspace.getProjects(), accessibleTo(user));
for (Project project : accessibleProjs) {
oids.add(project.getOID());
}
criteria.add(new AttributeConfigurationsInScopesCriterion(oids));
}

Clearly, there’s duplication here, both subtle and blatant. There are subtle differences in the three blocks above. Can you tell which are intentional and which are just noise?

First, there’s the alternating usage of Google Guava’s Collections2.transform and for each loops. Let’s at least make those consistent. That gives us this:

final Class queryClass = queryType.getTypeClass();
// TODO Yuck, this could use some refactoring
if (queryClass.equals(Project.class)) {
Workspace workspace = RallyRequestContext.getCurrent().getWorkspace();
Collection<Project> accessibleProjs = filter(workspace.getProjects(), accessibleTo(user));
Collection<Long> oids = transform(accessibleProjs, toOids());
criteria.add(new InstancesForOidsCriterion(oids));
} else if (queryClass.equals(Scope.class)) {
List<Long> oids = newArrayList();
Workspace workspace = RallyRequestContext.getCurrent().getWorkspace();
oids.add(workspace.getOID());
Collection<Project> accessibleProjs = filter(workspace.getProjects(), accessibleTo(user));
oids.addAll(transform(accessibleProjs, toOids()));
criteria.add(new InstancesForOidsCriterion(oids));
} else if(queryClass.equals(AttributeConfiguration.class)) {
List<Long> oids = newArrayList();
Workspace workspace = RallyRequestContext.getCurrent().getWorkspace();
oids.add(workspace.getOID());
Collection<Project> accessibleProjs = filter(workspace.getProjects(), accessibleTo(user));
oids.addAll(transform(accessibleProjs, toOids()));
criteria.add(new AttributeConfigurationsInScopesCriterion(oids));
}

Now that we’ve eliminated the subtle duplication, we’re left with blatant duplication. Let’s fix that up some by moving all of the repeated code out of the individual if-blocks:

final Class queryClass = queryType.getTypeClass();

Workspace workspace = RallyRequestContext.getCurrent().getWorkspace();
List<Long> oids = newArrayList();

Collection<Project> accessibleProjs = filter(workspace.getProjects(), accessibleTo(user));
oids.addAll(transform(accessibleProjs, toOids()));

// TODO Yuck, this could use some refactoring
if (queryClass.equals(Project.class)) {
criteria.add(new InstancesForOidsCriterion(oids));
} else if (queryClass.equals(Scope.class)) {
oids.add(workspace.getOID());
criteria.add(new InstancesForOidsCriterion(oids));
} else if (queryClass.equals(AttributeConfiguration.class)) {
oids.add(workspace.getOID());
criteria.add(new AttributeConfigurationsInScopesCriterion(oids));
}

This is getting better. We can now see what’s different about each case. Project and Scope queries both use the same type of criterion. Only Scope includes the workspace. Like Scope, AttributeConfiguration includes the workspace, but it uses a different criterion.

Could you quickly see any of that in the original code sample?

One problem is that we might actually be doing some extra work now, because code that was previously hidden behind conditionals is now executed every time. We can fix that by extracting this code into a method and adding a guard clause. This code could use some method extraction anyway.

###### Send Us Your Feedback

Provide us with some information about yourself and we'll be in touch soon. * Required Field