Alternative way of implementing nested if statement

Question

I am trying to refactor this code I have below that uses several nested if statements to check if two list contains the same items.

List<Car> CarList = CarService.findCarByConfigtype(pageName);
for (int i = 0; i < CarList.size(); i++) {
    System.out.println(CarRestApiController.data().getModel());
    if (CarList.get(i).getModel().equals(CarRestApiController.data().getModel())) {
        dataFound.add(CarList.get(i).getModel());
        if (CarList.get(i).getDerivative().equals(CarRestApiController.data().getDerivative())) {
            dataFound.add(CarList.get(i).getDerivative());
            if (CarList.get(i).getSvp().equals(CarRestApiController.data().getSvp())) {
                dataFound.add(CarList.get(i).getSvp());
                if (CarList.get(i).getEngine().equals(CarRestApiController.data().getEngine())) {
                    dataFound.add(CarList.get(i).getEngine());
                    if (CarList.get(i).getFueltype().equals(CarRestApiController.data().getFueltype())) {
                        dataFound.add(CarList.get(i).getFueltype());
                        if (CarList.get(i).getBodystyle().equals(CarRestApiController.data().getBodystyle())) {
                            dataFound.add(CarList.get(i).getBodystyle());
                            if (CarList.get(i).getTransmission().equals(CarRestApiController.data().getTransmission())) {
                                dataFound.add(CarList.get(i).getTransmission());
                                if (CarList.get(i).getSalescategory().equals(CarRestApiController.data().getSalescategory())) {
                                    dataFound.add(CarList.get(i).getSalescategory());
                                }
                            }
                        }
                    }
                }
            }
        }
    }
}

Answer 1

A solution could be to use Strategy design pattern. Have a strategy for each if statement, iterate over the list of strategies and process each car in the list

public interface CarFeatureStrategy {

    boolean canProcess(Car carToProcess, Car carToMatch);

    Object process(Car carToProcess);
}

The canHandle method should encapsulate the if statements which needs to be true to allow the processing and the process method should return the value of corresponding property of the car (for the example in the description there should be 8 strategies)

public class ModelStrategy implements CarFeatureStrategy {

    @Override
    public boolean canProcess(Car carToProcess, Car carToMatch) {
        return carToProcess.getModel().equals(carToMatch.getModel));
    }

    @Override
    public Object process(Car carToProcess) {
        return carToProcess.getModel();
    }
}

public class DerivativeStrategy implements CarFeatureStrategy {

    @Override
    public boolean canProcess(Car carToProcess, Car carToMatch) {
        return carToProcess.getModel().equals(carToMatch.getModel())
                && carToProcess.getDerivative().equals(carToMatch.getDerivative());
    }

    @Override
    public Object process(Car carToProcess) {
        return carToProcess.getDerivative();
    }
}

public class SvpStrategy implements CarFeatureStrategy {

    @Override
    public boolean canProcess(Car carToProcess, Car carToMatch) {
        return carToProcess.getModel().equals(carToMatch.getModel())
                && carToProcess.getDerivative().equals(carToMatch.getDerivative())
                && carToProcess.getSvp().equals(carToMatch.getSvp());
    }

    @Override
    public Object process(Car carToProcess) {
        return carToProcess.getSvp();
    }
}

// .... and so on for each condition which needs to be met
// EngineStrategy, FueltypeStrategy, BodystyleStrategy,
// TransmissionStrategy, SalescategoryStrategy

The CarProcessor retrieves the cars corresponding to the given pageName, retrieves the data from the CarRestApiController and uses the list of strategies to process the cars

public class CarProcessor {


    private CarService carService;
    private CarRestApiController restController;

    private List<CarFeatureStrategy> carFeatureStrategies;

    public void processCars(Object pageName) {
        // for example purpose the list of strategies is initialized here,
        // but it should be initialized somwhere where the initialization is done
        // only once rather than each time the processCars method is called
        carFeatureStrategies = new ArrayList<>();
        carFeatureStrategies.add(new ModelStrategy());
        carFeatureStrategies.add(new DerivativeStrategy());
        carFeatureStrategies.add(new SvpStrategy());
        // ....
        // add to the strategies list an instance of each strategy to process 
        // the car

        Car carToMatch = restController.data();
        List<Car> cars = carService.findCarByConfigtype(pageName);
        List<Object> dataFound = new ArrayList<>();

        for (Car carToProcess : cars) {
            for (CarFeatureStrategy carFeatureStrategy : carFeatureStrategies) {
                if (carFeatureStrategy.canProcess(carToProcess, carToMatch)) {
                    dataFound.add(carFeatureStrategy.process(carToProcess));
                }
            }
        }
    }
}

The example could be optimised implementing the Chain of responsibility design pattern. With a chain of responsibility the if statements in the canHandle methods will simplify to only one boolean condition per strategy.

For chain of responsibility the strategies have to be enhanced with a method to return the next strategy in the chain

public interface CarFeatureStrategy {

    boolean canProcess(Car carToProcess, Car carToMatch);

    Object process(Car carToProcess);

    CarFeatureStrategy next();
}

the strategy implementations have to be enhanced with a reference to the next strategy in the chain

public class ModelStrategy implements CarFeatureStrategy {

    private CarFeatureStrategy nextStrategy;

    public ModelStrategy(CarFeatureStrategy nextStrategy) {
        this.nextStrategy = nextStrategy;
    }

    @Override
    public boolean canProcess(Car carToProcess, Car carToMatch) {
        // check only the model
        return carToProcess.getModel().equals(carToMatch.getModel));
    }

    @Override
    public Object process(Car carToProcess) {
        return carToProcess.getModel();
    }

    @Override
    public CarFeatureStrategy next() {
        return this.nextStrategy;
    }
}

public class DerivativeStrategy implements CarFeatureStrategy {

    private CarFeatureStrategy nextStrategy;

    public DerivativeStrategy(CarFeatureStrategy nextStrategy) {
        this.nextStrategy = nextStrategy;
    }

    @Override
    public boolean canProcess(Car carToProcess, Car carToMatch) {
        // check only the derivative property
        return carToProcess.getDerivative().equals(carToMatch.getDerivative());
    }

    @Override
    public Object process(Car carToProcess) {
        return carToProcess.getDerivative();
    }

    @Override
    public CarFeatureStrategy next() {
        return this.nextStrategy;
    }
}

// ... and so on for all the strategies

The CarProcessor should build a chain of strategies and process each car until the chain is finished (the next method of current strategy returns null) or the current strategy cannot process the current car (canHandle method of current strategy returns false)

public class CarProcessor {

    private CarService carService;
    private CarRestApiController restController;

    public void processCars(Object pageName) {
        // for example purpose the chain of responsibilities is initialized here,
        // but it should be initialized somwhere where the initialization is done
        // only once rather than each time the processCars method is called

        // initialise the chain of responsibilities in revers order
        CarFeatureStrategy salesCategoryStrategy = new SalescategoryStrategy(null);
        CarFeatureStrategy transmissionStrategy = new TransmissionStrategy(salesCategoryStrategy);
        CarFeatureStrategy bodystyleStrategy = new BodystyleStrategy(transmissionStrategy);
        CarFeatureStrategy fueltypeStrategy = new FueltypeStrategy(bodystyleStrategy);
        CarFeatureStrategy engineStrategy = new EngineStrategy(fueltypeStrategy);
        // .... and so on until the first strategy in the chain
        CarFeatureStrategy modelStrategy = new ModelStrategy(...);

        Car carToMatch = restController.data();
        List<Car> cars = carService.findCarByConfigtype(pageName);
        List<Object> dataFound = new ArrayList<>();

        for (Car carToProcess : cars) {
            CarFeatureStrategy currentStrategy = modelStrategy;
            do {
                if ( !currentStrategy.canProcess(carToProcess, carToMatch)) {
                    // if current strategy cannot process the current car
                    // stop the chain
                    break;
                }

                dataFound.add(currentStrategy.process(carToProcess));
                // move to the next strategy in the chain
                currentStrategy = currentStrategy.next();

            } while (currentStrategy != null)
        }
    }
}

Answer 2

I would first save the result of CarList.get(i) and CarRestApiController.data() to variables as @T.J.Crowder suggested. Then I would flip the if-checks and use continue to get rid of the nesting. Like this:

List<Car> carList = CarService.findCarByConfigtype(pageName);

for (int i = 0; i < carList.size(); i++) {
     Car apiData = CarRestApiController.data();
     Car carListData = carList.get(i);

     System.out.println(CarRestApiController.data().getModel());

     if (!carListData.getModel().equals(apiData.getModel())) {
         continue;
     }
     dataFound.add(carListData.getModel());

     if (!carListData.getDerivative().equals(apiData.getDerivative())) {
         continue;
     }
     dataFound.add(carListData.getDerivative());

     if (!carListData.getSvp().equals(apiData.getSvp())) {
         continue;
     }
     dataFound.add(carListData.getSvp());

     // ... and so on.
 }