Reducing Code Duplication with For loop in java

Question

    for (Aircraft aircraft : landingQueue) {
        if (aircraft.hasEmergency()) {
            return aircraft;
        }
    }

    for (Aircraft aircraft : landingQueue) {
        if (aircraft.getFuelPercentRemaining() <= 20) {
            return aircraft;
        }
    }

    for (Aircraft aircraft : landingQueue) {
        if (aircraft instanceof PassengerAircraft) {
            return aircraft;
        }
    }
    return landingQueue.get(0);

So my program runs through a list of aircraft in a queue and will return one based on the importance of a task. For instance, it will first check the landingQueue ArrayList to see if any aircraft are in an emergency and if it can't find one it then checks for aircraft with fuel less than 20 and so on. Is there a simple way to reduce the duplication of the for loop and if statement? Much appreciated

Eritrean · Accepted Answer · 2021-04-26 14:19:25Z

4

Another approach could be to sort your list according to your priority criteria

public Aircraft returnOneBasedOnImportance(){
    Comparator<Aircraft> byEmer = Comparator.comparing(a -> !a.hasEmergency());
    Comparator<Aircraft> byFuel = Comparator.comparing(a -> !(a.getFuelPercentRemaining() <= 20));
    Comparator<Aircraft> byType = Comparator.comparing(a -> !(a instanceof PassengerAircraft));

    return landingQueue.stream()
                       .sorted(byEmer.thenComparing(byFuel).thenComparing(byType))
                       .findFirst().get();
}

answered Apr 26, 2021 at 14:19

Eritrean

16.6k3 gold badges25 silver badges28 bronze badges

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

Comments

wi2ard · Accepted Answer · 2021-04-26 14:18:28Z

The improvement is not as much about code duplication, more important it is to limit the number of times you iterate through that list.

Aircraft firstEmergencyAircraft = null;
Aircraft firstLowFuelAircraft = null;
Aircraft firstPassengerAircraft = null;

for (Aircraft aircraft : landingQueue) {
        if (firstEmergencyAircraft==null && aircraft.hasEmergency()) {
            firstEmergencyAircraft = aircraft;
        }
        if (firstLowFuelAircraft==null && aircraft.getFuelPercentRemaining() <= 20) {
            firstLowFuelAircraft = aircraft;
        }
        if (firstPassengerAircraft==null && aircraft instanceof PassengerAircraft) {
            firstPassengerAircraft = aircraft;
        }
    }

if(firstEmergencyAircraft!=null){
    return firstEmergencyAircraft;
}
if(firstLowFuelAircraft!=null){
    return firstLowFuelAircraft;
}
if(firstPassengerAircraft!=null){
    return firstPassengerAircraft;
}
return landingQueue.get(0);

Dietmar Höhmann · Accepted Answer · 2021-04-26 13:12:09Z

0

I'd make landingQueue some sort of SortedSet (i.e. TreeSet) with a Comparator that sorts by landing priority (i.e. sort criteria are emergency, low fuel level, is PassangerAircraft). Then you can always just pick the first (or last) entry from the list.

answered Apr 26, 2021 at 13:12

Dietmar Höhmann

3973 silver badges10 bronze badges

Comments

3limin4t0r · Accepted Answer · 2021-04-26 15:42:10Z

0

One option would be to create a stream of predicates first, then for each predicate loop through the list and try to find an aircraft that matches. If no match is found, return the first element.

Stream<Predicate<Aircraft>> predicates = Stream.of(
  (aircraft) -> aircraft.hasEmergency(),
  (aircraft) -> aircraft.getFuelPercentRemaining() <= 20,
  (aircraft) -> aircraft instanceof PassengerAircraft
);

return predicates
  .flatMap((predicate) -> landingQueue.stream().filter(predicate))
  .findFirst()
  .orElse(landingQueue.get(0));

edited Apr 26, 2021 at 15:42

answered Apr 26, 2021 at 15:30

3limin4t0r

21.4k3 gold badges38 silver badges61 bronze badges

Comments

Gene McCulley · Accepted Answer · 2021-04-26 13:08:50Z

-3

Why not just do the tests in one loop?

for (Aircraft aircraft : landingQueue) {
    if (aircraft.hasEmergency() ||
        aircraft.getFuelPercentRemaining() <= 20 ||
        aircraft instanceof PassengerAircraft) {
        return aircraft;
    }
}

return landingQueue.get(0);

answered Apr 26, 2021 at 13:08

Gene McCulley

1,3071 gold badge13 silver badges16 bronze badges

5 Comments

DanielBarbarian Over a year ago

Wouldn't this change the logic? It should first check if there is any aircraft that has an emergency, now it will return any aircraft that matches any of the conditions.

Dominik Over a year ago

Changes logic just like in QBrute's answer

Gene McCulley Over a year ago

See en.wikipedia.org/wiki/Short-circuit_evaluation and 15.24 of the Java Language Specification (docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-15.24). Boolean operands are evaluated in order and will short circuit.

Sweeper Over a year ago

Suppose the second item in the landingQueue is a passenger aircraft, but has no emergency. The fifth item has emergency, but is not a passenger aircraft, and all aircrafts have more than 20 fuel. Your algorithm will return the second item in the queue, but OP's code will return the fifth item.

Gene McCulley Over a year ago

I considered deleting my answer now that I realize it is wrong. I am leaving it here to help others see the logic error. I recommend that everyone read @Eritrean's answer as that is correct and the most elegant, in my opinion.

Collectives™ on Stack Overflow

Reducing Code Duplication with For loop in java

5 Answers 5

Comments

Comments

Comments

Comments

5 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

5 Answers 5

Comments

Comments

Comments

Comments

5 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related