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
– Turing85, Commented Feb 28, 2019 at 14:38
One more observation: with a little bit more refactoring, i.e. deploying for instance the Builder pattern, you could further improve readability of your method: VehicleVuScore vehicleVuScore = VehicleVuScore.builder().from(vehicleUtilization).build(); vehicleVuScoreList.add(vehicleVuScore); — Turing85
– Turing85, Commented Feb 28, 2019 at 14:56

Jochen · Accepted Answer · 2019-02-28 14:38:28Z

4

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.

answered Feb 28, 2019 at 14:38

Jochen

1931 silver badge8 bronze badges

Sign up to request clarification or add additional context in comments.

Comments

Abhijay · Accepted Answer · 2019-02-28 15:18:26Z

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, 2019 at 15:18

answered Feb 28, 2019 at 14:36

Abhijay

3281 silver badge8 bronze badges

Comments

meriton · Accepted Answer · 2019-02-28 15:28:59Z

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));
}

Collectives™ on Stack Overflow

Merging duplicate code that use different objects

3 Answers 3

Comments

Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related