Design Feedback
The FGNO OGs1] gave me some good ideas about my design: thanks, OGs. This morning I’ll start following some of those ideas. I use test doubles!
Every Tuesday evening for the past few years, has been FGNO, Friday Geeks Night Out2 via Zoom. Last night I asked for some feedback about the design of the dungeon program. OG A challenged me to do a refactoring that I mentioned was probably desirable. Did that, with far more typing errors than I ever seem to have when alone. Worked well. OG B helped with some pygame issues. I suggested that Dungeon should be split into the stable bits, and a DungeonContents new class instance containing the variable parts. Actually made that work.
I think it was OG H who suggested that the view side should do things like the decoding of key strokes, and send messages to the dungeon that make semantic sense to dungeon behavior. We didn’t try that, but it has made it to my list of things to do.
Then OG H, on a bit of a roll, allowed as how he’d do the contents/layout design with the dynamic part of the dungeon holding on to the more static part rather than the way I proposed with the top object holding both static and dynamic contents. We talked about that and I think that I probably agree, so this morning we’re going to work on that.
Let’s get started. Here’s the top of Dungeon now:
class Dungeon:
def __init__(self):
self.adventurer_cell = None
self.key_lock = None
# layout starts here
self.rooms = []
self.suite_sets = []
The arrangement of members and the comment are from FGNO. The last two instance variables are unchanging after the rooms are created. One name that we came up with for the new class was DungeonLayout, thus the comment.
Our target design here is to have a new class, DungeonLayout, that contains the rooms and suites. The class Dungeon will include the dynamic aspects of the game. (OG B-prime commented that this class would often be called Game.)
We would like to do this refactoring in small steps, with plenty of commits along the way. There are quite a few references to Dungeon: 19, according to PyCharm. Eight of those are in main, and the rest in tests. A quick look tells me that in main, there is only the import, one real reference, and the rest are type hints. The tests all look similar:
def test_make_suite_lists(self):
Cell.create_space(10, 10)
dungeon = Dungeon()
o_0 = Cell.at(3,1)
r_0 = RoomMaker().cave(number_of_cells=1, origin=o_0, name='0')
dungeon.add_room(r_0)
...
To a first approximation they’re all creating a Dungeon to put rooms or paths into it. So I think they’ll all want to refer to DungeonLayout when we’re done.
So I suggest the following plan:
- Rename Dungeon to DungeonLayout. This will change all the references and keep the tests running. Commit that.
- Then, create a new Dungeon class, via at least one test, and move the
populateand such code to it. Maybe write a test for that. - Waving hands: deal with DungeonView and its use in
main.
I’m pretty sure about that first step, and think that the second is probably the right next thing to do, but we’ll look a bit more deeply at the code before we pick the next move. I’m sure this first one is proper, as it will fix up most of the wiring with a machine refactoring.
Here goes: one rename command. PyCharm even offers to rename the file. Green. Commit: rename Dungeon to DungeonLayout.
OK. At this point everything works, including main. We will work mostly driven by main and see what we find. main looks like this:
def main():
Cell.create_space(64, 56)
# random.seed(234)
layout = DungeonLayout()
maker = RoomMaker()
make_diamond_in_round_room(layout, maker)
make_a_diamond_room(layout, maker)
make_a_round_room(layout, maker)
number_of_rooms = random.randint(8, 8)
for _ in range(number_of_rooms):
make_a_cave_room(layout, maker)
make_experimental_room(layout, maker)
layout.ensure_connected()
layout.populate()
view = DungeonView(layout)
view.main_loop()
That’s nearly good but it should end like this:
dungeon = Dungeon(layout)
dungeon.populate()
view = DungeonView(dungeon)
view.main_loop()
Ideally, that would be all we need to do in main, at least for now. We could argue that we shouldn’t have to ensure connected explicitly, and that perhaps the Dungeon should auto-populate. But perhaps not: there are probably all kinds of options to be specified. Anyway, given our current code, that’s about as good as it could get.
Let’s create a test that works somewhat like main, to drive out some changes.
class TestDungeon:
def test_exists(self):
Cell.create_space(10, 10)
room_cell = Cell.at(5, 5)
room = Room([room_cell])
layout = DungeonLayout()
layout.add_room(room)
dungeon = Dungeon(layout)
That’s a lot of wiring but I wanted a legitimate layout. This won’t compile or run because there is no Dungeon class. As is my common practice, I’ll begin by putting it in DungeonLayout’s file.
class Dungeon:
def __init__(self, layout):
self.layout = layout
Green. We can commit.
Let’s write a new test for populate and see what we can do.
def test_populate(self):
Cell.create_space(10, 10)
room_cell = Cell.at(5, 5)
room = Room([room_cell])
layout = DungeonLayout()
layout.add_room(room)
dungeon = Dungeon(layout)
dungeon.populate()
assert dungeon.adventurer_cell is None
assert dungeon.key_lock is None
These are the current population members in DungeonLayout, so presumably theyu’ll be moved to Dungeon … right now:
class Dungeon:
def __init__(self, layout):
self.layout = layout
self.adventurer_cell = None
self.key_lock = None
def populate(self):
dot_room = random.choice(self.layout.rooms)
dot_cell = random.choice(dot_room.cells)
self.adventurer_cell = dot_cell
I expected that to work. It did not. The test is wrong:
...
dungeon.populate()
assert dungeon.adventurer_cell is room_cell
assert dungeon.key_lock is None
Green. Commit: implementing Dungeon class.
Breathe. Reflect. Think.
Right. We need to make DungeonView work, for main to work. Most of what needs fixing is here:
class DungeonView:
def main_loop(self):
running = True
moving = False
background = "gray33"
color = "darkblue"
while running:
for event in pygame.event.get():
if event.type == pygame.QUIT:
running = False
elif event.type == pygame.KEYDOWN:
self.dungeon.handle_keydown(event)
elif event.type == pygame.KEYUP:
self.dungeon.handle_keyup(event)
self.screen.fill(background)
# self.draw_grid(color)
self.draw_rooms()
self.draw_contents()
self.clock.tick(60)
pygame.display.flip()
pygame.quit()
def draw_rooms(self):
for room in self.dungeon:
RoomView(room).draw(self.screen)
def draw_contents(self):
adventurer_cell = self.dungeon.adventurer_cell
cx = adventurer_cell.x*cell_size + cell_size//2
cy = adventurer_cell.y*cell_size + cell_size//2
pygame.draw.circle(self.screen, "red", [cx, cy], cell_size//2)
Dungeon does not understand in, and I don’t think we want it to. I do think it could understand rooms. So:
class DungeonView:
def draw_rooms(self):
for room in self.dungeon.rooms:
RoomView(room).draw(self.screen)
class Dungeon:
def __init__(self, layout):
self.layout = layout
self.adventurer_cell = None
self.key_lock = None
@property
def rooms(self):
return self.layout.rooms
def populate(self):
dot_room = random.choice(self.rooms)
dot_cell = random.choice(dot_room.cells)
self.adventurer_cell = dot_cell
I didn’t write a test for that. Let’s extend our existing one:
def test_populate(self):
Cell.create_space(10, 10)
room_cell = Cell.at(5, 5)
room = Room([room_cell])
layout = DungeonLayout()
layout.add_room(room)
dungeon = Dungeon(layout)
dungeon.populate()
assert dungeon.adventurer_cell is room_cell
assert dungeon.key_lock is None
rooms = dungeon.rooms
assert len(rooms) == 1
assert rooms[0].cells[0] is room_cell
A bit grubby but green. Commit: Dungeon provides rooms. (I have not been committing main, but am committing everything else.)
Let’s see what explodes now when we do run main. Good news, the map comes up. Then if I try to move Dot:
~~~line 26, in main_loop self.dungeon.handle_keydown(event) ^^^^^^^^^^^^^^^^^^^^^^^^^^^ AttributeError: ‘Dungeon’ object has no attribute ‘handle_keydown’
## Reflect. Plan.
OG "H" mentioned making the key handlers part of the View and sending semantic messages to the dungeon to do what the keys call for. This is a good time to do that. It'll be a bit more work but not much.
First we'll test in the semantic messages. I start with this to get a known situation:
~~~python
def test_adventurer_motion(self):
Cell.create_space(20, 20)
layout = DungeonLayout()
c55 = Cell.at(5, 5)
round = RoomMaker().round(radius=6, origin=c55)
layout.add_room(round)
dungeon = Dungeon(layout)
dungeon.populate()
dungeon.place_adventurer_at(c55)
assert dungeon.adventurer_cell is c55
We’ll need a way to set the adventurer location anyway. I will get tired of typing “adventurer” pretty soon.
class Dungeon:
def place_adventurer_at(self, cell):
self.adventurer_cell = cell
Green. Commit a save point.
Extend the test:
dungeon.move_adventurer_north()
assert dungeon.adventurer_cell is Cell.at(5, 4)
I considered shorter names or passing a string but decided on separate methods at least for now.
def move_adventurer_north(self):
self.adventurer_cell = self.adventurer_cell.north()
We already have the directional moves in Cell:
class Cell:
def east(self):
return self.attempt_move(1,0)
def north(self):
return self.attempt_move(0, -1)
def south(self):
return self.attempt_move(0, 1)
def west(self):
return self.attempt_move(-1, 0)
def attempt_move(self, dx, dy):
cell = self.at_offset(dx, dy)
if not cell or cell.is_available:
return self
return cell
Continue with the other three moves:
dungeon.move_adventurer_north()
assert dungeon.adventurer_cell is Cell.at(5, 4)
dungeon.move_adventurer_east()
assert dungeon.adventurer_cell is Cell.at(6,4)
dungeon.move_adventurer_south()
assert dungeon.adventurer_cell is Cell.at(6, 5)
dungeon.move_adventurer_west()
assert dungeon.adventurer_cell is Cell.at(5,5)
And the corresponding methods of course, just like north. I’ll spare you. Commit: Dungeon supports semantic move.
Reflect. Plan.
I think we “just” need to put key handlers somewhere sensible and make them send the new semantic messages. In DungeonView, we have:
class DungeonView:
elif event.type == pygame.KEYDOWN:
self.dungeon.handle_keydown(event)
elif event.type == pygame.KEYUP:
self.dungeon.handle_keyup(event)
It’s up to the view to decode what the keys mean, so let’s send those messages to self instead of the dungeon, and add a couple of new methods.
Should we write tests for that? Let’s do, because I think I claimed yesterday that I knew how to do it.
class TestHandlers:
def test_keydown(self):
assert False
Fails, so it’s hooked up. I pretty much always try that to be sure my setup is correct.
Hm. I guess the view will handle key locking as well. Interesting.
We’ll create a little test dummy to help with this.
Grr. We’ll want a DungeonView that doesn’t start up pygame. So we’ll do a bit of a hack.
class DungeonView:
def __init__(self, dungeon, testing=False):
self.dungeon = dungeon
if testing:
return
...
I needed two fake objects, though I could probably combine them into one:
class FakeDungeon:
def __init__(self):
self.received = 'nothing'
def move_adventurer_north(self):
self.received = 'north'
def move_adventurer_south(self):
self.received = 'south'
def move_adventurer_east(self):
self.received = 'east'
def move_adventurer_west(self):
self.received = 'west'
class FakeEvent:
def __init__(self, key):
self.key = key
class TestHandlers:
def test_keydown(self):
fake = FakeDungeon()
view = DungeonView(fake, testing=True)
event = FakeEvent(pygame.K_UP)
view.handle_keydown(event)
assert fake.received == 'north'
event = FakeEvent(pygame.K_DOWN)
view.handle_keydown(event)
assert fake.received == 'south'
event = FakeEvent(pygame.K_LEFT)
view.handle_keydown(event)
assert fake.received == 'west'
event = FakeEvent(pygame.K_RIGHT)
view.handle_keydown(event)
assert fake.received == 'east'
My test is failing on south. If I comment out everything but south, south works.
Ah. As intended: I”m handling key locking already!
class TestHandlers:
def test_keydown(self):
fake = FakeDungeon()
view = DungeonView(fake, testing=True)
event = FakeEvent(pygame.K_UP)
view.handle_keydown(event)
assert fake.received == 'north'
view.handle_keyup(event)
event = FakeEvent(pygame.K_DOWN)
view.handle_keydown(event)
assert fake.received == 'south'
view.handle_keyup(event)
event = FakeEvent(pygame.K_LEFT)
view.handle_keydown(event)
assert fake.received == 'west'
view.handle_keyup(event)
event = FakeEvent(pygame.K_RIGHT)
view.handle_keydown(event)
assert fake.received == 'east'
view.handle_keyup(event)
This works, because I moved the full handlers from wherever they were into DungeonView:
class DungeonView:
def handle_keydown(self, event):
if self.key_lock:
return
self.key_lock = event.key
if event.key == pygame.K_RIGHT:
self.dungeon.move_adventurer_east()
elif event.key == pygame.K_LEFT:
self.dungeon.move_adventurer_west()
elif event.key == pygame.K_UP:
self.dungeon.move_adventurer_north()
elif event.key == pygame.K_DOWN:
self.dungeon.move_adventurer_south()
else:
self.key_lock = None
def handle_keyup(self, event):
if event.key == self.key_lock:
self.key_lock = None
I think main should work now. Yes, we can move Dot around. Commit including main: DungeonView supports keydown events.
Now I’d like to move Dungeon to its own file, but I have concerns about the imports. We’ll try it since we are at a point where we can revert. Good news, everything works. Commit: Dungeon moved to own file.
Let’s call it a morning, since the article is getting to be book length.
Summary
The trick of renaming Dungeon to DungeonLayout was an important first move here, because most of the functionality wanted to move there. After that, everything proceeded in small steps.
It is quite unusual for me to build a test double, as we did here with the FakeEvent and FakeDungeon, but I just wanted to be sure that DungeonView sent the right messages, and setting up an entire Dungeon seemed excessive. The doubles tell me what I want to know in this case.
I am from the Detroit school of TDD, which generally does not use test doubles, as opposed to the London School, which uses them extensively. The London focus is mostly on whether objects are sending the right messages, while Detroit focuses more on whether, sent a message, an object does what we expect. I have not worked extensively in London style, and I have always wondered how you ever find out that the messages you correctly send are handled correctly. I’m sure it works: people I respect use the approach. I just haven’t worked that way enough to see where it bottoms out.
It seems likely that there are methods now that are unused. I tried to get rid of the ones I was aware of, but it would behoove us to do a scan of the affected classes to see what can be removed.
As for keeping the layout under the dungeon contents, it certainly works so far, for the one content item we have, good ol’ Dot. We’ll see how it goes in the future but at a guess it’ll in fact be better. With the contents under the layout, all the operational calls from view would have to bounce through Dungeon to DungeonContents, or we’d have to rip the contents out and handle them separately. This way, there’s only one bounce in Dungeon, the one that fetches rooms.
I think I’m going to like it. Bit of a hat tip to OG “H”. See you next time!