Conversation
| private final int startingFloor; | ||
| private final int destinationFloor; | ||
| private final TravelDirection desiredDirection; | ||
| private final Object lock = new Object(); |
There was a problem hiding this comment.
could also lock on this but yeah, this is also an idiom, neat
| synchronized (lock) { | ||
| return currentState; | ||
| } |
There was a problem hiding this comment.
thats probably obsolete, just a getter
| return currentEnteredElevatorId == null | ||
| ? OptionalInt.empty() | ||
| : OptionalInt.of(currentEnteredElevatorId); | ||
| synchronized (lock) { |
There was a problem hiding this comment.
probably obsolete as just a getter
| .add("destinationFloor=" + destinationFloor) | ||
| .add("currentEnteredElevatorId=" + currentEnteredElevatorId) | ||
| .toString(); | ||
| synchronized (lock) { |
There was a problem hiding this comment.
definitely unecessary. looks like synchronized-knowledge isnt super deep yet
| currentState = State.ARRIVED; | ||
| return; | ||
| } | ||
| if (currentState.equals(State.WAITING_FOR_ELEVATOR) |
There was a problem hiding this comment.
use == for enums. also inconsistent as == was used right above
| if (currentState.equals(State.WAITING_FOR_ELEVATOR) | ||
| && elevatorPanel.getCurrentFloor() == startingFloor | ||
| /* or advertised direction equals TravelDirection.NONE part is needed here as advertised direction | ||
| can be none if this was the last stop of the existing queue but more people need to board still */ | ||
| && ((elevatorPanel.getAdvertisedDirection() == desiredDirection) | ||
| || elevatorPanel.getAdvertisedDirection() == TravelDirection.NONE)) { |
There was a problem hiding this comment.
that stuff is a bit complex to read/understand. a helper method with proper javadoc would be useful
| private final int minFloor; | ||
| private final int floorsServed; | ||
| private final int maxFloor; | ||
| private final int[] requests; |
There was a problem hiding this comment.
naming issue, what does int represent? (human IDs? floors?)
| synchronized (lock) { | ||
| addRequest(destinationFloor); | ||
| } |
There was a problem hiding this comment.
a bit of excessive placement of sync everywhere. addRequest syncs itself already, so this one for example isnt needed
| .add("currentFloor=" + currentFloor) | ||
| .toString(); | ||
| public String toString() { | ||
| synchronized (lock) { |
| * | ||
| * @return a boolean (true means there are more requests in current direction, otherwise false) | ||
| */ | ||
| private boolean moreRequestsInCurrentDirection() { |
There was a problem hiding this comment.
name should start with is or are or something like that
| if (currentDirection == TravelDirection.DOWN) { | ||
| return countRequestsBelow() > 0; | ||
| } | ||
| throw new RuntimeException("currentDirection is in an invalid state!"); |
There was a problem hiding this comment.
would be useful to add the current state to the error message
| private void goUp() { | ||
| synchronized (lock) { | ||
| currentFloor += 1; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Moves the elevator down one floor | ||
| */ | ||
| private void goDown() { | ||
| synchronized (lock) { | ||
| currentFloor -= 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
no protection for the floor range
| throw new IllegalArgumentException(INVALID_FLOOR_ERROR); | ||
| } | ||
| synchronized (lock) { | ||
| requests[floor - minFloor] += 1; |
There was a problem hiding this comment.
floor - minFloor, would be useful to put that into a well named var
| throw new IllegalArgumentException(INVALID_FLOOR_ERROR); | ||
| } | ||
| synchronized (lock) { | ||
| return requests[floor - minFloor]; |
There was a problem hiding this comment.
(floor - minFloor, would be useful to put that into a well named var)
| throw new IllegalArgumentException(INVALID_FLOOR_ERROR); | ||
| } | ||
| synchronized (lock) { | ||
| requests[floor - minFloor] = 0; |
There was a problem hiding this comment.
floor - minFloor, would be useful to put that into a well named var.
since its repeated 3 times now, maybe even a helper method
| private static Elevator findNearestElevator(List<Elevator> elevators, int floor) { | ||
| Elevator nearest = null; | ||
| int nearestDistance = Integer.MAX_VALUE; | ||
| for (Elevator elevator : elevators) { | ||
| int distance = Math.abs(elevator.getCurrentFloor() - floor); | ||
| if (distance < nearestDistance) { | ||
| nearest = elevator; | ||
| nearestDistance = distance; | ||
| } | ||
| } | ||
| return nearest; |
There was a problem hiding this comment.
the method mentions returning null is a thing.
in that case Optional would have been the more appriopriate pick
| result = findNearestElevator(elevators, floor); | ||
| } | ||
| if (result == null) { | ||
| throw new RuntimeException("Found no elevators < Integer.MAX_VALUE distance away from complete list of elevators!"); |
There was a problem hiding this comment.
that "< Integer.MAX_VALUE distance" part is an assumption of implementation detail that isnt up to this method to make but only the find-method. comments like that easily drift apart from the implementation and rot
Zabuzard
left a comment
Zabuzard
left a comment
There was a problem hiding this comment.
also sophisticated implementation that operates relatively close to real life:
- elevators keep track of button presses, "floor requests"
- elevators move first in the direction with the most requests and do that until that direction is fully served, then they serve the other direction
- elevator system tries to find closest elevator currently going in the desired direction, otherwise closest elevator in general
- extras: multithreading/syncing
- code quality: okay, some things to improve but okay and has javadoc, cool. use of
synchronizedis a bit excessive, also on getters etc
1 participant
Submission by user C