Lift simulator in Java

Question

I implement a working version of the Lift Kata

Can you provide me some tips to improve my code?

I am not very proud of the handleLiftWithOpenDoors method that have a side effect on calls: as I am in a stream().map() function, I want the lambda to be pure, with no side effects. Do you think of something elegant to solve this problem?

public class LiftSystem {
    private final List<Integer> floors;
    private final Map<Integer, List<Call>> calls;
    private List<Lift> lifts;

    public LiftSystem(List<Integer> floors, List<Lift> lifts, List<Call> calls) {
        this.floors = floors;
        this.calls = calls.stream().collect(Collectors.groupingBy(Call::fromFloor));
        this.lifts = lifts;
    }

    public List<Integer> getFloorsInDescendingOrder() {
        List<Integer> shallowCopy = new ArrayList<>(floors);
        Collections.reverse(shallowCopy);
        return shallowCopy;
    }

    public List<Call> getCallsForFloor(int floor) {
        return calls.getOrDefault(floor, Collections.emptyList());
    }

    public List<Lift> getLifts() {
        return lifts;
    }

    public void tick() {
        lifts = lifts.stream()
                .map(lift -> {
                    List<Call> callsAtThisFloor = calls.getOrDefault(lift.floor(), Collections.emptyList());
                    if (lift.doorsOpen()) {
                        // FIXME handleLiftWithOpenDoors have a side effect on callsAtThisFloor
                        return handleLiftWithOpenDoors(lift, callsAtThisFloor);
                    } else {
                        return handleLiftWithClosedDoors(lift, callsAtThisFloor);
                    }
                })
                .collect(Collectors.toList());
    }

    private Lift handleLiftWithOpenDoors(Lift lift, List<Call> callsAtThisFloor) {
        List<Integer> remainingRequests = lift.filterRequests(lift.floor());

        if (!callsAtThisFloor.isEmpty()) {
            Direction liftDirection = liftDirection(lift, remainingRequests).orElse(callsAtThisFloor.get(0).direction());

            Map<Boolean, List<Call>> callsInLiftDirection = callsInLiftDirection(callsAtThisFloor, liftDirection);

            remainingRequests.addAll(callsInLiftDirection.get(true).stream().map(Call::destinationFloor).toList());

            // FIXME side effect : update the calls map ?
            calls.put(lift.floor(), callsInLiftDirection.get(false));
        }
        return lift.closeDoors(remainingRequests);
    }
    
    private Lift handleLiftWithClosedDoors(Lift lift, List<Call> callsAtThisFloor) {
        if (!lift.requests().isEmpty()) {
            return handleLiftWithRequests(lift, callsAtThisFloor);
        } else {
            if (!callsAtThisFloor.isEmpty()) {
                return lift.openDoors(lift.requests());
            } else {
                return handleWaitingCallsIfAny(lift);
            }
        }
    }

    private Lift handleWaitingCallsIfAny(Lift lift) {
        // find the closest call if any
        int floor = lift.floor();
        for (int i = 1; i < Math.max(lift.floor(), floors.size() - 1 - lift.floor()); i++) {
            if (isNotEmpty(calls.get(floor + i))) {
                return lift.moveUp();
            }
            if (isNotEmpty(calls.get(floor - i))) {
                return lift.moveDown();
            }
        }
        return lift;
    }

    private Lift handleLiftWithRequests(Lift lift, List<Call> callsForFloor) {
        if (lift.hasRequestForFloor(lift.floor())) {
            return lift.openDoors(lift.filterRequests(lift.floor()));
        } else {
            Direction liftDirection = liftDirection(lift, lift.requests()).get();
            if (hasCallsInLiftDirection(liftDirection, callsForFloor)) {
                return lift.openDoors(lift.requests());
            } else {
                return liftDirection.equals(Direction.UP) ? lift.moveUp() : lift.moveDown();
            }
        }
    }

    private boolean hasCallsInLiftDirection(Direction liftDirection, List<Call> callsForFloor) {
        return callsForFloor.stream().anyMatch(call -> call.direction().equals(liftDirection));
    }

    private static Map<Boolean, List<Call>> callsInLiftDirection(List<Call> callsForFloor, Direction liftDirection) {
        return callsForFloor.stream()
                .collect(Collectors.partitioningBy(call -> call.direction().equals(liftDirection)));
    }

    public boolean stillMoving() {
        return calls.values().stream().anyMatch(Predicate.not(Collection::isEmpty))
                || lifts.stream().anyMatch(lift -> lift.doorsOpen() || !lift.requests().isEmpty());
    }

    private Optional<Direction> liftDirection(Lift lift, List<Integer> remainingRequests) {
        if (remainingRequests.isEmpty()) {
            return Optional.empty();
        } else {
            int destination = lift.closestDestination();
            return Optional.of(destination > lift.floor() ? Direction.UP : Direction.DOWN);
        }
    }

    private boolean isNotEmpty(Collection<?> collection) {
        return collection != null && !collection.isEmpty();
    }
}


public record Lift(String id, int floor, List<Integer> requests, boolean doorsOpen) {
    public boolean hasRequestForFloor(int floor) {
        return this.requests.contains(floor);
    }

    public Lift openDoors(List<Integer> requests) {
        return new Lift(id, floor, requests, true);
    }

    public Lift closeDoors(List<Integer> requests) {
        return new Lift(id, floor, requests, false);
    }

    public int closestDestination() {
        return requests
                .stream()
                .min(Comparator.comparingInt(i -> Math.abs(i - floor)))
                .orElse(floor);
    }

    public Lift moveUp() {
        return new Lift(id, floor + 1, requests, doorsOpen);
    }

    public Lift moveDown() {
        return new Lift(id, floor - 1, requests, doorsOpen);
    }

    public List<Integer> filterRequests(int unwantedFloor) {
        return requests.stream()
                .filter(request -> request != unwantedFloor)
                .collect(Collectors.toList());
    }
}

public enum Direction {
    UP, DOWN;
}

public record Call (int fromFloor, int destinationFloor, Direction direction) {
}

Answer 1

In functional programming (FP) is referential transparency, in object oriented programming (OOP) is the single responsibility principle (SRP) of clean code and don't repeat yourself (DRY) principle of the pragmatic programmer.

Fix: implement the concern of lift calling for LiftSystem since it makes sense from all perspectives - real life, functional programming, object oriented programming - and call that concern from handleLiftWithOpenDoors...

 public void callLift(Lift liftToCall, List<Call> liftCalls) { calls.put(liftToCall.floor(), liftCalls); }

There are other improvements that implemented could unclutter the solution from different programming techniques perspectives. The tick method for instance chooses a handler and ticks lifts. Following OO programming SRP the tick method should just tick, that will let the LiftSystem code in a state leveraging further development with respect to the DRY principle if calling the lifts from multiple methods would be requested...

public void tick() {

    lifts = lifts.stream()
                 .map(lift -> { return LiftDoorHandlerFactory.handlerFor(lifts.get(0)).apply(lift, this); })
                 .collect(Collectors.toList());
    }

... and delegate the handler selection to a factory that being a LiftSystem concern could be implemented through a private static inner class of LiftSystem class...

private static class LiftDoorHandlerFactory {

    private static final BiFunction<Lift, LiftSystem, Lift> OPENED_DOORS_HANDLER = (liftToHandle, liftSystem) -> { 

        List<Integer> remainingRequests = liftToHandle.filterRequests(liftToHandle.floor());
        List<Call> callsAtThisFloor = liftSystem.getCallsForFloor(liftToHandle.floor());

        if (!callsAtThisFloor.isEmpty()) {
            Direction liftDirection = liftDirection(liftToHandle, remainingRequests).orElse(callsAtThisFloor.get(0).direction());

            Map<Boolean, List<Call>> callsInLiftDirection = callsInLiftDirection(callsAtThisFloor, liftDirection);

            remainingRequests.addAll( callsInLiftDirection.get(true).stream().map(Call::destinationFloor).collect(Collectors.toList())  );

            liftSystem.callLift(liftToHandle, callsInLiftDirection.get(false));
        }
        return liftToHandle.closeDoors(remainingRequests);
    };

    private static BiFunction<Lift, LiftSystem, Lift> CLOSED_DOORS_HANDLER = (liftToHandle, liftSystem) -> { 

        List<Call> callsAtThisFloor = liftSystem.getCallsForFloor(liftToHandle.floor());

        if (!liftToHandle.requests().isEmpty()) {
            return liftSystem.handleLiftWithRequests(liftToHandle, callsAtThisFloor);
        } else {
            if (!callsAtThisFloor.isEmpty()) {
                return liftToHandle.openDoors(liftToHandle.requests());
            } else {
                return liftSystem.handleWaitingCallsIfAny(liftToHandle);
            }
        }
    };

    private static Map<Boolean, List<Call>> callsInLiftDirection(List<Call> callsForFloor, Direction liftDirection) {
        return callsForFloor.stream()
                            .collect(Collectors.partitioningBy(call -> call.direction().equals(liftDirection)));
    }

    public static BiFunction<Lift, LiftSystem, Lift> handlerFor(Lift lift) { return ( lift.doorsOpen() ) ? OPENED_DOORS_HANDLER : CLOSED_DOORS_HANDLER; }
}

... also the domain specific language (DSL) could be different if Lift would know about its calls that being subject of personal preferences I am just mentioning it without diving into details.