8
\$\begingroup\$

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...
<etc.>
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 being implemented to simulate the time passing of an actual elevator moving. But again doesn't have much value without multithreading applied.

from time import sleep

class Elevator():

    def __init__(self, num, floor=0):
        """Takes current floor as a parameter."""

        self.floor = floor
        self.num = num
        self.wear = 0
        self.direction = None

    def __str__(self):
        return "Elevator {} on floor {}".format(self.num, self.floor)

    def __repr__(self):
        return "<Elevator object {} currently on floor {}>".format(
                                            self.num, self.floor)

    def open(self):
        print ("Doors opening on elevator {}.".format(self.num))

    def close(self):
        print ("Doors closing on elevator {}.".format(self.num))

    def move(self, system):
        """Takes an ElevatorSystem and moves one floor in self.direction."""

        system.floors[self.floor].remove(self)
        self.wear += 1
        self.floor += self.direction
        system.floors[self.floor].append(self)

        print("On floor {}...".format(self.floor))

    def deactivate(self, system, warning=True):
        """Takes Elevator System to remove self from."""

        system.floors[self.floor].remove(self)
        if warning:
            print ("Elevator {} needs servicing. It is now deactivated".
                                                            format(self))

    def repair(self, system=None):
        """Resets wear optionally takes system to reattach to.

        If the elevator was broken, pass in an ElevatorSystem that
        the elevator should reattach itself to.
        For normal maintenance, this isn't required."""

        self.wear = 0
        if system is not None:
            system.floors[self.floor].append(self)
            print ("Elevator {} has been repaired.".format(self))


class ElevatorSystem():
    """A system for managing a building's elevators.

    Takes floor and elevator count as parameters.
    init will create the necessary Elevator objects.
    Active elevators are held on floors and accessed there."""

    #Constants to determine how in need of repair elevators are
    WEAR_WARNING = 4000
    WEAR_THRESHOLD = 5000

    def __init__(self, floor_count=3, elevator_count=2):
        if floor_count < 2 or elevator_count < 2:
            raise (ValueError, "Invalid parameters"
                   "Floor and elevator counts should be greater than 1.")

        self.elevators = [Elevator(i) for i in range(elevator_count)]
        self.floors = [[] for _ in range(floor_count)]
        self.floors[0] = self.elevators[:]
        self.deactivated = []

    def __repr__(self):
        return ("<ElevatorSystem object containing {} floors and {} elevators>"
                            .format(len(self.floors), len(self.elevators)))

    def __str__(self):
        return ("Elevator System with {} elevators on floors "
                .format(len(self.elevators)) + 
                ', '.join(str(elevator.floor if elevator.floor else 'G')
                              for elevator in self.elevators))

    def validate_floor(self, floor):
        """Raises a ValueError if floor doesn't exist."""

        if floor >= len(self.floors):
            raise ValueError("Invalid floor {}, system only "
                             "has {} floors.".format(floor, len(self.floors)))

    def choose_elevator(self, elevators, direction):
        """Return elevator that's most suitable.

        Takes in direction to first filter out moving elevators.
        Alternatively filters out elevators moving the wrong direction.
        Lastly will take the least worn elevator as its choice."""

        # First try take static elevators
        chosen_elevators = [elevator for elevator in elevators if
                                elevator.direction is None]

        # Now get elevators travelling in the right direction
        if not chosen_elevators:
            [elevator for elevator in elevators if
                elevator.direction == direction]

        #If that fails too, just take what we've got
        if not chosen_elevators:
            chosen_elevators = elevators

        #Sort by wear and take the first ie. lowest value in the sorted list
        return sorted(chosen_elevators, key=lambda ele: ele.wear)[0]


    def call_elevator(self, floor, direction):
        """Chooses and returns an elevator after moving it to floor.

        Floor should be the floor the elevator is called to.
        Direction should indicate which way the user wants to travel.
        Valid values are 1 or -1."""

        self.validate_floor(floor)
        if direction not in (1, -1):
            raise ValueError("direction only accepts 1 or -1, not {}".
                                                        format(direction))

        #Check if there's any elevators on the current floor
        if self.floors[floor]:
            elevator = self.choose_elevator(self.floors[floor], direction)
            elevator.open()
            return elevator

        #Now search outward, one floor up and down at a time.
        #None pads the list if we run out of floors in one direction
        check_floors = map(None, range(floor + 1, len(self.floors)),
                                 range(floor - 1, -1, -1))
        for up, down in check_floors:
            if down is not None and self.floors[down]:
                elevator = self.choose_elevator(self.floors[down], direction)
                break
            if up is not None and self.floors[up]:
                elevator = self.choose_elevator(self.floors[up], direction)
                break
        else:
            raise Exception("Cannot find elevators.")

        self.move_elevator(elevator, floor)
        return elevator       

    def move_elevator(self, elevator, floor):
        """Send elevator to floor, moving one floor at a time."""

        self.validate_floor(floor)
        print("Elevator {} moving".format(elevator.num))

        move_up = elevator.floor < floor
        elevator.direction = 1 if move_up else -1

        # Build the range based on which direction we need to go
        # We do this because range won't work if floor < elevator.floor
        for _ in (range(elevator.floor, floor) if move_up
                            else range(elevator.floor, floor, -1)):
            elevator.move(self)
            sleep(0.5)

        elevator.open()
        elevator.direction = None

    def maintenance_check(self, warnings=True):
        """Update list of deactivated elevators

        Optionally warns about elevators that need maintenance now or soon.
        Pass False to suppress printing these warnings."""

        for floor in self.floors:
            for elevator in floor:
                wear = elevator.wear
                if wear > self.WEAR_THRESHOLD:
                    self.deactivated.append(elevator)
                    elevator.deactivate(self, warnings)
                elif warnings and wear > self.WEAR_WARNING:
                    print ("{} has wear of {} and should be serviced soon".
                                format(elevator, elevator.wear))

    def repair_elevators(self, maintenance=False):
        """Repair or optionally service broken and worn elevators.

        maintenance determines whether or not to repair any elevator with
        significant wear or just to repair deactivated ones."""

        for elevator in self.deactivated:
            elevator.repair(self)

        if maintenance:
            for floor in self.floors:
                for elevator in floor:
                    if elevator.wear > self.WEAR_WARNING:
                        elevator.repair()
\$\endgroup\$
6
\$\begingroup\$

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.
    <Elevator object 0 currently on floor 2>
    >>> 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.
    <Elevator object 0 currently on floor -2>
    

    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.
    <Elevator object 1 currently on floor 0>
    >>> es.call_elevator(3, 1)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      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.
    <Elevator object 2 currently on floor 1>
    >>> es.call_elevator(1, -1)
    Doors opening on elevator 2.
    <Elevator object 2 currently on floor 1>
    

    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.

\$\endgroup\$
  • \$\begingroup\$ Great catch about negative floors. I hadn't even thought about the error let alone implementing basement floors, thanks! About "In the Elevator.deactivate() method, you're dropping in self instead of self.num to identify the elevator." that's intentional. I'm passing the ElevatorSystem's self to the Elevator so that the Elevator can remove itself from the specified ES. This is related to your point about having the elevators leave the system alone. \$\endgroup\$ – SuperBiasedMan Sep 11 '15 at 15:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.