-
Notifications
You must be signed in to change notification settings - Fork 4
Submission E #10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Submission E #10
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,10 @@ | ||
| package org.togetherjava.event.elevator.elevators; | ||
|
|
||
| import java.util.StringJoiner; | ||
| import java.util.*; | ||
| import java.util.concurrent.atomic.AtomicInteger; | ||
|
|
||
| import static org.togetherjava.event.elevator.elevators.ElevatorSystem.mod; | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. prefer to not use static imports, it typically confuses readers |
||
|
|
||
| /** | ||
| * A single elevator that can serve a given amount of floors. | ||
| * <p> | ||
|
|
@@ -16,6 +18,10 @@ public final class Elevator implements ElevatorPanel { | |
| private final int minFloor; | ||
| private final int floorsServed; | ||
| private int currentFloor; | ||
| private int numberOfHumansAboard; | ||
| private int humanExited; | ||
| private List<Integer> requestedDestinationFloors; | ||
| private TravelDirection travellingDirection; | ||
|
|
||
| /** | ||
| * Creates a new elevator. | ||
|
|
@@ -37,6 +43,7 @@ public Elevator(int minFloor, int floorsServed, int currentFloor) { | |
| this.minFloor = minFloor; | ||
| this.currentFloor = currentFloor; | ||
| this.floorsServed = floorsServed; | ||
| this.requestedDestinationFloors = new ArrayList<>(); | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -57,12 +64,20 @@ public int getCurrentFloor() { | |
| return currentFloor; | ||
| } | ||
|
|
||
| public TravelDirection getTravellingDirection() { | ||
| return travellingDirection; | ||
| } | ||
|
|
||
| @Override | ||
| public void requestDestinationFloor(int destinationFloor) { | ||
| // TODO Implement. This represents a human or the elevator system | ||
| // itself requesting this elevator to eventually move to the given floor. | ||
| // The elevator is supposed to memorize the destination in a way that | ||
| // it can ensure to eventually reach it. | ||
| requestedDestinationFloors.add(destinationFloor); | ||
| if(destinationFloor > currentFloor) travellingDirection = TravelDirection.UP; | ||
| else if(destinationFloor < currentFloor) travellingDirection = TravelDirection.DOWN; | ||
| else travellingDirection = null; | ||
|
Comment on lines
+78
to
+80
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (add curlies, also spaces around the |
||
| System.out.println("Request for destination floor received"); | ||
| } | ||
|
|
||
|
|
@@ -76,15 +91,91 @@ public void moveOneFloor() { | |
| // meaning that the average time waiting (either in corridor or inside the elevator) | ||
| // is minimized across all humans. | ||
| // It is essential that this method updates the currentFloor field accordingly. | ||
| if(requestedDestinationFloors.isEmpty()) { | ||
| lastResort(); | ||
| return; | ||
| } | ||
| sortingList(); | ||
| int currentRequest = requestedDestinationFloors.getLast(); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. a bit unconventional to put the "best" request at the end instead of the front
Comment on lines
+98
to
+99
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. design-wise this is a bit difficult to understand that it belongs together: sortFoo();
int foo = foos.getLast();something better would be if it comes out of that method, so something like: int foo = pickBestFoo(); |
||
| if(currentRequest > currentFloor) { | ||
| currentFloor++; | ||
| travellingDirection = TravelDirection.UP; | ||
| } | ||
| else if(currentRequest < currentFloor) { | ||
| currentFloor--; | ||
| travellingDirection = TravelDirection.DOWN; | ||
| } | ||
| else { | ||
| requestedDestinationFloors.removeLast(); | ||
| removeRequests(); | ||
| } | ||
| System.out.println("Request to move a floor received"); | ||
| } | ||
|
|
||
| /** | ||
| * It is possible that the requests ends for the current elevator but there are still humans onboard. | ||
| * In that case the paternoster is the last resort to make sure the humans onboard reach the destination floor. | ||
|
Comment on lines
+116
to
+117
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how is that possible? lol. |
||
| */ | ||
| private void lastResort() { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. method naming. maybe |
||
| if(currentFloor == minFloor) travellingDirection = TravelDirection.UP; | ||
| else if(currentFloor == (minFloor + floorsServed -1)) travellingDirection = TravelDirection.DOWN; | ||
| else if(numberOfHumansAboard != 0) { | ||
| if(travellingDirection == TravelDirection.UP) currentFloor++; | ||
| else if(travellingDirection == TravelDirection.DOWN) currentFloor--; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * For instance, a number of humans can exit to the current floor. | ||
| * But the request for the current floor can make the elevator come to the current floor again or stay at it for consecutive steps. | ||
| * To prevent such loss of steps, this method removes the number of request according to the number of humans exited. | ||
| */ | ||
| private void removeRequests() { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. method naming. its unclear which requests it will remove. from the name it sounds like it clears all of them |
||
| for(int i = 0; i < humanExited; i++) { | ||
| for(int j = 0; j < requestedDestinationFloors.size(); j++) { | ||
| if(requestedDestinationFloors.get(j) == currentFloor) { | ||
| requestedDestinationFloors.remove(j); | ||
| humanExited--; | ||
| j--; | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+134
to
+142
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that logic looks a bit wobbly. especially with that outer loop, |
||
| } | ||
|
|
||
| /** | ||
| * To sort the list based on how far the requested floor is from the current floor | ||
| * The idea is to make the elevator go to the nearest or farthest request based on strategy. | ||
| */ | ||
|
|
||
| private void sortingList() { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. method naming could be improved. for example |
||
| for(int i = 0; i < requestedDestinationFloors.size() - 1; i++) { | ||
| if(mod(requestedDestinationFloors.get(i) - currentFloor) < mod(requestedDestinationFloors.get(i+1)) - currentFloor) { | ||
| int temp = requestedDestinationFloors.get(i + 1); | ||
| requestedDestinationFloors.remove(i + 1); | ||
| requestedDestinationFloors.add(i + 1,requestedDestinationFloors.get(i)); | ||
| requestedDestinationFloors.remove(i); | ||
| requestedDestinationFloors.add(i,temp); | ||
| } | ||
| } | ||
|
Comment on lines
+151
to
+159
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that looks a bit wobbly. and its also executed in every single step, so it probably contributes to some simulation slowdown. editing a list while you are iterating it is also dangerous, can easily blow up and lead to infinite loops |
||
| } | ||
|
|
||
| @Override | ||
| public void enteredElevator() { | ||
| numberOfHumansAboard++; | ||
| } | ||
|
|
||
| @Override | ||
| public void exitedElevator() { | ||
| numberOfHumansAboard--; | ||
| humanExited++; | ||
| } | ||
|
|
||
| @Override | ||
| public synchronized String toString() { | ||
| return new StringJoiner(", ", Elevator.class.getSimpleName() + "[", "]").add("id=" + id) | ||
| .add("minFloor=" + minFloor) | ||
| .add("floorsServed=" + floorsServed) | ||
| .add("currentFloor=" + currentFloor) | ||
| .add("currentFloor=" + this.currentFloor) | ||
| .toString(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,4 +25,15 @@ public interface ElevatorPanel { | |
| * @param destinationFloor the desired destination, must be within the range served by this elevator | ||
| */ | ||
| void requestDestinationFloor(int destinationFloor); | ||
|
|
||
|
|
||
| /** | ||
| * Counts the number of humans exited the elevator and updates the number of humans aboard counter. | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. worded a bit too much implementation-detailed. like, that second part of the sentence should just be dropped "updates the number of humans aboard counter", thats implementation details that might change and will eventually rot |
||
| */ | ||
| void exitedElevator(); | ||
|
|
||
| /** | ||
| * Counts the number of humans entered the elevator. | ||
| */ | ||
| void enteredElevator(); | ||
|
Comment on lines
+33
to
+38
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fair addition. (but IRL elevators usually dont really have an awareness for how many humans are aboard) |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,8 +2,7 @@ | |
|
|
||
| import org.togetherjava.event.elevator.humans.ElevatorListener; | ||
|
|
||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
| import java.util.*; | ||
|
|
||
| /** | ||
| * System controlling all elevators of a building. | ||
|
|
@@ -16,6 +15,7 @@ public final class ElevatorSystem implements FloorPanelSystem { | |
| private final List<Elevator> elevators = new ArrayList<>(); | ||
| private final List<ElevatorListener> elevatorListeners = new ArrayList<>(); | ||
|
|
||
|
|
||
| public void registerElevator(Elevator elevator) { | ||
| elevators.add(elevator); | ||
| } | ||
|
|
@@ -39,9 +39,48 @@ public void requestElevator(int atFloor, TravelDirection desiredTravelDirection) | |
| // The human can then enter the elevator and request their actual destination within the elevator. | ||
| // Ideally this has to select the best elevator among all which can reduce the time | ||
| // for the human spending waiting (either in corridor or in the elevator itself). | ||
|
|
||
| final Elevator bestElevator = getBestElevator(atFloor, desiredTravelDirection); | ||
| bestElevator.requestDestinationFloor(atFloor); | ||
|
|
||
| System.out.println("Request for elevator received"); | ||
| } | ||
|
|
||
| /** | ||
| * Cases covered :- | ||
| * 1. Lift has no requests, that means travelling direction is null. This makes sure each and every elevator moves to | ||
| * take requests from humans. | ||
| * 2. Lift has requests, and it'll go in the direction of the said human currently requesting. | ||
| * This prioritises that the human requesting hops in the elevator moving in their direction. | ||
| * 3.Lift has requests, and it'll go against the direction of the said human currently requesting. | ||
| * This ensures that humans will get a lift coming in there direction even if they may have to wait longer. | ||
| * | ||
| * @param atFloor the floor from which the human is requesting from. | ||
| * @param desiredTravelDirection the direction said human wants to go, this will help to provide them the best lift. | ||
| * @return | ||
| */ | ||
| private Elevator getBestElevator(int atFloor, TravelDirection desiredTravelDirection) { | ||
| Elevator bestElevator = elevators.getFirst(); | ||
| for(Elevator elevator : elevators) { | ||
| if(elevator.getTravellingDirection() == null && mod((bestElevator.getCurrentFloor() - atFloor)) > mod((elevator.getCurrentFloor() - atFloor))) { | ||
| bestElevator = elevator; | ||
| } | ||
| else if(elevator.getTravellingDirection() == desiredTravelDirection && mod((bestElevator.getCurrentFloor() - atFloor)) > mod((elevator.getCurrentFloor() - atFloor))) { | ||
| bestElevator = elevator; | ||
| } | ||
| else if(mod((bestElevator.getCurrentFloor() - atFloor)) > mod((elevator.getCurrentFloor() - atFloor))) { | ||
| bestElevator = elevator; | ||
| } | ||
| } | ||
| return bestElevator; | ||
| } | ||
|
|
||
| static int mod(int num) { | ||
| int modValue = num; | ||
| if(modValue < 0) modValue *= -1; | ||
| return modValue; | ||
| } | ||
|
Comment on lines
+78
to
+82
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not quite sure what that is supposed to do. looks like |
||
|
|
||
| public void moveOneFloor() { | ||
| elevators.forEach(Elevator::moveOneFloor); | ||
| elevators.forEach(elevator -> elevatorListeners.forEach(listener -> listener.onElevatorArrivedAtFloor(elevator))); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,9 +2,11 @@ | |
|
|
||
| import org.togetherjava.event.elevator.elevators.ElevatorPanel; | ||
| import org.togetherjava.event.elevator.elevators.FloorPanelSystem; | ||
| import org.togetherjava.event.elevator.elevators.TravelDirection; | ||
|
|
||
| import java.util.OptionalInt; | ||
| import java.util.StringJoiner; | ||
| import java.util.concurrent.atomic.AtomicInteger; | ||
|
|
||
| /** | ||
| * A single human that starts at a given floor and wants to | ||
|
|
@@ -14,9 +16,12 @@ | |
| * for example requesting an elevator, eventually entering and exiting them. | ||
| */ | ||
| public final class Human implements ElevatorListener { | ||
| private static final AtomicInteger NEXT_ID = new AtomicInteger(0); | ||
| private final int id; | ||
| private State currentState; | ||
| private final int startingFloor; | ||
| private final int destinationFloor; | ||
| private boolean requestedDestinationFloor; | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should start with sth like |
||
| /** | ||
| * If the human is currently inside an elevator, this is its unique ID. | ||
| * Otherwise, this is {@code null} to indicate that the human is currently on the corridor. | ||
|
|
@@ -36,13 +41,16 @@ public Human(int startingFloor, int destinationFloor) { | |
| if (startingFloor <= 0 || destinationFloor <= 0) { | ||
| throw new IllegalArgumentException("Floors must be at least 1"); | ||
| } | ||
|
|
||
| this.id = NEXT_ID.getAndIncrement(); | ||
| this.startingFloor = startingFloor; | ||
| this.destinationFloor = destinationFloor; | ||
|
|
||
| currentState = State.IDLE; | ||
| this.requestedDestinationFloor = false; | ||
| this.currentState = State.IDLE; | ||
| } | ||
|
|
||
| public int getId() { | ||
| return id; | ||
| } | ||
| public State getCurrentState() { | ||
| return currentState; | ||
| } | ||
|
|
@@ -60,6 +68,15 @@ public void onElevatorSystemReady(FloorPanelSystem floorPanelSystem) { | |
| // TODO Implement. The system is now ready and the human should leave | ||
| // their initial IDLE state, requesting an elevator by clicking on the buttons of | ||
| // the floor panel system. The human will now enter the WAITING_FOR_ELEVATOR state. | ||
| if(startingFloor == destinationFloor) { | ||
| currentState = State.ARRIVED; | ||
| return; | ||
| } | ||
| TravelDirection travelDirection; | ||
| if(startingFloor - destinationFloor > 0) travelDirection = TravelDirection.UP; | ||
| else travelDirection = TravelDirection.DOWN; | ||
|
Comment on lines
+76
to
+77
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (in java its more common to use curly-braces even on one-liner ifs)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. a ternary could also be used here for doing it more compact: Foo foo = condition ? A : B; |
||
| floorPanelSystem.requestElevator(startingFloor, travelDirection); | ||
| currentState = State.WAITING_FOR_ELEVATOR; | ||
| System.out.println("Ready-event received"); | ||
| } | ||
|
|
||
|
|
@@ -70,7 +87,25 @@ public void onElevatorArrivedAtFloor(ElevatorPanel elevatorPanel) { | |
| // elevator and request their actual destination floor. The state has to change to TRAVELING_WITH_ELEVATOR. | ||
| // If the human is currently traveling with this elevator and the event represents | ||
| // arrival at the human's destination floor, the human can now exit the elevator. | ||
| System.out.println("Arrived-event received"); | ||
| if(currentEnteredElevatorId == null) { | ||
| if (currentState.equals(State.WAITING_FOR_ELEVATOR) && elevatorPanel.getCurrentFloor() == this.getStartingFloor()) { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. enums can and should be checked with |
||
| currentEnteredElevatorId = elevatorPanel.getId(); | ||
| if(!requestedDestinationFloor) { | ||
| elevatorPanel.requestDestinationFloor(this.destinationFloor); | ||
| elevatorPanel.enteredElevator(); | ||
| requestedDestinationFloor = true; | ||
| } | ||
| this.currentState = State.TRAVELING_WITH_ELEVATOR; | ||
| } | ||
| } | ||
| else { | ||
| if (currentState.equals(State.TRAVELING_WITH_ELEVATOR) && elevatorPanel.getId() == this.currentEnteredElevatorId && elevatorPanel.getCurrentFloor() == this.getDestinationFloor()) { | ||
| this.currentState = State.ARRIVED; | ||
| this.currentEnteredElevatorId = null; | ||
| elevatorPanel.exitedElevator(); | ||
| System.out.println("Arrived-event received"); | ||
| } | ||
| } | ||
|
Comment on lines
+90
to
+108
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is coded a bit too defensively and hence its easy to not notice bugs. better would be to change the structure to check on the state and do the rest as assertions: if (currentState == State.WAITING_FOR_ELEVATOR) {
if (currentEnteredElevatorId != null) throw IllegalStateException("cant be in an elevator when waiting for one...");
...
} else if (currentState == State.TRAVELING_WITH_ELEVATOR) {
if (something is wrong...) throw ...
...
}
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (doing it based on the states is often called "state-machine") |
||
| } | ||
|
|
||
| public OptionalInt getCurrentEnteredElevatorId() { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,6 +64,10 @@ public static Simulation createRandomSimulation(long seed, int amountOfElevators | |
|
|
||
| return new Simulation(elevators, humans); | ||
| } | ||
| public static Simulation createCustomSimulation() { | ||
| return new Simulation(List.of(new Elevator(1, 10, 10)), | ||
| List.of(new Human(2, 2),new Human(1, 2))); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. missing space after |
||
| } | ||
|
|
||
| public Simulation(List<Elevator> elevators, List<Human> humans) { | ||
| this.elevators = new ArrayList<>(elevators); | ||
|
|
@@ -96,7 +100,6 @@ public void start() { | |
|
|
||
| public void step() { | ||
| elevatorSystem.moveOneFloor(); | ||
|
|
||
| humanStatistics.forEach(HumanStatistics::step); | ||
| stepCount++; | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_should be shifted to avoid confusion (10_000)