19
\$\begingroup\$

In light of our current I decided to build an Elevator Management System.

Initially I intended to program the EMS like a real-time operating system and the elevators as finite-state machine threads with each their own handler. It didn't go according to plan, but it works. For some values of works, since my current infrastructure makes writing tests quite hard.

Thread safety is quite an issue here. I'm grasping this opportunity to learn a thing or two about multi-threading and best practices surrounding that topic.

There's also a generalization problem. As in, there's way too much copy-paste going on. I would fix that, but since I think the architectural flaws are a much bigger problem than copied code.

This project is not finished. However, I think the current state is perfectly reviewable since it's doing what it's supposed to do.

Smaller issues:

  • broadcast_event is global while it should be a member of Bank. Putting it in there bricked the function, probably due to another design flaw.
  • I'm using sys.stdout.write instead of print because print is not thread-safe. However, this feels like an ugly hack.
  • I should probably split up the code to split the module from the invocations. This is planned after the broadcast_event issue mentioned earlier is fixed.
  • The elevator.target attribute is unused. This should be the final destination of the elevator before it turns IDLE again, while still stopping at all floors in between.
  • The elevators are unfamiliar with the concept of doors. Same goes for time and size.
  • The message handling of the elevators should probably be split from the actual movement.
  • No documentation yet. This is a known problem and will be addressed in the next version.

import sys
import threading
import Queue
import time

banks_mail = []
elevators_mail = []
elevators = []
floors_mail = []


class Bank(threading.Thread):
    def __init__(self):
        threading.Thread.__init__(self)
        self.mailbox = Queue.Queue()
        banks_mail.append(self.mailbox)

    def run(self):
        while True:
            data = self.mailbox.get()
            sys.stdout.write(str(self) + ' received: ' + str(data) + '\n')
            if data == 'shutdown':
                sys.stdout.write(str(self) + ' shutting down' + '\n')
                return
            elif data[1] == 'UP':
                for idx, elevator in enumerate(elevators):
                    if elevator.floor <= data[0] and (elevator.direction == 'UP' or elevator.direction == 'IDLE'):
                        sys.stdout.write('Elevator ' + str(idx) + ' available for floor ' + str(data[0]) + '\n')
                        elevators_mail[idx].put(data)
                        break
            elif data[1] == 'DOWN':
                for idx, elevator in enumerate(elevators):
                    if elevator.floor >= data[0] and (elevator.direction == 'DOWN' or elevator.direction == 'IDLE'):
                        sys.stdout.write('Elevator ' + str(idx) + ' available for floor ' + str(data[0]) + '\n')
                        elevators_mail[idx].put(data)
                        break
            else:
                sys.stdout.write('Invalid direction received')

    def stop(self):
        banks_mail.remove(self.mailbox)
        self.mailbox.put('shutdown')
        self.join()


class Elevator(threading.Thread):
    def __init__(self, floor=0, direction='IDLE'):
        threading.Thread.__init__(self)
        self.mailbox = Queue.Queue()
        elevators_mail.append(self.mailbox)
        elevators.append(self)
        self.floor = floor
        self.direction = direction
        self.target = self.floor

    def run(self):
        while True:
            data = self.mailbox.get()
            sys.stdout.write(str(self) + ' received: ' + str(data) + '\n')
            sys.stdout.write(str(data))
            if data == 'shutdown':
                sys.stdout.write(str(self) + ' shutting down' + '\n')
                return
            elif data[0] == self.floor:
                sys.stdout.write(str(self) + ' already at ' + str(data[0]) + '\n')
                break
            elif data[0] >= self.floor:
                self.direction = 'UP'
                for i in xrange(self.floor, data[0]):
                    sys.stdout.write(str(self.floor) + ' from/to ' + str(data[0]) + '\n')
                    self.floor += 1
                    sys.stdout.write(str(self) + ' at ' + str(self.floor) + ' going up\n')
                self.direction = 'IDLE'
            elif data[0] <= self.floor:
                self.direction = 'DOWN'
                for i in xrange(data[0], self.floor):
                    sys.stdout.write(str(self.floor) + ' from/to ' + str(data[0]) + '\n')
                    self.floor -= 1
                    sys.stdout.write(str(self) + ' at ' + str(self.floor) + ' going down' + '\n')
                self.direction = 'IDLE'

    def stop(self):
        elevators_mail.remove(self.mailbox)
        self.mailbox.put('shutdown')
        self.join()


class Floor(threading.Thread):
    def __init__(self, number=0):
        threading.Thread.__init__(self)
        self.mailbox = Queue.Queue()
        floors_mail.append(self.mailbox)
        self.number = number

    def run(self):
        while True:
            data = self.mailbox.get()
            if data == 'shutdown':
                sys.stdout.write(str(self) + ' shutting down' + '\n')
                return
            sys.stdout.write(str(self) + ' received: ' + str(data) + '\n')

    def stop(self):
        floors_mail.remove(self.mailbox)
        self.mailbox.put('shutdown')
        self.join()

    def call(self, data):
        banks_mail[0].put((self.number, data))


def broadcast_event(data):
    for q in elevators_mail:
        q.put(data)

b0 = Bank()
b0.start()

t1 = Elevator()
t2 = Elevator()
t3 = Elevator(8, 'DOWN')
t1.start()
t2.start()
t3.start()

f0 = Floor(0)
f1 = Floor(1)
f2 = Floor(2)
f3 = Floor(3)
f4 = Floor(4)
f5 = Floor(5)
f6 = Floor(6)
f7 = Floor(7)
f8 = Floor(8)

f0.start()
f1.start()
f2.start()
f3.start()
f4.start()
f5.start()
f6.start()
f7.start()
f8.start()

f1.call('UP')
f4.call('UP')
f6.call('DOWN')

time.sleep(0.5) # Make sure all other calls are finished before shutdown

sys.stdout.write('\nRecall all elevators back to Floor 0\n\n')

broadcast_event((0, 'IDLE'))

f0.stop()
f1.stop()
f2.stop()
f3.stop()
f4.stop()
f5.stop()
f6.stop()
f7.stop()
f8.stop()

t1.stop()
t2.stop()
t3.stop()

b0.stop()
\$\endgroup\$
  • 3
    \$\begingroup\$ To quote RubberDuck: So, the elevator challenge is a lot harder than I expected. \$\endgroup\$ – Mast Sep 27 '15 at 16:53
17
\$\begingroup\$

There's also a generalization problem.

I agree! Whenever you start writing things like:

f0 = Floor(0)
f1 = Floor(1)
f2 = Floor(2)
f3 = Floor(3)
f4 = Floor(4)
f5 = Floor(5)
f6 = Floor(6)
f7 = Floor(7)
f8 = Floor(8)

stop immediately, and consider using lists and/or dictionaries instead:

floors = [Floor(i) for i in range(9)]

This now makes subsequent repetition easier to factor out, e.g.:

for floor in floors:
    floor.start()

and if you add a new Floor, you don't have to change much of the code. It's a bit trickier with the elevators, because some of them use default arguments, but you could do something like:

elevators = [Elevator(*args) for args in ((), (), (8, 'DOWN'))]

You can do something similar to replace:

f1.call('UP')
f4.call('UP')
f6.call('DOWN')

with e.g.

for floor, direction in ((1, 'UP'), (4, 'UP'), (6, 'DOWN')):
    floors[floor].call(direction)

One clear structural problem is the global variables. Imagine adding another Bank to the script - what happens then? Clearly elevators in two different banks cannot serve the same floors, which suggests that e.g. the list of elevators should live within the Bank instance in which the elevators sit.


I would also be tempted to allow a more direct sending of messages. Rather than:

elevators_mail[idx].put(data)

why not simply:

elevator.call(data)

Each Elevator instance then exposes its own mailbox via appropriate methods, rather than having a separate structure storing them.

\$\endgroup\$
  • \$\begingroup\$ Looks like I overlooked quite some things which are relatively easy to implement and still have a major impact. Thanks! \$\endgroup\$ – Mast Sep 27 '15 at 17:42

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.