patternpythonModerate
Messed up Elevator Management System
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:
```
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
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_eventis global while it should be a member ofBank. Putting it in there bricked the function, probably due to another design flaw.
- I'm using
sys.stdout.writeinstead ofprintbecauseprintis 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_eventissue mentioned earlier is fixed.
- The
elevator.targetattribute is unused. This should be the final destination of the elevator before it turnsIDLEagain, 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:
stop immediately, and consider using lists and/or dictionaries instead:
This now makes subsequent repetition easier to factor out, e.g.:
and if you add a new
You can do something similar to replace:
with e.g.
One clear structural problem is the global variables. Imagine adding another
I would also be tempted to allow a more direct sending of messages. Rather than:
why not simply:
Each
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.