CODEWITHSUNDEEP
javarefactoringcyclomatic-complexity
Mayabi
Mayabi

Reputation: 1

How to refactor to reduce complexity of this code?

I have the following code and i need to refactor it to reduce complexity and increase modularity and encapsulation. I also need to reduce the ck metrics value.

    private void initialiseVehicle(String vehicleName) {
        if(vehicleName.equals("Boat")) {
            vehicle = new Boat("Apollo ");
        }
        else if(vehicleName.equals("Ship")) {
            vehicle = new Ship("Cruizz");
        }
        else if(vehicleName.equals("Truck")) {
            vehicle = new Truck("Ford F-650");
        }
        else if(vehicleName.equals("Motorcycle")) {
            vehicle = new Motorcycle("Suzuki");
        }
        else if(vehicleName.equals("Bus")) {
            vehicle = new Bus("Aero");
        }
        else if(vehicleName.equals("Car")) {
            vehicle = new Car("BMW");
        }
        else if(vehicleName.equals("Bicycle")) {
            vehicle = new Bicycle("A-bike");
        }
        else if(vehicleName.equals("Helicopter")) {
            vehicle = new Helicopter("Eurocopter");
        }
        else if(vehicleName.equals("Airplane")) {
            vehicle = new Airplane("BA");
        }
        else if(vehicleName.equals("Tram")) {
            vehicle = new Tram("EdinburghTram");
        }
        else if(vehicleName.equals("Train")) {
            vehicle = new Train("Virgin",4);
        }       
    }

How do you refactor this code? Does switch-cases reduce any complexity?

Upvotes: 0

Views: 149

Answers (2)

OscarRyz
OscarRyz

Reputation: 199215

Using reflection:

Vehicle vehicle;

Map<String,String> m = new HashMap<>() {{
    put("Boat", "Apollo");
    put("Ship", "Cruizz");
    // etc
}};
private void initializeVehicle(String name) throws Exception {
    vehicle = (Vehicle) Class.forName(name)
                             .getConstructor(String.class)
                             .newInstance(m.get(name));
}

But I honestly think your origina code is simple enough. Cyclomatic complexity shouldn't be a goal on itself.

This code may score very low in CK but it's not as easy to understand than the if/else chain.

So, take into consideration what are you going to use this for, the above example is very useful for libraries that doesn't known upfront the class to be created.

Here's the full running example

import java.util.*;
import java.lang.reflect.*;
import static java.lang.System.out;

class Vehicle {
    String name;

    public Vehicle(String aName) {
        name = aName;
    }
}

class Boat extends Vehicle {
    public Boat(String s) {
        super(s);
    }
}

class Ship extends Vehicle {
    public Ship(String s) {
        super(s);
    }
}

class Main {
    Vehicle vehicle;

    Map<String,String> m = new HashMap<>() {{
        put("Boat", "Apollo");
        put("Ship", "Cruizz");
        // etc
    }};
    private void initializeVehicle(String name) throws Exception {
        vehicle = (Vehicle) Class.forName(name).getConstructor(String.class).newInstance(m.get(name));
    }

    public static void main(String... args) throws Exception {
      Main main = new Main();
      main.initializeVehicle("Ship");
      System.out.println(main.vehicle.name); // prints Cruizz as expected 
      
    }
}

Upvotes: 1

Erik
Erik

Reputation: 648

One option could look like this:

Map<String, Function<String, Vehicle>> constructors = new HashMap<>();
constructors.put("Boat", name -> new Boat(name));
constructors.put("Ship", name -> new Ship(name));

Then the if/else code could look like

Function<String, Vehicle> constructor = constructors.get(vehicleName);
Vehicle vehicle = constructor.apply("Apollo");

Upvotes: 2

Related Questions