The Instanceof Code Smell and One Way to Get Around It

Using instanceof is a code smell. I think we may agree on that. Whenever I see a construction like that, I’m sure that something went awry. Maybe someone just didn’t notice a problem when making a change? Maybe there was an idea, but it was so complex that it required so much effort or time that a developer made a decision not to do it? Maybe it was just laziness? Who knows. The fact remains that the code evolved into such state and we have to work with it. 

Or maybe there is something that we can do about it? Something that will open our code for extensions?

Today I want to show you how you can achieve that. 

But first, let me explain why this instanceof is a problem at all.

Take a Look at the Code

Today we will talk a bit about this code:

public class ChangeProcessingHandler {
   public CodeDelta triggerProcessingChangeOf(Code code, Change change) {
       verifyChangeOf(code, change);

       if (change instanceof Refactoring) {
           return processRefactoring(code, (Refactoring) change);
       } else if (change instanceof Improvement)  {
           return processImprovement(code, (Improvement) change);
       } else if (change instanceof Growth) {
           return processGrowth(code, (Growth) change);
       } else {
           throw new UnsuportedChangeException();
       }
   }

   // some more code
}

And we will try to improve it.

I tried to make this code descriptive, but let me just briefly summarize it. 

Depending on a specific type of the Change interface’s implementation, we choose an accurate way of processing. In case we don't find the type that matches, we just throw an exception. 

Now, let’s take a look at what are the problems with this code.

Interface and its Implementations?

When you look at the method’s declaration, what can you say about it? That it needs two input parameters, that’s for sure. What kind of information does it give us? We know dependencies and based on their API we know how in the body of the method we may interact with those passed objects.

Is it true in the given example? Unfortunately not. We are passing an instance of Change and we expect that the body of the method will depend on its interface. But inside we are casting our instance into a specific type, which results in an increased number of dependencies. 

This itself is not a good design decision, but what is even worse - we increase this number behind the scene. Until you won’t read the body of the method you won’t know that. 

This lack of knowledge is far worse than the number of dependencies.

New Type Is not so Easy to Add

Let’s imagine that you have to add a new implementation of the Change interface. What will happen? Well, nothing. You will add the class definition and tests for it. You will run all tests. You will be lucky if there is at least one component or system test that will reach the presented code with the newly introduced implementation of the Change interface and will fail.

The problem starts when there is no such test and you won’t even know there’s a place you should change in order to meet new functionality. 

Everything will compile and you will just work until…

Exception? Why?

You notice this nice UnsupportedChangeException in the code? To be honest, it’s there only because of a wrong design. 

There are two reasons why we have it:

• Code would not compile without it. Of course, we could skip it if the method would be void, but in our example we have to return or throw something. We could replace last if-else with just else, but this is not something we like to do. 
  
• It prevents us from adding a new type and forgetting to add support of newly introduced functionality in there. Assuming there is at least one test that will fail in such situation.
  

Why have I called it bad design? Well, using an exception to signalize the need of support of new functionality is rather misusing of exceptions. I also believe that would be far better if our code would signalize such thing by not compiling. It would make sense for me and definitely would give faster feedback.

Visitor for the Rescue!

Visitor allows us to add an additional functionality whose implementation depends on the specific type of the object. It allows for that with the usage of an interface’s method. Thanks to that we can avoid retrieving information about specific interface’s implementation on our own.

First off, we need to make it possible to retrieve information about the type of an object. To do so, we have to add to our interface one method which will allow us to pass a visitor:

public interface Change {
   void accept(Visitator visitator);
}

It is implementation in each object that implements an interface is pretty straightforward:

public class Refactoring implements Change {
   @Override
   public void accept(Visitator visitator) {
       visitator.visit(this);
   }
   // some code
}

What might we observe by looking at the line where we have got invocation of a method visit()? This is the place where information about type is retrieved. There’s no need for instanceof, no need for casting. This is what we get for free with the support of better design.

At this moment, you probably know how interface of Visitor looks like:

public interface Visitator {
   void visit(Refactoring refactoring);
   void visit(Improvement improvement);
   void visit(Growth growth);
}

Not so complicated, is it?

After this we have to extract some code from ChangeProcessingHandler class to the class that implements our Visitor interface:

public class ChangeProcessor implements Visitator {
   private final Code code;


   public ChangeProcessor(Code code) {
       this.code = code;
   }


   @Override
   public void visit(Refactoring refactoring) {
       // some code
   }


   @Override
   public void visit(Improvement improvement) {
       // some code
   }


   @Override
   public void visit(Growth growth) {
       // some code
   }
}

And of course we have to use this in the right place:

public class ChangeProcessingHandlerRefactored {
   public void triggerProcessingChangeOf(Code code, Change change) {
       verifyChangeOf(code, change);
       change.accept(new ChangeProcessor(code));
   }
}

Is it Better?

OK, so we changed our original code. Now let me explain what we’ve gained.

• We’ve just gotten rid of an exception. It is no longer needed because the required support for newly introduced implementation would be signaled by non-compiling code. 
  
• Fast feedback is a result of using interfaces that will tell us what more we have to implement to have everything fully supported.
  
• Single Responsibility Principle comes into play because each specific implementation of Visitor interface is responsible only for one functionality.
  
• Design is behavior-oriented (interfaces), not implementation-oriented (instanceof + casting). In this way, we are hiding implementation details.
  
• Design is open for extensions. It is really easy to introduce new functionality which implementation differ for specific objects.

It is not so Perfect

Each design is a tradeoff. You get something, but it comes at a cost.

I listed benefits in the previous paragraph, so what about the cost?

• So many objects
  One may say that is an obvious result of using any design pattern, and I would say yes. However, it does not change the fact that with an increased number of objects, it is harder to navigate through them. Having everything in one object may be a problem, but poorly named or disorganized classes might result in a mess.
  
• Complexity
  All of those objects need a name, and it’s great if these objects are domain related. In such cases, we end up with a better understanding of our application. But it is not always the case. Also, we have to be very careful with naming the newly introduced classes. All of them have to be named in a self-explanatory manner. Which is not as easy as some may think.
  
• Where’s my (bounded) context?
  Visitor may help with problems similar to the one presented in the example. But if there are a lot of places like that, you have to realize that each visitor is in some way putting a behavior of the object into another object. What about the Law of Demeter? What about Tell, don’t ask? Before you will visitor to solve your instanceof problem, you should ask yourself if this functionality isn't a part of the object itself. Some developers explain to me that that’s a way of having small objects. Well, for me, such an explanation is a proof that we should think about bounded contexts instead. Objects still would be small and their behavior would not leak to the outer class.

That’s All, Folks

That’s all for today. I hope that you found this idea of redesign useful and that after you’ve read this article, the smells in your code will definitely feel endangered. 

As always, I encourage you to write comments and share your perspective and experiences. Maybe you know more about benefits/problems related to such change.