How to get rid of instanceof?

user_12894128 0
Viewed 155

I have been told that using instanceof like in the code below is bad practice as it becomes repetitive and makes extension hard. I'm pretty new to Java though and don't see straight away what my alternatives are, what would you recommend I do to get rid of instance of and abstract the code?

void moveit(Vehicle car) {
        if(car instanceof Volvo240){
            volvoPoint.x = (int) car.getXCoordinate();
            volvoPoint.y = (int) car.getYCoordinate();
        }
        if(car instanceof Scania) {
            scaniaPoint.x = (int) car.getXCoordinate();
            scaniaPoint.y = (int) car.getYCoordinate() + 100;
        }
        if(car instanceof Saab95) {
            saabPoint.x = (int) car.getXCoordinate();
            saabPoint.y = (int) car.getYCoordinate() + 200;
        }
        repaint();
    }
instanceof oop java
3 Answers

InstanceOf's will make it hard to introduce a new type of car. You will have to find all the places you did these instanceof checks and modify with the new car. See the "Open for extension and closed for modification" principle.

You should have an interface

interface Vehicle {
  Integer getXCoordinate();
  Integer getYCoordinate();
  void moveIt(Point point);
}

And three implementations, Saab95, Volvo240 and Scania

class Saab95 implements Vehicle {
   moveIt(Point point) {
      point.x = getXCoordinate();
      point.y= getYCoordinate() + 200
   }
}

And so on for the other cars

user_4071200

Without knowing the complete code, and assuming that you only have those three subclasses. The least intrusive approach is to take advantage of method overloading:

void moveit(Volvo240 car){
     volvoPoint.x = (int) car.getXCoordinate();
     volvoPoint.y = (int) car.getYCoordinate();
     repaint();
}

void moveit(Scania car){
     volvoPoint.x = (int) car.getXCoordinate();
     volvoPoint.y = (int) car.getYCoordinate() + 100;
     repaint();
}

void moveit(Saab95 car){
     saabPoint.x = (int) car.getXCoordinate();
     saabPoint.y = (int) car.getYCoordinate() + 200;
     repaint();
}

void moveit(Vehicle car){
     repaint();
}

It seems to me that the variables volvoPoint.x and volvoPoint.y should belong to the class Volvo240 (and the same applies to the other variables). But you kept those variables (that should belong to the classes Volvo240, Scania, and Saab95) in a single place, so that (I would assume) you can repaint based on those variables' values.

You should consider an alternative approach in which you teach each Vehicle how to repaint themselves. Hence, moving the repaint logic and those variables to each of the subclasses, accordingly:

public class Volvo240 extends Vehicle{

       public repaint(){
              volvoPoint.x = (int) car.getXCoordinate();
              volvoPoint.y = (int) car.getYCoordinate();
              // do the repaint logic
       } 
}
user_13668710

I'm going to say right up front, I have no problem with your original solution. More on that at the bottom of this post. If you really want your current code to work without using instanceof, here's one way.

You could make a new class to store the information associated with each type of vehicle, like so:

public class VehicleInfo {
    private final Point point;
    private final int offset;

    public VehicleInfo(Point point, int offset) {
        this.point = point;
        this.offset = offset;
    }

    public Point getPoint() {
        return point;
    }

    public int getOffset() {
        return offset;
    }
}

You could then use that class in your method like so:

private static final HashMap<Type, Point> POINT_MAP = new HashMap<Type, Point>() {
    {
        put(Volvo240.class, new VehicleInfo(volvoPoint, 0));
        put(Scania.class, new VehicleInfo(scaniaPoint, 100));
        put(Saab95.class, new VehicleInfo(saabPoint, 200));
    }
};

void moveit(Vehicle car) {
    VehicleInfo info = POINT_MAP.get(car.getClass());
    info.getPoint().x = car.getXCoordinate();
    info.getPoint().y = car.getYCoordinate() + info.getOffset();
    repaint();
}

That said, I really don't have a problem with your original solution using instanceof. The program is going to have to check the type of the argument at some point, so might as well explicitly show that in your code. Unless this part of the program is going to be repeatedly modified and this is part of some huge project where everyone needs to use standard coding conventions, I would just stick with what you've got, despite what everyone else here is saying. I was recently introduced to the concept of YAGNI, and I think that definitely applies here. If it works, it works.

user_136237420
Related