Whenever I see a method where the behavior switches on the type of its parameter, I immediately consider first if that method actually belongs on the method parameter. For example, instead of having a method like:
public void sort(List values) {
if (values instanceof LinkedList) {
// do efficient linked list sort
} else { // ArrayList
// do efficient array list sort
}
}
I would do this:
values.sort();
// ...
class ArrayList {
public void sort() {
// do efficient array list sort
}
}
class LinkedList {
public void sort() {
// do efficient linked list sort
}
}
We move the behavior to the place that knows when to use it. We create a real abstraction where you don't need to know the types or the details of the implementation. For your situation, it might make more sense to move this method from the original class (which I will call O) to type A and override it in type B. If the method is called doIt on some object, move doIt to A and override with the different behavior in B. If there are data bits from where doIt is originally called, or if the method is used in enough places, you can leave the original method and delegate:
class O {
int x;
int y;
public void doIt(A a) {
a.doIt(this.x, this.y);
}
}
We can dive a little deeper, though. Let's look at the suggestion to use a boolean parameter instead and see what we can learn about the way your co-worker is thinking. His proposal is to do:
public void doIt(A a, boolean isTypeB) {
if (isTypeB) {
// do B stuff
} else {
// do A stuff
}
}
This looks an awful lot like the instanceof I used in my first example, except that we are externalizing that check. This means that we would have to call it in one of two ways:
o.doIt(a, a instanceof B);
or:
o.doIt(a, true); //or false
In the first way, the call point has no idea what type of A it has. Therefore, should we be passing booleans all the way down? Is that really a pattern we want all over the code base? What happens if there is a third type we need to account for? If this is how the method is called, we should move it to the type and let the system choose the implementation for us polymorphically.
In the second way, we must already know the type of a at the call point. Usually that means we either are creating the instance there, or taking an instance of that type as a parameter. Creating a method on O that takes a B here would work. The compiler would know which method to choose. When we are driving through changes like this, duplication is better than creating the wrong abstraction, at least until we figure out where we are really going. Of course, I am suggesting that we aren't really done no matter what we've changed to this point.
We need to look more closely at the relationship between A and B. Generally, we are told we should favor composition over inheritance. This isn't true in every case, but it is true in a surprising number of cases once we dig in. B inherits from A, meaning that we believe B is an A. B should be used just like A, except that it works a little differently. But what are those differences? Can we give the differences a more concrete name? Is it not B is an A, but really A has an X that could be A' or B'? What would our code look like if we did that?
If we moved the method onto A as suggested earlier, we could inject an instance of X into A, and delegate that method to the X:
class A {
X x;
A(X x) {
this.x = x;
}
public void doIt(int x, int y) {
x.doIt(x, y);
}
}
We can implement A' and B', and get rid of B. We've improved the code by giving a name to a concept that might have been more implicit, and allowed ourselves to set that behavior at runtime instead of compile time. A has actually become less abstract as well. Instead of an extended inheritance relationship, it is calling methods on a delegated object. That object is abstract, but more focused only on the differences in implementation.
There is one last thing to look at though. Let's roll back to your co-worker's proposal. If at all the call sites we explicitly know the type of A we have, then we should be making calls like:
B b = new B();
o.doIt(b, true);
We assumed earlier when composing that A has an X that is either A' or B'. But maybe even this assumption is not correct. Is this the only place where this difference between A and B matters? If it is, then maybe we can take a slightly different approach. We still have an X that is either A' or B', but it doesn't belong to A. Only O.doIt cares about it, so let's only pass it to O.doIt:
class O {
int x;
int y;
public void doIt(A a, X x) {
x.doIt(a, x, y);
}
}
Now our call site looks like:
A a = new A();
o.doIt(a, new B'());
Once again, B disappears, and the abstraction moves into the more focused X. This time, though, A is even simpler by knowing less. It is even less abstract.
It is important to reduce duplication in a code base, but we must consider why the duplication happens in the first place. Duplication can be a sign of deeper abstractions that are trying to get out.
