Submission B #7
Submission B #7
Conversation
|
|
||
| simulation.printResult(); | ||
| } catch (SimulationException ex) { | ||
| log.error("Elevator Simulation error: {}", ex.getMessage()); |
There was a problem hiding this comment.
should append the exception itself as last param log.error(..., e)
| // public static final String LOG_ELEVATOR_DOESNT_MOVE_NO_PENDING_FLOORS = | ||
| // "elevator {} doesn't move, no pending floors"; |
| public static final String LOG_ELEVATOR_MOVING_FROM_TO = | ||
| "elevator {} moving {} from {} to {} (target: {})"; |
There was a problem hiding this comment.
pattern-strings should have sth like PATTERN in the name so its clear that they still need a formatter
| public static void printSummary(Simulation simulation) { | ||
| View view = simulation.getView(); | ||
| view.printSummary(); | ||
| } | ||
|
|
||
| public static void prettyPrint(Simulation simulation) { | ||
| View view = simulation.getView(); | ||
| view.prettyPrint(); | ||
| } |
There was a problem hiding this comment.
this feels super obsolete and doesnt really save anyone anything.
simulation.prettyPrint() versus SimulationUtils.prettyPrint(simulation)
| // log.info( | ||
| // "creating random simulation: elevatorsCount {}, humansCount {}, floorsServed {}", | ||
| // amountOfElevators, | ||
| // amountOfHumans, | ||
| // floorsServed); |
| public void onElevatorSystemReady(FloorPanelSystem floorPanelSystem) { | ||
|
|
||
| final int start = this.startingFloor; | ||
|
|
||
| log.info("checking human at floor {}, state", start); | ||
|
|
||
| final int destination = this.destinationFloor; | ||
|
|
||
| if (start == destination) { | ||
|
|
||
| final HumanState state = HumanState.ARRIVED; | ||
|
|
||
| log.info("starting floor is the same as destination floor => marking state as {}", state); | ||
|
|
||
| this.currentState = state; | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| TravelDirection direction = start < destination ? TravelDirection.UP : TravelDirection.DOWN; | ||
|
|
||
| log.info("computed direction for human at floor {} is {}", start, direction); | ||
|
|
||
| floorPanelSystem.requestElevator(start, direction); | ||
|
|
||
| HumanState newState = HumanState.WAITING_FOR_ELEVATOR; | ||
|
|
||
| log.info("updating human at floor {} state from {} to {}", start, this.currentState, newState); | ||
|
|
||
| this.currentState = newState; | ||
|
|
||
| log.info(" human at floor {} state updated", start); | ||
|
|
||
| log.info( | ||
| "human startin from floor {}, requesting elevator towards floor {}, direction {}", | ||
| start, | ||
| destination, | ||
| direction); | ||
| } |
There was a problem hiding this comment.
kinda weird style with all these newlines between every single line, makes the code hard to read if there is no proper grouping applied
| HumanState newState = HumanState.WAITING_FOR_ELEVATOR; | ||
|
|
||
| log.info("updating human at floor {} state from {} to {}", start, this.currentState, newState); | ||
|
|
||
| this.currentState = newState; |
There was a problem hiding this comment.
should be tight coupled and not interrupted with a log to prevent bugs
all those extra variables are also sorta obsolete
|
|
||
| this.currentState = newState; | ||
|
|
||
| log.info(" human at floor {} state updated", start); |
There was a problem hiding this comment.
these logs shouldnt be INFO but DEBUG level
| // log.debug( | ||
| // "human at floor {}, (state: {}) received elevator {} arrival at floor {}", | ||
| // this.startingFloor, | ||
| // this.currentState, | ||
| // elevatorId, | ||
| // elevatorCurrentFloor); |
There was a problem hiding this comment.
dangling logs (also elsewhere in the codebase) instead of making proper use of logger levels
| @Getter private final int minFloor; | ||
| @Getter private final int floorsServed; | ||
| private int currentFloor; | ||
| private final Set<Integer> pendingFloors = new LinkedHashSet<>(); |
There was a problem hiding this comment.
good pick for a data structure
| // destinationFloor); | ||
|
|
||
| if (destinationFloor < this.minFloor || destinationFloor >= this.minFloor + this.floorsServed) { | ||
| throw new SimulationException("destination outof range for elevator " + this.id); |
There was a problem hiding this comment.
sorta wrong pick for an exception. elevators dont necessarily deal with simulations
| if (!this.pendingFloors.contains(destinationFloor)) { | ||
|
|
||
| this.pendingFloors.add(destinationFloor); |
There was a problem hiding this comment.
could be simplified to if (!pendingFloors.add(destinationFloor)) as attempting to add on a Set already tells whether it could be added or not
| if (!this.pendingFloors.isEmpty()) { | ||
|
|
||
| this.activeTarget = this.pendingFloors.iterator().next(); | ||
|
|
||
| // log.debug( | ||
| // "Elevator {}, has no active target, selecting next pending floor: {}", | ||
| // this.id, | ||
| // this.activeTarget); | ||
|
|
||
| } else { | ||
| // log.debug("elevator {} is idle (no pending floors)", this.id); | ||
| return; | ||
| } |
There was a problem hiding this comment.
difficult to read. early-return would be better:
if (pendingFloors.isEmpty()) return;
...
| if (!this.pendingFloors.isEmpty()) { | ||
| this.activeTarget = this.pendingFloors.iterator().next(); | ||
| log.debug("elvator {} selecting next target floor: {}", this.id, this.activeTarget); |
There was a problem hiding this comment.
this "lets pick the next target" logic is duplicated. maybe better for an extra well named helper method such as
private OptionalInt pickNextTarget() {...}or
private void selectNextActiveTarget() {...}
| this.currentFloor++; | ||
|
|
||
| log.debug( | ||
| SimulationUtils.LOG_ELEVATOR_MOVING_FROM_TO, |
There was a problem hiding this comment.
simulation is from another package and doesnt necessarily deal with elevators, its the wrong util class for using here, design-wise
|
|
||
| Elevator target = staging.getFirst(); | ||
|
|
||
| int distanceFlag = Math.abs(atFloor - target.getCurrentFloor()); |
There was a problem hiding this comment.
its not a flag, confusing name
| for (int i = 1; i < staging.size(); i++) { | ||
|
|
||
| Elevator elevator = staging.get(i); |
There was a problem hiding this comment.
why not enhanced for loop as also used elsewhere in the code?
Zabuzard
left a comment
Zabuzard
left a comment
There was a problem hiding this comment.
working solution, the logic is overall a bit weak but totally works:
- elevator system assigns closest elevator (based on distance, not travel dir)
- humans enter/exit whenever elevator is at desired floor
- elevators keep a set of floor requests that they process one by one. the choice of
LinkedHashSetfor this is good and the implementation makes sure to clear any subsequent requests of floors visited meanwhile, so no "unecessary movement". but there is no sense of priority or similar. when a request queue would look like (4, 1, 5) then the elevator would move to floor 4, then to 1 and then to 5 instead of maybe first going from 4 to 5 and then to 1. - code quality: tried to add some more biz/corp style by changing packages, auto-formatting everything with a different style, adding lombok for a few
@Getter, adding logger, aSimulationUtilsclass and the like. in general, the direction is okay. but its mostly executed a bit poor. for example the logger isnt used properly, no log levels, no proper log configuration. or theSimulationUtilsis used in places where the util is inappropriate design-wise, some of the util methods in there are questionable etc
Submission by user B