My company's legacy code is suffering from prevalent usage of instanceof switch-casing, in the form of:
if(object instanceof TypeA) {
TypeA typeA = (TypeA) object;
...
...
}
else if(object instanceof TypeB) {
TypeB typeB = (TypeB) object;
...
...
}
...
...
To make things worse, several of the TypeX classes in questions are actually wrappers of classes found in 3rd party libraries.
The suggested approach of using the visitor design pattern and dedicated visitor design pattern wrappers on 3rd party classes as seen here (instanceof -> Visitor DP) and here (Visitor DP with 3rd party classes) seems like a good approach.
However, during a code review session where this approach was suggested, the question of the additonal overhead of boilerplate code required by each refactoring of the instaceof switch-casing has lead to this mechanism being declined.
I would like to fix this ongoing issue and I am considering a generic approach to the problem:
A utility class which will wrap the visitor design pattern with generic referencing to the visited objects. The idea is to implement the generic core of the visitor utility class once and only once, and provide specific implementations to the TypeX object behaviour where needed - hopefully even reusing some implementations via OO extention of the functionality implementing classes.
My question is - has anyone here done something similiar? If not - can you point out any pros/cons that might be relevant?
EDIT : Too much boilerplate code = implementing the visitor design pattern specifically for each instance of an instanceof switch-case. This is obviously redundent and will cause a lot of code duplication, if the visitor DP is not implemented using generics.
As for the generic visitor DP utility I had in mind :
First of all, usage of reflection with the visitor DP as seen here.
Second, the following usage of generics (based on the reflective visitor):
public interface ReflectiveVisitor<GenericReturn,GenericMetaData>
{
public GenericReturn visit(Object o, GenericMetaData meta);
}
public interface ReflectiveVisitable<A,B>
{
public GenericReturn accept(Visitor visitor, GenericMetaData meta);
}
GenericReturn and GenericMetaData are interfaces aimed at providing any additionally required meta data for specific logics to be implemented, and to provide versatility for return types returned by the visitor DP.
Thanks in advance!
EDIT : Boiler plate coding when refactoring from instanceof to the visitor :
A common use case I'd have to handle is instanceof switchcasing in order to perform single API calls of concrete implementations :
public class BoilerPlateExample
...
if(object instanceof TypeA) {
((TypeA) object).specificMethodTypeA(...)......;
}
else if(object instanceof TypeB) {
((TypeB) object).completeyDifferentTypeBMethod(...)......;
}
...
...
As for the visitor design handling this?
public interface Visitor
{
// notice how I just binded my interface to a specific set of methods?
// this interface will have to be generic in order to avoid an influx of
// of dedicated interfaces
public void visit(TypeA typeA);
public void visit(TypeB typeB);
}
public interface Visitable
{
public void accept(Visitor visitor);
}
public class BoilerPlateExampleVisitable<T> implements Visitable
{
// This is basically a wrapper on the Types
private T typeX;
public BoilerPlateExampleVisitable (T typeX) {
this.typeX = typeX;
}
public void accept(Visitor visitor) {
visitor.visit(typeX);
}
}
public class BoilerPlateExampleVisitor implements Visitor
{
public void visit(TypeA typeA) {
typeA.specificMethodTypeA(...)......;
}
public void visit(TypeB typeB) {
typeB.completeyDifferentTypeBMethod(...)......;
}
}
public static final BoilerPlateExampleVisitor BOILER_PLATE_EXAMPLE_VISITOR = new BoilerPlateExampleVisitor();
public static void main(....) {
TypeA object = .....; // created by factory
BoilerPlateExampleVisitable boilerPlateVisitable = VisitableFactory.create(object); // created by dedicated factory, warning due to implicit generics
boilerPlateVisitable.accept(BOILER_PLATE_EXAMPLE_VISITOR);
}
TL;DR: Let's say that you have N classes with M operations each. You need the visitor pattern only if M may grow and N is already big. Else use polymorphism.
Maybe I will push an open door, because you already thought of this, but here are a few thoughts.
In the generic case, you will use the visitor pattern only if you want to add new operations without refactoring all classes. That is when M may grow and N is already big.
For every new operation, you create a new visitor. This visitor accepts the N classes and handles the operation for each of them:
public class NewOperationVisitor implements Visitor
{
public void visit(TypeA typeA) {
// apply my new operation to typeA
}
public void visit(TypeB typeB) {
// apply my new operation to typeB
}
...
}
Thus, you don't have to add the new operation to all the N classes, but you have to refactor every visitor if you add a class.
Now, if M is stable, please avoid the visitor pattern: use polymorphism. Every class has a well defined set of methods (roughly one per operation). If you add a class, just define the known operations for that class:
public class TypeX implements Operator
{
public void operation1() {
// pretty simple
}
public void operation2() {
// pretty simple
}
}
Now, you have to refactor every class if you add an operation, but adding a class is very easy.
This tradeoff is explained in Clean Code by R. C. Martin (6. Objects and Data Structures, Data/Object Anti-Symmetry):
Procedural code [here: the visitor] makes it hard to add new data structures because all the functions must change. OO code makes it hard to add new functions because all the classes must change.
As stated in a @radiodef comment, avoid reflection and other tricks. This will be worse than the issue.
Clearly separate where you really need the visitor pattern, and where you don't. Count classes and operations. I bet that, in most of the cases, you don't need the visitor pattern. (Your managers are probably right!). If you need the visitor pattern in 10 % of the cases, maybe the "additonal overhead of boilerplate code" will be acceptable.
Since several of your TypeX classes are already wrappers, you maybe need to wrap better. Sometimes, one wraps from bottom to top: "My 3rd party class has those methods: I will wrap the methods I need and forget the others. And I will keep the same names to keep it simple." Instead, you have to carefully define the service a TypeX class should provide. (A hint: look in your if ... instanceof ... bodies). A then wrap again the 3rd party libs to provide thoses services.
Really: avoid avoid reflection and other tricks.
You asked in a comment for a pseudo-code, but I can't give it to you since I have a method in mind, not a procedure or an algorithm.
Here's a minimal step by of step of what I would do in such a situation.
instanceof switch" in a methodThat's almost a medical advice! Before:
public void someMethod() {
...
...
if(object instanceof TypeA) {
TypeA typeA = (TypeA) object;
...
...
}
else if(object instanceof TypeB) {
TypeB typeB = (TypeB) object;
...
...
}
...
...
}
After:
public void someMethod() {
...
...
this.whatYouDoInTheSwitch(object, some args);
...
...
}
And:
private void whatYouDoInTheSwitch(Object object, some args) {
if(object instanceof TypeA) {
TypeA typeA = (TypeA) object;
...
...
}
else if(object instanceof TypeB) {
TypeB typeB = (TypeB) object;
...
...
}
}
Any decent IDE will do it for free.
Leave the code as this, but document it:
/** Needs fix: use Visitor Pattern, because... (growing set of operations, ...) */
private void whatYouDoInTheSwitch(Object object, some args) {
...
}
The goal is to switch from:
this.whatYouDoInTheSwitch(object, other args);
To:
object.whatYouDoInTheSwitch(this, other args);
You have a little refactoring to do:
A. Create a method for every case in the big switch. All those methods should have the same signature, except for the type of the object:
private void whatYouDoInTheSwitch(Object object, some args) {
if(object instanceof TypeA) {
this.doIt((TypeA) object, some args);
}
else if(object instanceof TypeB) {
this.doIt((TypeB) object, some args);
}
}
Again, any IDE will do it for free.
B. Create an interface with the following method:
doIt(Caller caller, args);
Where Caller is the type of the class you are refactoring (the one that contains the big switch).
C. Make every TypeX implement this interface by converting every doIt(TypeX objX, some args) into a doIt(Caller, some args) method from TypeX. Basically, that is a simple find-replace: replace this by caller and objX by this. But this may be a little more time consuming than the rest.
D. Now, you have:
private void whatYouDoInTheSwitch(Object object, some args) {
if(object instanceof TypeA) {
((TypeA) object).doIt(this, some args);
}
else if(object instanceof TypeB) {
((TypeB) object).doIt(this, some args);
}
}
This is strictly equivalent to:
private void whatYouDoInTheSwitch(Object object, some args) {
if(object instanceof TypeA) {
object.doIt(this, some args);
}
else if(object instanceof TypeB) {
object.doIt(this, some args);
}
}
Because at runtime, the JVM will find the right method for the right class (that's polymorphism!). Thus, this is also equivalent to (if object has one of the enumerated types):
private void whatYouDoInTheSwitch(Object object, some args) {
object.doIt(this, some args);
}
E. Inline the method and you have in your Caller class:
public void someMethod() {
...
...
object.doIt(this, some args);
...
...
}
Actually, this is only a sketch, and many special cases could occur. But it is guaranteed to be relatively fast and clean. It may be done for a selected method only of for all methods.
Be sure to test, if possible, the code after every step. And be sure to choose the right names for the methods.