Instanceof Code Smell

I have noticed several times over the last few years that Java developers misunderstand when to use instanceof. In particular I have noticed an instance of java.lang.Object interrogated using instanceof through a sequence of if statements. Each time I have seen this I have explained why this is a bad idea and have tried to find a reference that summarizes this code smell, but to no avail, so I have chosen to write something myself.

First of all, there are legitimate uses of instanceof. A good example is overriding the equals method in a class you are designing yourself. However too often instanceof is used as a solution when the underlying problem is poor class structure. To illustrate this, I have created a toy example. Suppose we have two entity classes: Book and Car. The Book class is shown below.

package example1;



public class Book {

 private String title;

 private String author;

 private String isbn;



 public Book(String title, String author, String isbn) {

   this.title = title;

   this.author = author;

   this.isbn = isbn;

 }



 public String getAuthor() {

   return author;

 }



 public String getIsbn() {

   return isbn;

 }



 public String getTitle() {

   return title;

 }

}

There is nothing special about this class. Similarly with the Car class:

package example1;



import java.awt.Color;





public class Car {

 private String manufacturer;

 private String model;

 private Color colour;



 public Car(String manufacturer, String model, Color colour) {

   this.manufacturer = manufacturer;

   this.model = model;

   this.colour = colour;

 }



 public Color getColour() {

   return colour;

 }



 public String getManufacturer() {

   return manufacturer;

 }



 public String getModel() {

   return model;

 }

}

The important thing to notice with these two classes is that from a business point of view they are unrelated, so there is no natural class hierarchy for them. Their only common superclass is java.lang.Object.

Suppose now that we want to be able to generate an XML representation for a list that can contain either Book or Car instances. Using instanceof we could do this as follows:

package example1;



import java.util.Iterator;

import java.util.List;



public class XMLGenerator {

 public String generateXML(List items){

   StringBuffer result = new StringBuffer();

   Iterator iterator = items.iterator();

   while (iterator.hasNext()){

     result.append("<item>");

     Object obj = iterator.next();

     if (obj instanceof Book){

       result.append(generateXML((Book) obj));

     } else if (obj instanceof Car){

       result.append(generateXML((Car) obj));

     } else {

       throw new IllegalArgumentException("Unexpected object: " + obj);

     }

     result.append("</item>");

   }

   return result.toString();

 }



 private String generateXML(Car car) {

   StringBuffer result = new StringBuffer();

   result.append("<car>");

   result.append("<manufacturer>");

   result.append(car.getManufacturer());

   result.append("</manufacturer>");

   result.append("<model>");

   result.append(car.getModel());

   result.append("</model>");

   result.append("<colour>");

   result.append(car.getColour());

   result.append("</colour>");

   result.append("</car>");

   return result.toString();

 }



 private String generateXML(Book book) {

   StringBuffer result = new StringBuffer();

   result.append("<book>");

   result.append("<title>");

   result.append(book.getTitle());

   result.append("</title>");

   result.append("<author>");

   result.append(book.getAuthor());

   result.append("</author>");

   result.append("<isbn>");

   result.append(book.getIsbn());

   result.append("</isbn>");

   result.append("</book>");

   return result.toString();

 }

}

The part of this class that I really object to is

    Object obj = iterator.next();

   if (obj instanceof Book){

     result.append(generateXML((Book) obj));

   } else if (obj instanceof Car){

     result.append(generateXML((Car) obj));

   } else {

     throw new IllegalArgumentException("Unexpected object: " + obj);

   }

The point is that we know from our design that all of the objects in this list should be capable of having XML generated. However by using instances of java.lang.Object we are throwing away this information and then recovering it using instanceof. That is, we are using features from the language to help us overcome our poor design.As an aside, I can also point out that using method overloading in the way this class does, is another symptom of this code smell.

How can we resolve this? The issue is to in some way pass responsibility for generating the XML onto the objects themselves. A first step could be to define an IGenerateXML interface

package example2;



public interface IGenerateXML {

 public String generateXML();

}

Book and Car will then implement this interface:

package example2;



public class Book implements IGenerateXML {

 private String title;

 private String author;

 private String isbn;



 public Book(String title, String author, String isbn) {

   this.title = title;

   this.author = author;

   this.isbn = isbn;

 }



 public String getAuthor() {

   return author;

 }



 public String getIsbn() {

   return isbn;

 }



 public String getTitle() {

   return title;

 }



 public String generateXML() {

   StringBuffer result = new StringBuffer();

   result.append("<book>");

   result.append("<title>");

   result.append(getTitle());

   result.append("</title>");

   result.append("<author>");

   result.append(getAuthor());

   result.append("</author>");

   result.append("<isbn>");

   result.append(getIsbn());

   result.append("</isbn>");

   result.append("</book>");

   return result.toString();

 }

}



package example2;



import java.awt.Color;





public class Car implements IGenerateXML {

 private String manufacturer;

 private String model;

 private Color colour;



 public Car(String manufacturer, String model, Color colour) {

   this.manufacturer = manufacturer;

   this.model = model;

   this.colour = colour;

 }



 public Color getColour() {

   return colour;

 }



 public String getManufacturer() {

   return manufacturer;

 }



 public String getModel() {

   return model;

 }



 public String generateXML() {

   StringBuffer result = new StringBuffer();

   result.append("<car>");

   result.append("<manufacturer>");

   result.append(getManufacturer());

   result.append("</manufacturer>");

   result.append("<model>");

   result.append(getModel());

   result.append("</model>");

   result.append("<colour>");

   result.append(getColour());

   result.append("</colour>");

   result.append("</car>");

   return result.toString();

 }

}

The XMLGenerator class is now greatly simplified:

package example2;



import java.util.Iterator;

import java.util.List;





public class XMLGenerator {

 public String generateXML(List items){

   StringBuffer result = new StringBuffer();

   Iterator iterator = items.iterator();

   while (iterator.hasNext()){

     result.append("<item>");

     IGenerateXML obj = (IGenerateXML) iterator.next();

     result.append(obj.generateXML());

     result.append("</item>");

   }

   return result.toString();

 }

}

That's it! No messing around with java.lang.Object, instanceof or erroneously overloaded methods.