How to avoid 'instanceof' when implementing factory design pattern?

Question

I am attempting to implement my first Factory Design Pattern, and I'm not sure how to avoid using instanceof when adding the factory-made objects to lists. This is what I'm trying to do:

for (Blueprint bp : blueprints) {
    Vehicle v = VehicleFactory.buildVehicle(bp);
    allVehicles.add(v);
                
    // Can I accomplish this without using 'instanceof'?
    if (v instanceof Car) {
        cars.add((Car) v);
    } else if (v instanceof Boat) {
        boats.add((Boat) v);
    } else if (v instanceof Plane) {
        planes.add((Plane) v);
    }
}

From what I've read on Stack Overflow, using 'instanceof' is a code smell. Is there a better way to check the type of vehicle that was created by the factory without using 'instanceof'?

I welcome any feedback/suggestions on my implementation as I'm not even sure if I'm going about this the right way.

Full example below:

import java.util.ArrayList;

class VehicleManager {
    
    public static void main(String[] args) {
        
        ArrayList<Blueprint> blueprints = new ArrayList<Blueprint>();
        ArrayList<Vehicle> allVehicles = new ArrayList<Vehicle>();
        ArrayList<Car> cars = new ArrayList<Car>();
        ArrayList<Boat> boats = new ArrayList<Boat>();
        ArrayList<Plane> planes = new ArrayList<Plane>();
        
        /*
        *  In my application I have to access the blueprints through an API
        *  b/c they have already been created and stored in a data file.
        *  I'm creating them here just for example.
        */
        Blueprint bp0 = new Blueprint(0);
        Blueprint bp1 = new Blueprint(1);
        Blueprint bp2 = new Blueprint(2);
        blueprints.add(bp0);
        blueprints.add(bp1);
        blueprints.add(bp2);
        
        for (Blueprint bp : blueprints) {
            Vehicle v = VehicleFactory.buildVehicle(bp);
            allVehicles.add(v);
            
            // Can I accomplish this without using 'instanceof'?
            if (v instanceof Car) {
                cars.add((Car) v);
            } else if (v instanceof Boat) {
                boats.add((Boat) v);
            } else if (v instanceof Plane) {
                planes.add((Plane) v);
            }
        }
        
        System.out.println("All Vehicles:");
        for (Vehicle v : allVehicles) {
            System.out.println("Vehicle: " + v + ", maxSpeed: " + v.maxSpeed);
        }
        
        System.out.println("Cars:");
        for (Car c : cars) {
            System.out.println("Car: " + c + ", numCylinders: " + c.numCylinders);
        }
        
        System.out.println("Boats:");
        for (Boat b : boats) {
            System.out.println("Boat: " + b + ", numRudders: " + b.numRudders);
        }
        
        System.out.println("Planes:");
        for (Plane p : planes) {
            System.out.println("Plane: " + p + ", numPropellers: " + p.numPropellers);
        }
    }
}

class Vehicle {
    
    double maxSpeed;
    
    Vehicle(double maxSpeed) {
        this.maxSpeed = maxSpeed;
    }
}

class Car extends Vehicle {
    
    int numCylinders;
    
    Car(double maxSpeed, int numCylinders) {
        super(maxSpeed);
        this.numCylinders = numCylinders;
    }
}

class Boat extends Vehicle {
    
    int numRudders;
    
    Boat(double maxSpeed, int numRudders) {
        super(maxSpeed);
        this.numRudders = numRudders;
    }
}

class Plane extends Vehicle {
    
    int numPropellers;
    
    Plane(double maxSpeed, int numPropellers) {
        super(maxSpeed);
        this.numPropellers = numPropellers;
    }
}

class VehicleFactory {
    
    public static Vehicle buildVehicle(Blueprint blueprint) {
        
        switch (blueprint.type) {
            
            case 0:
                return new Car(100.0, 4);
                
            case 1:
                return new Boat(65.0, 1);
                
            case 2:
                return new Plane(600.0, 2);
                
            default:
                return new Vehicle(0.0);
        }
    }
}

class Blueprint {
    
    int type; // 0 = car; // 1 = boat; // 2 = plane;
    
    Blueprint(int type) {
        this.type = type;
    }
}

Posthaste

You can start by adding getters and setters for your fields such as maxSpeed and numPropellers. This is known as information hiding. You can read more on this here : en.wikipedia.org/wiki/Information_hiding. Next, you can define an Enum called VehicleType instead of using numbers such as 0 or 1 to represent a vehicle type. This will make the code more readable. – Marmara

Couldn't each AVehicle subclass override toString()? Then you could print them all out without worrying about the type. If there's another reason why the caller needs to know the type, let us know and we can make other suggestions. – Hampstead

The factory pattern is designed to hide the subclasses of AVehicle from the programmer using it (keyword encapsulation). Are you sure the factory pattern is the correct design pattern for you? – Gershom

Answer 1

You could implement the Visitor pattern.

---

Detailed Answer

The idea is to use polymorphism to perform the type-checking. Each subclass overrides the accept(Visitor) method, which should be declared in the superclass. When we have a situation like:

void add(Vehicle vehicle) {
    //what type is vehicle??
}

We can pass an object into a method declared in Vehicle. If vehicle is of type Car, and class Car overrode the method we passed the object into, that object would now be processed within the method declared in the Car class. We use this to our advantage: creating a Visitor object and pass it to an overriden method:

abstract class Vehicle {
    public abstract void accept(AddToListVisitor visitor);
}

class Car extends Vehicle {
    public void accept(AddToListVisitor visitor) {
        //gets handled in this class
    }
}

This Visitor should be prepared to visit type Car. Any type that you want to avoid using instanceof to find the actual type of must be specified in the Visitor.

class AddToListVisitor {
    public void visit(Car car) {
        //now we know the type! do something...
    }

    public void visit(Plane plane) {
        //now we know the type! do something...
    }
}

Here's where the type checking happens!

When the Car receives the visitor, it should pass itself in using the this keyword. Since we are in class Car, the method visit(Car) will be invoked. Inside of our visitor, we can perform the action we want, now that we know the type of the object.

---

So, from the top:

You create a Visitor, which performs the actions you want. A visitor should consist of a visit method for each type of object you want to perform an action on. In this case, we are creating a visitor for vehicles:

interface VehicleVisitor {
    void visit(Car car);
    void visit(Plane plane);
    void visit(Boat boat);
}

The action we want to perform is adding the vehicle to something. We would create an AddTransportVisitor; a visitor that manages adding transportations:

class AddTransportVisitor implements VehicleVisitor {
    public void visit(Car car) {
        //add to car list
    }

    public void visit(Plane plane) {
        //add to plane list
    }

    public void visit(Boat boat) {
        //add to boat list
    }
}

Every vehicle should be able to accept vehicle visitors:

abstract class Vehicle {
    public abstract void accept(VehicleVisitor visitor);
}

When a visitor is passed to a vehicle, the vehicle should invoke it's visit method, passing itself into the arguments:

class Car extends Vehicle {
    public void accept(VehicleVisitor visitor) {
        visitor.visit(this);
    }
}

class Boat extends Vehicle {
    public void accept(VehicleVisitor visitor) {
        visitor.visit(this);
    }
}

class Plane extends Vehicle {
    public void accept(VehicleVisitor visitor) {
        visitor.visit(this);
    }
}

That's where the type-checking happens. The correct visit method is called, which contains the correct code to execute based on the method's parameters.

The last problem is having the VehicleVisitor interact with the lists. This is where your VehicleManager comes in: it encapsulates the lists, allowing you to add vehicles through a VehicleManager#add(Vehicle) method.

When we create the visitor, we can pass the manager to it (possibly through it's constructor), so we can perform the action we want, now that we know the object's type. The VehicleManager should contain the visitor and intercept VehicleManager#add(Vehicle) calls:

class VehicleManager {
    private List<Car> carList = new ArrayList<>();
    private List<Boat> boatList = new ArrayList<>();
    private List<Plane> planeList = new ArrayList<>();

    private AddTransportVisitor addVisitor = new AddTransportVisitor(this);

    public void add(Vehicle vehicle) {
        vehicle.accept(addVisitor);
    }

    public List<Car> getCarList() {
        return carList;
    }

    public List<Boat> getBoatList() {
        return boatList;
    }

    public List<Plane> getPlaneList() {
        return planeList;
    }
}

We can now write implementations for the AddTransportVisitor#visit methods:

class AddTransportVisitor implements VehicleVisitor {
    private VehicleManager manager;

    public AddTransportVisitor(VehicleManager manager) {
        this.manager = manager;
    }

    public void visit(Car car) {
        manager.getCarList().add(car);
    }

    public void visit(Plane plane) {
        manager.getPlaneList().add(plane);
    }

    public void visit(Boat boat) {
       manager.getBoatList().add(boat);
    }
}

I highly suggest removing the getter methods and declaring overloaded add methods for each type of vehicle. This will reduce overhead from "visiting" when it's not needed, for example, manager.add(new Car()):

class VehicleManager {
    private List<Car> carList = new ArrayList<>();
    private List<Boat> boatList = new ArrayList<>();
    private List<Plane> planeList = new ArrayList<>();

    private AddTransportVisitor addVisitor = new AddTransportVisitor(this);

    public void add(Vehicle vehicle) {
        vehicle.accept(addVisitor);
    }

    public void add(Car car) {
        carList.add(car);
    }

    public void add(Boat boat) {
        boatList.add(boat);
    }

    public void add(Plane plane) {
        planeList.add(plane);
    }

    public void printAllVehicles() {
        //loop through vehicles, print
    }
}

class AddTransportVisitor implements VehicleVisitor {
    private VehicleManager manager;

    public AddTransportVisitor(VehicleManager manager) {
        this.manager = manager;
    }

    public void visit(Car car) {
        manager.add(car);
    }

    public void visit(Plane plane) {
        manager.add(plane);
    }

    public void visit(Boat boat) {
       manager.add(boat);
    }
}

public class Main {
    public static void main(String[] args) {
        Vehicle[] vehicles = {
            new Plane(),
            new Car(),
            new Car(),
            new Car(),
            new Boat(),
            new Boat()
        };

        VehicleManager manager = new VehicleManager();
            for(Vehicle vehicle : vehicles) {
                manager.add(vehicle);
            }

            manager.printAllVehicles();
    }
}

Copeland

While this works I would consider this "solution" to be far worse than the problem he's trying to cure. – Ujiji

@LorenPechtel It would be nice if you elaborated on how it's way worse. – Copeland

@LorenPechtel, visitor is the most elegant solution even though it looks odd to an untrained eye. It allows turning single dispatch into multiple dispatch. The basic philosophy is just, "don't call us, we'll call you." – Invertebrate

@VinceEmigh I see two problems: 1) Vastly more code. 2) It means the objects know about the container, something they shouldn't. – Ujiji

@VinceEmigh If a vehicle is supposed to add itself to the correct list then it must know about the list to add itself to. – Ujiji

This is exactly the kind of solution I was looking for. I knew it could be done better but I just wasn't sure how. Thanks so much for taking the time to explain this in detail! I very much appreciate it. – Posthaste

@LorenPechtel A Vehicle doesn't add itself to a list. The visitor does the adding. Vehicle instances do not know of VehicleManager – Copeland

The visitor pattern is very similar to an if(... instanceof ...) chain, except that you will get an error if you miss any cases (good), and it requires a lot more boilerplate (bad). – Yamashita

@immibis They both give the same results: check the type, and yeah, the visitor pattern requires more code. But instanceof has it's problems (downcasting, no modularity, increased execution speed due to # of conditions) as well, so in all, it depends on the dev and the situation. – Copeland

@immibis the instanceof chain can catch an error--if you get to the bottom of the chain you missed a case and should throw an exception. – Ujiji

@LorenPechtel The instanceof chain doesn't catch errors; an exception won't be thrown due to not including a condition. Yeah, if you try casting without checking if that object is an instance of a specific type, then you'd get a ClassCastException, and you could consider that "error checking". But that's being informed of the error at runtime, while with the Visitor pattern, you'll get a compiler error if you try to call visitor.visit(this) and the type you are using isn't specified by the visitor. – Copeland

@VinceEmigh okay, the downcasts are boilerplate, but far less than there is with the visitor pattern. I'd say it has better modularity, because now all the code for dealing with the various cases is centralised in one place. – Yamashita

@immibis Boilerplate code isn't the big issue; not everyone minds writing extra syntax. That explains why some prefer the visitor although it's a lot more verbose. If you'd like to know why (and continue this discussion), please visit this chat – Copeland

Answer 2

You can add method to vehicle class to print the text. Then override the method in each specialized Car class. Then just add all the cars to the vehicle list. And loop the list to print the text.

Shigella

Answer 3

Done some restructuring of your code. Hope that works for you. Check this:

    import java.util.ArrayList;

    class VehicleManager {

        public static void main(String[] args) {

            ArrayList<ABluePrint> bluePrints = new ArrayList<ABluePrint>();
            ArrayList<AVehicle> allVehicles = new ArrayList<AVehicle>();
            ArrayList<ACar> cars = null;
            ArrayList<ABoat> boats = null;
            ArrayList<APlane> planes = null;

            /*
            *  In my application I have to access the blueprints through an API
            *  b/c they have already been created and stored in a data file.
            *  I'm creating them here just for example.
            */
            ABluePrint bp0 = new ABluePrint(0);
            ABluePrint bp1 = new ABluePrint(1);
            ABluePrint bp2 = new ABluePrint(2);
            bluePrints.add(bp0);
            bluePrints.add(bp1);
            bluePrints.add(bp2);

            for (ABluePrint bp : bluePrints) {
                AVehicle v = AVehicleFactory.buildVehicle(bp);
                allVehicles.add(v);

                // Can I accomplish this without using 'instanceof'?

                // dont add objects to list here, do it from constructor or in factory
                /*if (v instanceof ACar) {
                    cars.add((ACar) v);
                } else if (v instanceof ABoat) {
                    boats.add((ABoat) v);
                } else if (v instanceof APlane) {
                    planes.add((APlane) v);
                }*/
            }

            cars = ACar.getCars();
            boats = ABoat.getBoats();
            planes = APlane.getPlanes();

            System.out.println("All Vehicles:");
            for (AVehicle v : allVehicles) {
                System.out.println("Vehicle: " + v + ", maxSpeed: " + v.maxSpeed);
            }

            System.out.println("Cars:");
            for (ACar c : cars) {
                System.out.println("Car: " + c + ", numCylinders: " + c.numCylinders);
            }

            System.out.println("Boats:");
            for (ABoat b : boats) {
                System.out.println("Boat: " + b + ", numRudders: " + b.numRudders);
            }

            System.out.println("Planes:");
            for (APlane p : planes) {
                System.out.println("Plane: " + p + ", numPropellers: " + p.numPropellers);
            }
        }
    }

    class AVehicle {

        double maxSpeed;

        AVehicle(double maxSpeed) {
            this.maxSpeed = maxSpeed;
        }

        void add(){}
    }

    class ACar extends AVehicle {

        static ArrayList<ACar> cars = new ArrayList<ACar>();
        int numCylinders;

        ACar(double maxSpeed, int numCylinders) {
            super(maxSpeed);
            this.numCylinders = numCylinders;
        }

        void add(){
            cars.add(this);
        }

        public static ArrayList<ACar> getCars(){
            return cars;
        }
    }

    class ABoat extends AVehicle {

        static ArrayList<ABoat> boats = new ArrayList<ABoat>();
        int numRudders;

        ABoat(double maxSpeed, int numRudders) {
            super(maxSpeed);
            this.numRudders = numRudders;
        }

        void add(){
            boats.add(this);
        }

        public static ArrayList<ABoat> getBoats(){
            return boats;
        }
    }

    class APlane extends AVehicle {

        static ArrayList<APlane> planes = new ArrayList<APlane>();
        int numPropellers;

        APlane(double maxSpeed, int numPropellers) {
            super(maxSpeed);
            this.numPropellers = numPropellers;
        }

        void add(){
            planes.add(this);
        }

        public static ArrayList<APlane> getPlanes(){
            return planes;
        }
    }

    class AVehicleFactory {

        public static AVehicle buildVehicle(ABluePrint blueprint) {

            AVehicle vehicle;

            switch (blueprint.type) {

                case 0:
                    vehicle = new ACar(100.0, 4);
                    break;

                case 1:
                    vehicle = new ABoat(65.0, 1);
                    break;

                case 2:
                    vehicle = new APlane(600.0, 2);
                    break;

                default:
                    vehicle = new AVehicle(0.0);
            }

            vehicle.add();
            return vehicle;
        }
    }

    class ABluePrint {

        int type; // 0 = car; // 1 = boat; // 2 = plane;

        ABluePrint(int type) {
            this.type = type;
        }
    }

With the above code, the class will have to know about the collection to which it has to be added. This can be considered as a downside to a good design and it can be overcome using the visitor design pattern as demonstrated in the accepted answer (How to avoid 'instanceof' when implementing factory design pattern?).

Geraint

The question is: how to avoid type checks, which are a design smell. Using Class.isInstance instead of instanceof doesn't remove the type check. – Stoffel

Any instance of AVehicle should not be shouldered with the responsibility of add()-ing itself to... wait, even the add() method is implemented wrongly. Calling add() in this code just adds itself to its own list every time. This is quite a regression. – Kamenskuralski

@Geraint sorry, now I see the static modifier on each of the class's field... Still, your solution imposes an understanding that any AVehicle instance knows how to to add() itself to somethiing, which makes this a very mysterious method. And what if we are not adding to a List, but to a Set, Map, SecretDungeonOfMotorizedAssets etc.? There will still be a lot of places to update... – Kamenskuralski

Answer 4

I'm not too happy with the lists of cars, boats and planes in the first place. You have multiple examples of reality but the list isn't inherently all-inclusive--what happens when your factory starts making submarines or rockets?

Instead, how about an enum with the types car, boat and plane. You have an array of lists of vehicles.

The generic vehicle has an abstract property CatalogAs, the various vehicles actually implement this and return the proper value.

Ujiji

I gotta agree, the use of multiple lists is a downfall. But the enum system you suggest doesn't allow each subtype of vehicle (Car, Boat) to have it's own unique properties (boat has rudders, car has cylidars, ect..), nor does it allow multiple instances. He could create multiple enum declarations for each type (enum CarType), then compose Vehicle of it, but that's another design flaw: an object shouldn't be composed of a type. Objects should be of a type (is that type or a subtype of it). Plus, writing implementations for specific enums could make a file quite bulky – Copeland

@VinceEmigh Sure, it does. I'm not saying to remove the code he already has, just add a property that identifies how to classify the vehicle. Use that as the index to an array of lists that hold them. My version uses fewer statements than his instance-of version he doesn't like. – Ujiji

It's not always about how many statements are used. A lot of other things play a role in writing "efficient code"; it it were just about how many lines of code or statements, private fields with getter methods would be shunned. – Copeland

@VinceEmigh Private?? The routine putting it in the lists has to see it, it can't be private. – Ujiji

I'm not saying anything in my answer is private. That statement was an example for how less code isnt always better. public fields remove the need for getter methods, but allow mutability. The number of lines of code does not matter as much as you think. It's all about design.. – Copeland

Answer 5

I know its been a long time since this question was asked. I found http://www.nurkiewicz.com/2013/09/instanceof-operator-and-visitor-pattern.html which looks to be useful. Sharing it here in case if somebody is interested.

Archambault

Answer 6

Had a similar issue so I used this pattern, to understand it better I created a simple UML drawing showing the sequence of things in comments (follow the numbers). I used Vince Emighs solution above.. The pattern solution is more elegant but can requires some time to truly understand. It requires one interface and one class more then the original but they are very simple.

Morphine

Answer 7

What if AVehicle classes are out of your control? E.g. you have it from some 3rd party lib? So you have no way to add the Visitor pattern accept() method. Also you could probably dislike boilerplate code in each of the AVehicle subclass and prefer to put everything in one special class keeping your classes clean. For some cases it could be better just to use HashMap.

In your sample just use:

Map<Class<? extends AVehicle>, List<? extends AVehicle>> lists = new HashMap<>();
lists.put(ACar.class, new ArrayList<ACar>());
lists.put(ABoat.class, new ArrayList<ABoat>());
lists.put(APlane.class, new ArrayList<APlane>());

for (ABluePrint bp : bluePrints) {
     AVehicle v = AVehicleFactory.buildVehicle(bp);
     allVehicles.add(v);
     lists.get(v.getClass()).add(v);
}

The problem with this HashMap approach is that you have to register all possible classes including all known subclasses. Although if you have huge hierarchy and it is not needed all classes for your task you can save lots of work registering in the Map just needed ones.

Psia