Tag Archives: instanceof

instanceof is bad design

I often see code like this:

public void displayVehicles(List<Vehicle> vehicles){
  for (Vehicle vehicle : vehicles) {
    setManufacturer(vehicle.getManufacturerName());
    
    if (vehicle instanceof Car) {
      setIcon("CarIcon");
      Car car = (Car) vehicle;
      String displayText = car.getCarName()+ " "+car.getModelName();
      setDisplayText(displayText);
    }
    if (vehicle instanceof Truck){
      setIcon("TruckIcon");
      Truck truck = (Truck) vehicle;
      String displayText = truck.getTruckSerie();
      setDisplayText(displayText);
    }
    
  }
}

Of course, you might immediately see that this is not what inheritance should look like and how you could solve this. The worst thing is that when you would add a third implementation “Bike” you will surely forget about this view-code.

There are different ways of improving this:

  1. Put a getDisplayText() and getIcon() method in the Vehicle interface
  2. Create an own object which contains all information necessary for this view. (or Composition)
  3. In some (rare) cases a visitor pattern might help as it allows dynamic dispatch. (Remark: There are languages which support multiple/double dispatch by default, like Lisp or Xtend)

As number 1 and 2 are quite easy solution it might not always make sense to change your domain model just for getting data for a view. So I have prepared a solution with the visitor pattern. In general, I’m actually not a big fan of it, but it is better than instanceof.

The Visitor Pattern

also read Wikipedia 🙂

The Visitor interface which needs to be implemented with one method for each concrete type.

public interface VehicleVisitor {

  void visit(Car car);

  void visit(Truck car);
}

I added an accept method to the Vehicle interface and really simple implementations to both Car and Truck which look identically. It is important, that it is added to every subclass and not to a abstract super class, as the reference this refers to the concrete type now.

 public void accept(VehicleVisitor vehicleVisitor){
   vehicleVisitor.visit(this);
 }

Now comes the ugly part. As the visit-Methods do not have a return type, I need to remember its state in the Visitor itself. I’m using simple fields here.

public class DisplayNameModelVisitor implements VehicleVisitor {

  private String displayText;
  private String icon;

  public void visit(Car car) {
    displayText = car.getCarName() + " " + car.getModelName();
    icon="CarIcon";
  }

  public void visit(Truck truck) {
    displayText = truck.getTruckSerie();
    icon="TruckIcon";
  }
  
  public String getDisplayName(){
    return displayText;
  }
  
  public String getIcon() {
    return icon;
  }
}

The implementation of my VehicleDisplay class got a lot easier now:

public void displayVehiclesWithVisitor(List<Vehicle> vehicles){
  DisplayNameModelVisitor modelVisitor = new DisplayNameModelVisitor();
  for (Vehicle vehicle : vehicles) {
    setManufacturerName(vehicle.getManufacturerName());
    vehicle.accept(modelVisitor);
    setDisplayText(modelVisitor.getDisplayName());
    setIcon(modelVisitor.getIcon());
  }
}

So for me this is a valid way to remove the uses of instanceof. A major advantage is that when you add a bike class and you insert the accept method on the class you will realize that you have to extend the Visitor interface and all Visitor implementations to be able to handle bikes now correctly.

I have the complete maven eclipse project here: InstanceOf-Visitor-Example