Elevator Management System

So here's my attempt at the September community challenge, making an elevator handling system. I'm not really very familiar with OOP, so I'm particularly looking to get feedback on that part of the script even if you're unfamiliar with Python. I'm fairly certain there's times I've mishandled abstraction and what class should have which method, but that's for you to review.

Basically, my ElevatorSystem is the main class you use. Instantiating it will create all the necessary Elevator objects within it. The Elevator performs some small functions and feedback, but my assumption was that in a real system it would be handling the actual control of the individual elevator, moving floors and opening doors. Most of the time Elevator objects should really just be used as parameters for move_elevator calls and otherwise be left alone.

The ElevatorSystem is where the magic happens. When someone calls for an elevator, it chooses the best one based on floor proximity, then whether the elevator is moving or at least moving in a favourable direction and finally based on how much use the elevator has had. I have a rudimentary wear attribute in place as a means to prevent certain elevators getting disproportionately called.

The basic use would go something like this:

>>> es = ElevatorSystem(40, 5)
>>> ele = es.call_elevator(12, -1)
#Note that the result of call_elevator is assigned to ele for later use
Elevator 0 moving
On floor 1...
On floor 2...

On floor 12...
Doors opening on elevator 0.
>>> es.move_elevator(ele, 9)
#Now that the users have boarded elevator 'ele', pass ele to move_elevator
Elevator 0 moving
On floor 11...
On floor 10...
On floor 9...
Doors opening on elevator 0.

Obviously its use is a bit more limited without setting up some sort of multithreading so that multiple elevators could actually be called at once, but I actually know nothing about that and didn't want to bite off more than I could chew for this challenge. Also sleep is only

I don't have time to do a proper review, but here are some comments based on a quick skim through the code:

-
I don't like the way Elevator is muddling with the internals of the ElevatorSystem. It should be a self-contained class, and all the work of managing the system should be handled in ElevatorSystem.

-
__repr__() should ideally return a string that would give an equal object when passed to eval(); for example:

>>> e = Elevator(num=2, floor=0)
>>> repr(e)
'Elevator(2, 0)'


-
You don't give any explanation for the num parameter in the constructor docstring for Elevator. Something like id_num would clarify that this identifies the elevator as unique.

-
The floor boundaries for ElevatorSystem seem a little off. For example, here I instantiate a system with three floors but I can somehow visit at least five distinct floors:

>>> es = ElevatorSystem(3, 2)
>>> es.call_elevator(2, 1)
Elevator 0 moving
On floor 1...
On floor 2...
Doors opening on elevator 0.

>>> es.call_elevator(-2, 1)
Elevator 0 moving
On floor 1...
On floor 0...
On floor -1...
On floor -2...
Doors opening on elevator 0.



Note also that if there are only three floors and I'm on the second floor, then I can't ask to go up – but there's nothing in ElevatorSystem to highlight the discrepancy.

Trying to get to a floor that's too low returns some confusing output, while trying to get to a floor that's too high is a ValueError:

>>> es.call_elevator(-3, 1)
Doors opening on elevator 1.

>>> es.call_elevator(3, 1)
Traceback (most recent call last):
File "", line 1, in
File "elevator.py", line 126, in call_elevator
self.validate_floor(floor)
File "elevator.py", line 93, in validate_floor
"has {} floors.".format(floor, len(self.floors)))
ValueError: Invalid floor 3, system only has 3 floors.


I think it would be better to define explicit lower and upper boundaries, and check that I don't try to exceed those. The number of floors above and below the ground floor aren't usually symmetric.

-
The open() and close() methods on Elevator don't keep track of whether the doors are already open. This can lead to the confusing case where the doors can be opened twice:

>>> es.call_elevator(1, -1)
Elevator 2 moving
On floor 1...
Doors opening on elevator 2.

>>> es.call_elevator(1, -1)
Doors opening on elevator 2.



I'd suggest adding a doors_open attribute to the Elevator class, which tracks whether the doors are open, and only prints a message about doors opening/closing when it's appropriate.

-
In the Elevator.deactivate() method, you're dropping in self instead of self.num to identify the elevator.

-
This part of choose_elevator doesn't do anything:

if not chosen_elevators:
    [elevator for elevator in elevators if
        elevator.direction == direction]

You filter our elevators that match the given direction, but that result gets discarded.

StackExchange Code Review Q#104242, answer score: 6