-
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
Conversation
| simulation.prettyPrint(); | ||
|
|
||
| if (simulation.getStepCount() >= 100_000) { | ||
| if (simulation.getStepCount() >= 100_00) { |
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)
| } | ||
| public static Simulation createCustomSimulation() { | ||
| return new Simulation(List.of(new Elevator(1, 10, 10)), | ||
| List.of(new Human(2, 2),new Human(1, 2))); |
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.
missing space after , between the two humans
|
|
||
|
|
||
| /** | ||
| * Counts the number of humans exited the elevator and updates the number of humans aboard counter. |
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.
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(); |
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.
fair addition. (but IRL elevators usually dont really have an awareness for how many humans are aboard)
| private State currentState; | ||
| private final int startingFloor; | ||
| private final int destinationFloor; | ||
| private boolean requestedDestinationFloor; |
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 start with sth like has (or is or are etc) so its clearer that its a boolean
| if(startingFloor - destinationFloor > 0) travelDirection = TravelDirection.UP; | ||
| else travelDirection = TravelDirection.DOWN; |
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.
(in java its more common to use curly-braces even on one-liner ifs)
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.
a ternary could also be used here for doing it more compact:
Foo foo = condition ? A : B;| if(currentEnteredElevatorId == null) { | ||
| if (currentState.equals(State.WAITING_FOR_ELEVATOR) && elevatorPanel.getCurrentFloor() == this.getStartingFloor()) { | ||
| 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"); | ||
| } | ||
| } |
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.
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 ...
...
}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.
(doing it based on the states is often called "state-machine")
| // 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()) { |
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.
enums can and should be checked with ==
| if(destinationFloor > currentFloor) travellingDirection = TravelDirection.UP; | ||
| else if(destinationFloor < currentFloor) travellingDirection = TravelDirection.DOWN; | ||
| else travellingDirection = null; |
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.
(add curlies, also spaces around the if (...))
| * The idea is to make the elevator go to the nearest or farthest request based on strategy. | ||
| */ | ||
|
|
||
| private void sortingList() { |
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.
method naming could be improved. for example sortRequestsByPriority
| * 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. |
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.
how is that possible? lol.
but great that u added a fallback
| * 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. | ||
| */ | ||
| private void lastResort() { |
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.
method naming. maybe moveOneFloorByFallbackStrategy
| return; | ||
| } | ||
| sortingList(); | ||
| int currentRequest = requestedDestinationFloors.getLast(); |
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.
a bit unconventional to put the "best" request at the end instead of the front
| sortingList(); | ||
| int currentRequest = requestedDestinationFloors.getLast(); |
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.
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();| * 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() { |
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.
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--; | ||
| } | ||
| } | ||
| } |
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.
that logic looks a bit wobbly. especially with that outer loop, i isnt used anywhere either. smells a bit.
| import java.util.*; | ||
| import java.util.concurrent.atomic.AtomicInteger; | ||
|
|
||
| import static org.togetherjava.event.elevator.elevators.ElevatorSystem.mod; |
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.
prefer to not use static imports, it typically confuses readers
| static int mod(int num) { | ||
| int modValue = num; | ||
| if(modValue < 0) modValue *= -1; | ||
| return modValue; | ||
| } |
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.
not quite sure what that is supposed to do. looks like Math.abs(x)? (abs = absolute).
the name mod sounds more like modulo (%)
| 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); | ||
| } | ||
| } |
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.
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
Zabuzard
left a comment
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.
working submission. the logic is a bit wobbly, potentially has some smaller bugs but its a decent attempt at improving the logic from something more basic
- humans enter/exit when elevators are at their corresponding floors
- elevator system calls the closest elevator that is going into the desired direction
- elevators move based on a list of requested floors. there is an attempt to sort/group these so they align in direction for an optimal order to serve them, great. the logic is a bit wobbly overall though (potentially has bugs), a fallback paternoster logic was added to cover cases where the code goes bonkers. great save, haha
- code quality: overall good, great javadoc as well, helper methods as well (could have better names though)
Submission by user E