0

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?

General Grievance
  • 4,555
  • 31
  • 31
  • 45
Mayabi
  • 1
  • 2
  • 1
    A `switch` wouldn't do it but you can make a `Map` and change your code to use `map.get(vehicleName)`. – VLAZ Feb 04 '21 at 16:54
  • Could you give an example by taking a line from the code? – Mayabi Feb 06 '21 at 19:31
  • Could you elaborate on how can i use Map map = new HashMap(); to solve this by showing an example? Thanks @VLAZ – Mayabi Feb 09 '21 at 14:39
  • @VLAZ surely a map is the preferred way, but why wouldn't a switch work? – juwil Feb 09 '21 at 20:56
  • @VLAZ That is surprising for me as well! A switch makes the metric values go up by quite a lot of points. – Mayabi Feb 09 '21 at 20:59
  • @juwil because it doesn't reduce the cyclomatic complexity. You still have the same amount of branches in the same function. Cyclomatic complexity just counts the number of possible paths to the code. – VLAZ Feb 09 '21 at 20:59
  • @juwil The OP is seeking to reduce the cyclomatic complexity of the code, so a switch would give equivalent behavior and might look less noisy, but it will not result in a complexity reduction. – General Grievance Feb 09 '21 at 21:00
  • @Mayabi one extra `case` is one extra branch of code. It's also equivalent to one extra `if`. If you don't have `case`s or `if`s, you don't have extra branches. Getting things from a map only has two possibilities - either you find it or not (two code paths). – VLAZ Feb 09 '21 at 21:01
  • Can you guys show how to refactor this with hashmaps? So i could get it done. Thanks @VLAZ – Mayabi Feb 09 '21 at 21:03

2 Answers2

2

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");
Erik
  • 578
  • 2
  • 9
  • Map map = new HashMap(); Can this be used to solve the problem? Any other approach is more than welcome. I need one more approach to refactor my code as well. Thanks again @Erik – Mayabi Feb 09 '21 at 20:51
  • Nice use of lambdas. – General Grievance Feb 09 '21 at 21:03
  • 1
    I'm not sure that a `Map` would be helpful here, but I'm lacking some context. In your code, all `Boat`s will be named "Apollo " and all `Car`s will be named "BMW" which doesn't make sense. Are these names supposed to be hard-coded? – Erik Feb 09 '21 at 21:04
  • 1
    So close, but why did you make all the objects receive string `"Apollo"` as the constructor argument? That's not what the code in the question is doing. Use [`Supplier`](https://docs.oracle.com/javase/8/docs/api/java/util/function/Supplier.html), and lambdas like `() -> new Boat("Apollo ")` and `() -> new Train("Virgin", 4)`. – Andreas Feb 09 '21 at 21:06
  • Thanks @Calculuswhiz ! Could you show an example taking a line from the code? – Mayabi Feb 09 '21 at 21:07
  • Hi @Andreas by using Supplier How can i make all the objects receive string for the vehicleNames? – Mayabi Feb 09 '21 at 21:12
  • @Mayabi What vehicleName? The code in the question has a `vehicleName` parameter, which is not what is passed to the object constructors. The `vehicleName` parameter is the key to the map shown in this answer. If the code in the question is not what you want, why did you show us that code? – Andreas Feb 09 '21 at 23:07
1

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 
      
    }
}
OscarRyz
  • 196,001
  • 113
  • 385
  • 569