Merging duplicate code that use different objects

Question

I use two api calls to get data about vehicleUtils depending on contentFilter. I have very similar code for both (drivers and vehicles). What i tried to do is to extract the code into a single method and apply Strategy pattern like they suggest here Refactoring methods, but i could not figure out how to implement it. Am i using a good approach or is there any better way?

if (contentFilter.equalsIgnoreCase(Contentfilters.VEHICLES.toString())) {

  VuScores vuScores = new VuScores();
  List<VehicleVuScores> vehicleVuScoresList = new ArrayList<>();
  List<VehicleUtilization> vehicleUtilizations = RestClient.getVehicles(request).join().getVehicleUtilizations();


  if (Objects.nonNull(vehicleUtilizations)) {
    vehicleUtilizations.forEach(vehicleUtilization -> {
      vuScores.getVehicleVuScores().forEach(vehicleVuScore -> {

        vehicleVuScore.getScores().setTotal(vehicleUtilization.getFuelEfficiencyIndicators().getTotal().getValue());
        vehicleVuScore.getScores().setBraking(vehicleUtilization.getFuelEfficiencyIndicators().getGroupIndicators().get(3).getIndicators().get(0).getValue());
        vehicleVuScore.getScores().setCoasting(vehicleUtilization.getFuelEfficiencyIndicators().getGroupIndicators().get(3).getIndicators().get(1).getValue());
        vehicleVuScore.getScores().setIdling(vehicleUtilization.getFuelEfficiencyIndicators().getGroupIndicators().get(0).getIndicators().get(0).getValue());
        vehicleVuScore.getScores().setAnticipation(vehicleUtilization.getFuelEfficiencyIndicators().getGroupIndicators().get(3).getValue());
        vehicleVuScore.getScores().setEngineAndGearUtilization(vehicleUtilization.getFuelEfficiencyIndicators().getGroupIndicators().get(1).getValue());
        vehicleVuScore.getScores().setStandstill(vehicleUtilization.getFuelEfficiencyIndicators().getGroupIndicators().get(0).getValue());
        vehicleVuScore.getScores().setWithinEconomy(vehicleUtilization.getFuelEfficiencyIndicators().getGroupIndicators().get(1).getIndicators().get(7).getValue());
        vehicleVuScore.setAvgFuelConsumptionPer100Km(vehicleUtilization.getMeasures().getTotal().getAverageConsumption().getValue());
        vehicleVuScore.setAvgSpeedDrivingKmh(vehicleUtilization.getMeasures().getTotal().getAverageSpeed().getValue());
        vehicleVuScore.setEngineLoad(vehicleUtilization.getFuelEfficiencyIndicators().getGroupIndicators().get(1).getIndicators().get(1).getValue());
        vehicleVuScore.setTotalDistanceInKm(vehicleUtilization.getMeasures().getDriving().getDistance().getValue());
        vehicleVuScore.setTotalTime(Math.toIntExact(vehicleUtilization.getMeasures().getTotal().getTime().getValue()));

        vehicleVuScoresList.add(vehicleVuScore);
      });
    });
    vuScores.setVehicleVuScores(vehicleVuScoresList);
  }
  return CompletableFuture.completedFuture(vuScores);

} else if (contentFilter.equalsIgnoreCase(Contentfilters.DRIVERS.toString())) {

  VuScores vuScores = new VuScores();
  List<DriverVuScores> driverVuScoresList = new ArrayList<>();
  List<VehicleUtilization> vehicleUtilizations = RestClient.getDrivers(request).join().getVehicleUtilizations();


  if (Objects.nonNull(vehicleUtilizations)) {
    vehicleUtilizations.forEach(vehicleUtilization -> {
      vuScores.getDriverVuScores().forEach(driverVuScores -> {

        driverVuScores.getScores().setTotal(vehicleUtilization.getFuelEfficiencyIndicators().getTotal().getValue());
        driverVuScores.getScores().setBraking(vehicleUtilization.getFuelEfficiencyIndicators().getGroupIndicators().get(3).getIndicators().get(0).getValue());
        driverVuScores.getScores().setCoasting(vehicleUtilization.getFuelEfficiencyIndicators().getGroupIndicators().get(3).getIndicators().get(1).getValue());
        driverVuScores.getScores().setIdling(vehicleUtilization.getFuelEfficiencyIndicators().getGroupIndicators().get(0).getIndicators().get(0).getValue());
        driverVuScores.getScores().setAnticipation(vehicleUtilization.getFuelEfficiencyIndicators().getGroupIndicators().get(3).getValue());
        driverVuScores.getScores().setEngineAndGearUtilization(vehicleUtilization.getFuelEfficiencyIndicators().getGroupIndicators().get(1).getValue());
        driverVuScores.getScores().setStandstill(vehicleUtilization.getFuelEfficiencyIndicators().getGroupIndicators().get(0).getValue());
        driverVuScores.getScores().setWithinEconomy(vehicleUtilization.getFuelEfficiencyIndicators().getGroupIndicators().get(1).getIndicators().get(7).getValue());
        driverVuScores.setAvgFuelConsumptionPer100Km(vehicleUtilization.getMeasures().getTotal().getAverageConsumption().getValue());
        driverVuScores.setAvgSpeedDrivingKmh(vehicleUtilization.getMeasures().getTotal().getAverageSpeed().getValue());
        driverVuScores.setEngineLoad(vehicleUtilization.getFuelEfficiencyIndicators().getGroupIndicators().get(1).getIndicators().get(1).getValue());
        driverVuScores.setTotalDistanceInKm(vehicleUtilization.getMeasures().getDriving().getDistance().getValue());
        driverVuScores.setTotalTime(Math.toIntExact(vehicleUtilization.getMeasures().getTotal().getTime().getValue()));

        driverVuScoresList.add(driverVuScores);
      });
    });
    vuScores.setDriverVuScores(driverVuScoresList);
  }
  return CompletableFuture.completedFuture(vuScores);
}

Looking at your code, I have got to wonder: do `VehicleVuScores` and `DriverVuScores` have a common superclass or a common interface? If so, the only difference I see (ad hoc) is the initialization of the `List` and the call to the REST API. The rest looks pretty much the same to me. The code within the `if-else`-cases would then collapse to two lines each, with the rest of the code being outside both cases, eliminating all duplication. — Turing85, Feb 28 '19 at 14:38
One more observation: with a little bit more refactoring, i.e. deploying for instance the [Builder pattern](https://en.wikipedia.org/wiki/Builder_pattern), you could further improve readability of your method: `VehicleVuScore vehicleVuScore = VehicleVuScore.builder().from(vehicleUtilization).build(); vehicleVuScoreList.add(vehicleVuScore);` — Turing85, Feb 28 '19 at 14:56
This question belongs on https://codereview.stackexchange.com/. — jaco0646, Feb 28 '19 at 16:21

score 4 · Answer 1 · answered Feb 28 '19 at 14:38

Try to think about a common (abstract) base class, that holds the common code. The actual classes hold the differing code.

You then don't need to to work with instanceof or Contentfilters or whatever kind decission functionality you use. You just can call the common methods, as your function should take the (abstract) base class. This really removes code duplication.

Abhijay · Answer 2 · 2019-02-28T15:18:26.353

1

Use an interface, implement it in both the classes, and use that interface in both places to get or set values. Since all the method names are same, the interface should contain all the necessary getters and setters. This way you won't have to use different classes.

edited Feb 28 '19 at 15:18

answered Feb 28 '19 at 14:36

Abhijay

278
1
8

meriton · Answer 3 · 2019-02-28T15:28:59.310

So, everything is the same except

the types of the DTO you copy the data to (VehicleVuScores vs DriverVuScores)
the RestClient method invoked

The main challenge is sharing the code that invokes the setters. We need a way to refer to the target object without knowing whether its a VehicleVuScores or a DriverVuScores. We could declare it as:

Object vuScores;

but since Object doesn't declare the setters, we'd get compilation errors when trying to invoke the setters. To fix that, we can move the declaration of these getters and setters into a common base type:

abstract class VuScoresBase {
    // fields, getters and setters
}

class DriverVuScores extends VuScoresBase {}
class VehicleVuScores extends VuScoresBase {}

so we can write:

public void convert(VehicleUtilization vehicleUtilization, VuScoresBase result) {
    // invoke the setters here
}

and use this method in both cases.

With generics, we could also reuse the iteration code:

<V extends VuScoresBase> public void convertList(List<VehicleUtilization> vehicleUtilizations, List<V> resultList, Supplier<V> constructor) {
    // iterate       
        V vuScore = constructor.apply();
        convert(vehicleUtilization, vuScore);
        resultList.add(vuScore);
}

so we could invoke it with

convertList(vehicleUtilizations, driverVuScores, DriverVuScore::new);

but i'd probably refrain from that, because the generics make the code hard to understand.

However, since the DriverVuScores and VehicleVuScores are so similar, I'd question whether we really need them to be separate types. If we can use VuScoresBase everywhere, this would vastly simplify the conversion logic:

VuScoresBase convert(VehicleUtilization vehicleUtilization) {
   VuScoresBase vuScores = new VuScoreBase();
   // invoke setters
   return vuScores;
}

and

List<VuScoresBase> convertList(List<VehicleUtilization> vehicleUtilizations) {
  // iterate
     result.add(convert(vehicleUtilization));
}

Merging duplicate code that use different objects

3 Answers3