HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonModerate

Messed up Elevator Management System

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
managementmessedelevatorsystem

Problem

In light of our current community-challenge 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)
sel

Solution

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.

Code Snippets

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)
floors = [Floor(i) for i in range(9)]
for floor in floors:
    floor.start()
elevators = [Elevator(*args) for args in ((), (), (8, 'DOWN'))]
f1.call('UP')
f4.call('UP')
f6.call('DOWN')

Context

StackExchange Code Review Q#105843, answer score: 17

Revisions (0)

No revisions yet.