Horrendous Mess™
We are making a Horrendous Mess™, and we’re glad of it.
In the grand scheme of things, we are engaged in shifting responsibility from the Cell class to DungeonLayout, or, if appropriate, to other classes known to the Layout. At the end of that process, we envision a much simpler Cell, perhaps little more than a coordinate or address. It is possible that at that point, we’ll feel that there is too much responsibility in the Layout. Even if that’s true, it’ll be better than having those responsibilities in Cell, because with those things in Cell, an individual Cell knows too much. In particular, it knows its containers, and that always feels icky, and often leads to trouble when things have more than one container.
A Cell can be in a Layout, a Room, and a Suite, all at the same time. It’s a miracle that we don’t already have trouble.
The question facing me this morning is what to do next. I’m guessing it’s going to be related to the neighbor concept, but what we’ll do is look at Cell and see what’s what. So long as it contains any references to the current Layout, we’re not done.
Sure enough there’s this one:
class Cell:
def at_offset(self, dx, dy):
from dungeonlayout import current
return current.at_offset(self.xy, (dx, dy))
My initial reaction to this is concern, because this is the method that enables a cell to access its neighboring cells, and I suspect it is in use a lot. So we’ll look to its users and see what we can see.
class Cell:
def _border_at(self, x, y):
neighbor = self.at_offset(x, y)
if neighbor is None or neighbor.room is None:
return "wall"
elif neighbor.room == self.room:
return "open"
else:
return "door"
One laughs. This is a display method that is used to decide how to draw the borders between suites. This one should be moved off to DungeonView or RoomView or somewhere like that. What else?
A surprise!
Right next to that method I find this one:
class Cell:
def furthest_cells(self):
maximizer = MaximizingMachine()
for cell, distance in (Flooder(layout=None, origin=self)
.in_any_room()
.flood()):
maximizer.process(cell, distance)
return [cd.cell for cd in maximizer.items]
I just happened to notice the layout=None in that call to Flooder. I thought yesterday that removing the shim in Flooder meant that we always had a layout and that Flooder was fully disconnected from Cell. That turns out not to be the case.
Well. Did I realize yesterday that Flooder was not done? I don’t think I did, or if I did I’d forgotten it by morning. Anyway, we may wish to finish Flooder, or we may wish to work on Cell first. Let’s at least divert long enough to see what Flooder is doing with its Cells.
I think these are the only troubling references to Cell in Flooder:
class Flooder:
def _get_neighbors(self, next_cell):
if random.random() < self._randomness:
return next_cell.random_neighbors
return next_cell.neighbors
Do we have corresponding methods in Layout?
class DungeonLayout:
def neighbors(self, cell):
neighbors = []
for xy in self.deltas:
if n := self.at_offset(cell.xy, xy):
neighbors.append(n)
return neighbors
We do not have random_neighbors. Let’s provide that, it’s easy enough. I’m going to do this without a special test as I expect existing tests to hit it.
class DungeonLayout:
def random_neighbors(self, cell):
neighbors = self.neighbors(cell)
return random.sample(neighbors, len(neighbors))
Now in Flooder, I plan to just break things, like this:
class Flooder:
def _get_neighbors(self, next_cell):
if random.random() < self._randomness:
return self._layout.random_neighbor(next_cell)
return self._layout.neighbors(next_cell)
Seven tests break. I expect this to fix at least some:
class Cell:
def furthest_cells(self):
from dungeonlayout import current
maximizer = MaximizingMachine()
for cell, distance in (Flooder(layout=current, origin=self)
.in_any_room()
.flood()):
maximizer.process(cell, distance)
return [cd.cell for cd in maximizer.items]
It only fixes one. This is not as good as I had hoped. I remain hopeful. Look at the broken tests.
Ah, I misnamed the method in Layout, should be random_neighbor, singular.
class DungeonLayout:
def random_neighbor(self, cell):
neighbors = self.neighbors(cell)
return random.sample(neighbors, 1)
Clumsy. Tempted to revert, but only 4 more tests failing. No. I don’t know why they’re not working. This isn’t working. So tempted to debug. Instead, revert and regroup.
OK, New Plan
Let’s make sure that Flooder is always getting a layout, and using it. It has a typo in init:
class Flooder:
def __init__(self, *, layout, origin):
self_layout = layout
self._delivered = None
self._to_be_delivered = None
self._origin = origin
self._select = lambda cell: True
self._next_value = lambda cell, value: value + 1
self._initial_value = 0
self._randomness = 0.0
Should have seen that. Didn’t. Put in the dot on that first line. Green, commit: fix Flooder typo to properly set instance variable.
Now look at all the constructor calls:
I believe they are all good except for the one in Cell:
class Cell:
def furthest_cells(self):
maximizer = MaximizingMachine()
for cell, distance in (Flooder(layout=None, origin=self)
.in_any_room()
.flood()):
maximizer.process(cell, distance)
return [cd.cell for cd in maximizer.items]
Clearly we need a method in the Layout for this. Who’s asking? One test, and main. Let’s change the test to ask this of the layout, then change main, then remove the Flooder call from Cell. Should be simple, I’m glad I reverted.
def test_max_distance_cells(self):
layout = DungeonLayout()
layout = DungeonLayout(5, 5)
map = '''
..1..
..1..
11111
..1..
..1..
'''
layout.add_rooms_from_map(map)
given = layout.at(2,2)
max_cells = given.furthest_cells()
assert len(max_cells) == 4
max_xy = [c.xy for c in max_cells]
assert (2,0) in max_xy
assert (2,4) in max_xy
assert (0,2) in max_xy
assert (4,2) in max_xy
Via Wishful Thinking, we’ll ask the layout for the furthest cells:
max_cells = layout.furthest_cells(given)
Exactly this test fails and no other. Much more like it. Implement:
class DungeonLayout:
def furthest_cells(self, origin):
maximizer = MaximizingMachine()
for cell, distance in (Flooder(layout=self, origin=origin)
.in_any_room()
.flood()):
maximizer.process(cell, distance)
return [cd.cell for cd in maximizer.items]
Test passes. Commit: add furthest_cells to DungeonLayout.
Change main and test.
main.py
...
target = layout.at(32, 28)
options = layout.furthest_cells(target)
...
Works. Commit: main uses layout.furthest_cells.
Now presumably we can remove furthest_cells from Cell. Try it. Green. Commit: remove Cell.furthest_cells.
Reflection
We sort of took a hard left there, thinking we were going to do one thing to DungeonLayout and then discovering we needed another. The changes went smoothly, but I think a short interval of rest and reflection makes sense before shifting context back to chasing the use of current out of Cell.
OK, I think I’m settled. Let’s look, perhaps a bit more carefully, and see whether Cell.at_offset is what we should go after now.
Chasing at_offset
Here’s our current sole reference to current and its users:
class Cell:
def at_offset(self, dx, dy):
from dungeonlayout import current
return current.at_offset(self.xy, (dx, dy))
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
def _border_at(self, x, y):
neighbor = self.at_offset(x, y)
if neighbor is None or neighbor.room is None:
return "wall"
elif neighbor.room == self.room:
return "open"
else:
return "door"
Oh, right, that. Who is using border_at?
class Cell:
def borders(self):
result = dict()
for k,v in {'n':(0,-1), 's':(0,1), 'e':(1,0), 'w':(-1,0)}.items():
result[k] = self._border_at(*v)
return result
OK, fine, fine, who’s using THAT? Just one test and a use in RoomView. Here’s the test:
def test_border_characterization(self):
layout = DungeonLayout(5, 5)
cell = layout.at(3,3)
cell.room = 1
cell1 = layout.at(2,3)
cell1.room = 1
cell2 = layout.at(4,3)
cell2.room = 2
c_33 = layout.at(3,3)
borders = c_33.borders()
assert len(borders) == 4
assert borders["n"] == "wall"
assert borders["e"] == "door"
assert borders["w"] == "open"
assert borders["s"] == "wall"
Clearly this needs to be put on the DungeonLayout. We can make it work by forwarding, for the nonce:
borders = layout.borders(c_33)
class DungeonLayout:
def borders(self, cell):
return cell.borders()
OK, now bring over the real methods and make them work.
class DungeonLayout:
def borders(self, cell):
result = dict()
for k,v in {'n':(0,-1), 's':(0,1), 'e':(1,0), 'w':(-1,0)}.items():
result[k] = self._border_at(cell, v)
return result
def _border_at(self, cell, offset):
neighbor = self.at_offset(cell.xy, offset)
if neighbor is None or neighbor.room is None:
return "wall"
elif neighbor.room == cell.room:
return "open"
else:
return "door"
Green. Commit: borders implemented on DungeonLayout.
That was a bi more tricky than I’ve shown, because I forgot to say cell.room in _border_at, and that eluded me for a moment.
Now we need to get the RoomView to use the layout. We’ll need to send it one.
That got slightly weird for a moment. The views have a Dungeon instance, not a layout. But a Dungeon has a layout, so we’re good:
class RoomView:
class RoomView:
def __init__(self, dungeon, room):
self.dungeon = dungeon
self.layout = dungeon.layout
self.room = room
def draw_border(self, cell, my_cells, screen):
borders = self.layout.borders(cell)
colors = {"open":"black", "door":"green", "wall":"red"}
directions = {'n':(0,-1), 's':(0,1), 'e':(1,0), 'w':(-1,0)}
lines = self.make_lines(cell)
for direction, neighbor_offset in directions.items():
color = colors[borders[direction]]
self.line(*lines[neighbor_offset], color, screen)
To make a test run, I had to add a useless layout member to the FakeDungeon test double that was used to test the keyboard handlers. No big deal, it just had to be there for the init, never got used.
Green, and main works. Let’s remove the borders methods from Cell. Still good. Commit: move borders to DungeonLayout.
I think we’re at a good session-ending point. We are left with this use of current in Cell:
class Cell:
def at_offset(self, dx, dy):
from dungeonlayout import current
return current.at_offset(self.xy, (dx, dy))
We’ve removed the one irritating use by moving the borders code. We have the moving code and the east west stuff to move, which we’ll do another time.
Summary
The move of material from Cell to Layout continues to go well. Occasional bumps in the road, and sometimes we reverted a bit, and sometimes we just moved forward and fixed things up. We have made good use of Wishful Thinking, where we change code to call a method that doesn’t exist, observe the failures, then implement the method.
Sometimes we implemented the method first by forwarding to the old receiver (Cell) and then moved the code over. Sometimes we just let it fail and then moved the code without the forwarding step. In retrospect, I think the forwarding move is better. It’s basically trivial so we just call the missing method, implement it trivially, and then refactor. It’s one more step but we can actually commit the forwarding version, notching in just a bit of progress. Feels better to me.
The worst part of all this is that while Cell is becoming simpler and more cohesive, DungeonLayout is becoming what we professionals refer to as a Horrendous Mess™. It’s perilously close to 200 lines now, and the only reason I can deal with it at all is that PyCharm is really good at taking me to the method I’m looking for, because the methods seem to be in random order.
We have some cleanup to do there. Might make sense to start now, but I feel that we are probably just one more move away from having Cell within tolerance, so I suspect we’ll wait, or maybe do a few small things as we notice them.
The thing is, though, that although now DungeonLayout is getting pretty heavily burdened, it is with behavior that belongs there, or in simple classes that work for it, as we may discover when we start improving it. Those classes will be justly referring to things that they know, where Cell was really dealing with things it shouldn’t know, like the way it had of dealing with aggregates of Cells, which is really not what an instance ought to be doing.
So it’s a better Horrendous Mess than we had, and we are pleased with how it’s going.
See you next time!