Refactoring legacy instanceof switch casing via design patterns

Question

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

Answer 1

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.

Visitor pattern

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.

Polymorphism

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.

What you should do

1. As stated in a @radiodef comment, avoid reflection and other tricks. This will be worse than the issue.

2. 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.

3. 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.

4. Really: avoid avoid reflection and other tricks.

What would I do?

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.

Isolate every "big instanceof switch" in a method

That'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.

If you are in a case that would need the Visitor pattern

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) {
    ...
}

If you want to use polymorphism

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.

Answer 2

Seems like polymorphism. Such code could stem from a heterogene set of business object classes, like Excel ReportX, Zip, TableY, and actions like Open, Close, Save and such.

As it is, this kind of programming causes a huge coupling between classes, and has issues with completeness of all cases, extendibility.

In the case of polymorphism the actual wrapper for some bussiness object should provide Actions (Open, Save, Close).

This mechanism is similar to java swing, where an edit field has its list of actions (Cut, Copy, Paste and such), and a tree view an overlapping set of actions. Depending on the focus the actual actions will be installed in the menu actions.

A declarative specification might be in order: say an XML that "holds" beans and their actions.

Should you have some MVC paradigm on the radar, consider the following: every action could have parameters. Use PMVC (my idea), a Parameters class apart from the Model class, as those infos have a different life cycle, and are constant.

The road hereto could be:

• a prototype with two business objects and two actions.
• refactoring with one polymorph business object containing all (old code).
• slowly moving one class after another to their own business object.
• the first removal from the polymorph business object being used to clean up the new architecture.

I would refrain from using inheritance (a BaseDocument with open/save) as this probably does not fit a more heterogeneous reality, and could cause parallel class hierarchies (XDoc with XContainer and XObject).

How it is actually done is still your work. I also would be eager to learn whether there exists an established paradigm.

---

Asked pseudo-code One would need a bit of analysis with some prototype made - a proof of concept. However there is a discovery of (dynamic) capabilities/features.

public interface Capabilities {
    <T> Optional<T> as(Class<T> type);
}

Add this interface to every case class, and you can do:

void f(Capabilities animal) {
    int distance = 45;
    animal.as(Flying.class).ifPresent(bird -> bird.fly(distance));
}

The infrastructure would be: first the registration of capabilities and discovery can be placed in a separate class.

/**
 * Capabilities registration & discovery map, one can delegate to.
 */
public class CapabilityLookup implements Capabilities {

    private final Map<Class<?>, Object> capabilitiesMap = new HashMap<>();

    public final <T> void register(Class<T> type, T instance) {
        capabilitiesMap.put(type, instance);
    }

    @Override
    public <T> Optional<T> as(Class<T> type) {
        Object instance = capabilitiesMap.get(type);
        return instance == null ? Optional.empty()
                                : Optional.of(type.cast(instance));
    }
}

Then the legacy classes can be augmented:

/** Extended legacy class. */
public class Ape implements Capabilities {

    private final CapabilityLookup lookup = new CapabilityLookup();

    public Ape() {
        lookup.register(String.class, "oook");
    }

    @Override
    public <T> Optional<T> as(Class<T> type) {
        return lookup.as(type); // Delegate to the lookup map.
    }
}

/** Extended legacy class. */
public class Bird implements Capabilities {

    private final CapabilityLookup lookup = new CapabilityLookup();

    public Bird() {
        lookup.register(Flying.class, new Flying() {
            ...
        });
        lookup.register(Singing.class, new Singing() {
            ...
        });
    }

    @Override
    public <T> Optional<T> as(Class<T> type) {
        return lookup.as(type); // Delegate to the lookup map.
    }
}

As you can see with Bird the original code would move into the actual class of the interface, here into Bird, as the instance was created in the constructor. But instead of an anonymous class one could make a BirdAsFlying class, a kind of action class in java swing parlance. An inner class has the advantage to access Bird.this.

The refactoring can be done incrementally. Add Capabilities to all "instanceof" legacy classes. An if-sequence would normally be one interface, but could also be two, or one interface with two methods.