Avoiding instanceof in Java

Question

Having a chain of "instanceof" operations is considered a "code smell". The standard answer is "use polymorphism". How would I do it in this case?

There are a number of subclasses of a base class; none of them are under my control. An analogous situation would be with the Java classes Integer, Double, BigDecimal etc.

if (obj instanceof Integer) {NumberStuff.handle((Integer)obj);}
else if (obj instanceof BigDecimal) {BigDecimalStuff.handle((BigDecimal)obj);}
else if (obj instanceof Double) {DoubleStuff.handle((Double)obj);}

I do have control over NumberStuff and so on.

I don't want to use many lines of code where a few lines would do. (Sometimes I make a HashMap mapping Integer.class to an instance of IntegerStuff, BigDecimal.class to an instance of BigDecimalStuff etc. But today I want something simpler.)

I'd like something as simple as this:

public static handle(Integer num) { ... }
public static handle(BigDecimal num) { ... }

But Java just doesn't work that way.

I'd like to use static methods when formatting. The things I'm formatting are composite, where a Thing1 can contain an array Thing2s and a Thing2 can contain an array of Thing1s. I had a problem when I implemented my formatters like this:

class Thing1Formatter {
  private static Thing2Formatter thing2Formatter = new Thing2Formatter();
  public format(Thing thing) {
      thing2Formatter.format(thing.innerThing2);
  }
}
class Thing2Formatter {
  private static Thing1Formatter thing1Formatter = new Thing1Formatter();
  public format(Thing2 thing) {
      thing1Formatter.format(thing.innerThing1);
  }
}

Yes, I know the HashMap and a bit more code can fix that too. But the "instanceof" seems so readable and maintainable by comparison. Is there anything simple but not smelly?

Note added 5/10/2010:

It turns out that new subclasses will probably be added in the future, and my existing code will have to handle them gracefully. The HashMap on Class won't work in that case because the Class won't be found. A chain of if statements, starting with the most specific and ending with the most general, is probably the best after all:

if (obj instanceof SubClass1) {
    // Handle all the methods and properties of SubClass1
} else if (obj instance of SubClass2) {
    // Handle all the methods and properties of SubClass2
} else if (obj instance of Interface3) {
    // Unknown class but it implements Interface3
    // so handle those methods and properties
} else if (obj instance of Interface4) {
    // likewise.  May want to also handle case of
    // object that implements both interfaces.
} else {
    // New (unknown) subclass; do what I can with the base class
}

Answer 1

You might be interested in this entry from Steve Yegge's Amazon blog: "when polymorphism fails". Essentially he's addressing cases like this, when polymorphism causes more trouble than it solves.

The issue is that to use polymorphism you have to make the logic of "handle" part of each 'switching' class - i.e. Integer etc. in this case. Clearly this is not practical. Sometimes it isn't even logically the right place to put the code. He recommends the 'instanceof' approach as being the lesser of several evils.

As with all cases where you are forced to write smelly code, keep it buttoned up in one method (or at most one class) so that the smell doesn't leak out.

Answer 2

As highlighted in the comments, the visitor patter is would be a good choice, but without direct control over the target/acceptor/visitee you can't implement the pattern. Here's one way the visitor pattern could possibly still be used here even though you have no direct control over the subclasses by using wrappers. Taking Integer as an example:

public class IntegerWrapper {
    private Integer integer;
    public IntegerWrapper(Integer anInteger){
        integer = anInteger;
    }
    //Access the integer directly such as
    public Integer getInteger() { return integer; }
    //or method passthrough...
    public int intValue() { return integer.intValue(); }
    //then implement your visitor:
    public void accept(NumericVisitor visitor) {
        visitor.visit(this);
    }
}

Of course, wrapping a final class might be considered a smell of its own but maybe its a good fit with your subclasses. Personally, I don't think instanceof is that bad a smell here, especially if confined to one method and I would happily use it (probably over my own suggestion above). As you say, its quite readable, typesafe and maintainable. As always, keep it simple.

Answer 3

Instead of a huge if, you can put the instances you handle in a map (key: class, value: handler).

If the lookup by key returns null, call a special handler method which tries to find a matching handler (for example by calling isInstance() on every key in the map).

When a handler is found, register it under the new key.

This makes the general case fast and simple and allows you to handle inheritance.

Answer 4

You could consider the Chain of Responsibility pattern. For your first example, something like:

public abstract class StuffHandler {
   private StuffHandler next;

   public final boolean handle(Object o) {
      boolean handled = doHandle(o);
      if (handled) { return true; }
      else if (next == null) { return false; }
      else { return next.handle(o); }
   }

   public void setNext(StuffHandler next) { this.next = next; }

   protected abstract boolean doHandle(Object o);
}

public class IntegerHandler extends StuffHandler {
   @Override
   protected boolean doHandle(Object o) {
      if (!o instanceof Integer) {
         return false;
      }
      NumberHandler.handle((Integer) o);
      return true;
   }
}

and then similarly for your other handlers. Then it's a case of stringing together the StuffHandlers in order (most specific to least specific, with a final 'fallback' handler), and your despatcher code is just firstHandler.handle(o);.

(An alternative is to, rather than using a chain, just have a List<StuffHandler> in your dispatcher class, and have it loop through the list until handle() returns true).