I’ve had an obvious but seemingly decent idea. Let’s try it. Here’s my cunning plan …

Class variable holding current DungeonLayer

What we’ll do is provide a class variable on DungeonLayout, current, which we’ll set every time we create a Layout. When we need it in Cell, we’ll fetch it. We’ll work, over time, not to need it, essentially by replacing Cell.whatever by calls to current. That will be the thing we need to let us change out our 112 uses of at one at a time, allowing for incremental refactoring. Of course, in practice, I’ll do them in a few batches, but in Real Life we could take our time.


class DungeonLayout:
    current = None

    def __init__(self, max_x=10, max_y=10):
        DungeonLayout.current = self
        self.cells = dict()
        for x in range(max_x):
            for y in range(max_y):
                self.cells[(x,y)] = Cell(x, y)
        self.rooms = []

That, of course, continues to pass all the tests. Commit: add and update current class member.

Use class variable in Cell

It seems to me that we can probably fix cell to use current from DungeonLayer. But, now that I thin of it, it would be better if it were a global in that file, like this:

class DungeonLayout:

    def __init__(self, max_x=10, max_y=10):
        global current
        current = self
        self.cells = dict()
        for x in range(max_x):
            for y in range(max_y):
                self.cells[(x,y)] = Cell(x, y)
        self.rooms = []
...
current: DungeonLayout = DungeonLayout(1,1)

To test that we have things rewired, this test:

    def test_current(self):
        layout = DungeonLayout(5, 5)
        c_via_layout = layout.at(3,3)
        c_via_cell = Cell.at(3, 3)
        assert c_via_cell is c_via_layout

That’s failing at present. We need to fix Cell, which I think we can do.

class Cell:
    deltas = [(-1, 0), (1, 0), (0, -1), (0, 1)]

    @classmethod
    def create_space(cls, max_x, max_y):
        from dungeonlayout import DungeonLayout
        DungeonLayout(max_x, max_y)

    def __init__(self, x, y):
        self._xy = (x,y)
        self._room = None

    @classmethod
    def at(cls, x, y):
        from dungeonlayout import current
        return current.at(x, y)

    @classmethod
    def unused_cell_or_none(cls):
        from dungeonlayout import current
        return current.unused_cell_or_none()

I’m not entirely happy with the name current. But the tests are green, we can commit: Cell refers to current layout as needed.

Removing the connections

I think we’re moving toward a situation where Cell only understands things about x and y, and Layout handles where the rooms are and other multi-cell aspects of things. So part of that will be refactoring code that now talks to Cell and making it talk to Layout. We’ll just explore around and see what we see. But I think our first move should be to find all the calls to Cell.create_space and replace them with creation of a layout.

I think it’s prudent to start with some tests, but I’d like to have some focus on main, since it is as close as we have come to a full circle dungeon build. But first some tests. Might as well start in TestDungeonLayout, for example:

class TestDungeonLayout:
    def test_adding_rooms(self):
        dungeon = DungeonLayout()
        assert dungeon.number_of_rooms == 0
        Cell.create_space(100, 100)
        size = 10
        origin = Cell.at(32, 28)
        room = RoomMaker().cave(number_of_cells=size, origin=origin)
        dungeon.add_room(room)
        assert dungeon.number_of_rooms == 1

We can recast this, first by renaming dungeon to layout:

class TestDungeonLayout:
    def test_adding_rooms(self):
        layout = DungeonLayout()
        assert layout.number_of_rooms == 0
        Cell.create_space(100, 100)
        size = 10
        origin = Cell.at(32, 28)
        room = RoomMaker().cave(number_of_cells=size, origin=origin)
        layout.add_room(room)
        assert layout.number_of_rooms == 1

Then by just creating the layout with (100, 100):

class TestDungeonLayout:
    def test_adding_rooms(self):
        layout = DungeonLayout(100, 100)
        assert layout.number_of_rooms == 0
        size = 10
        origin = layout.at(32, 28)
        room = RoomMaker().cave(number_of_cells=size, origin=origin)
        layout.add_room(room)
        assert layout.number_of_rooms == 1

That runs, commit: refactoring tests.

Now I am sorely tempted to divert our attention to RoomMaker, which is probably talking to Cell a lot and which could be given a convenient layout instead.

class RoomMaker:
    def __init__(self):
        pass

    def cave(self, *, number_of_cells: int, origin: Cell, name='cave', **kwargs):
        cells = CaveCellCollector().build(number_of_cells=number_of_cells, origin=origin, name=name)
        return Room(cells, name)

class CaveCellCollector:
    def __init__(self):
        self.cells:list[Cell] = []
        self.growth_candidates:list[Cell] = []

    def build(self, *, number_of_cells: int, origin: Cell, name=''):
        self._build_cave(number_of_cells, origin)
        return self.cells

    def _build_cave(self, number_of_cells, origin):
        new_cell: Cell = origin
        for _ in range(number_of_cells):
            self.take(new_cell)
            self.growth_candidates.append(new_cell)
            new_cell = self.find_adjacent_cell()
            if new_cell is None:
                return

    def take(self, new_cell: Cell):
        self.cells.append(new_cell)
        new_cell.room = self

    def find_adjacent_cell(self):
        for cell in sample(self.growth_candidates,
                           len(self.growth_candidates)):
            available = cell.available_neighbors
            if available:
                return choice(available)
            else:
                self.growth_candidates.remove(cell)
        return None

Looking at that, I see, near the bottom cell.avaiable_neighbors, which leads to:

class Cell:
    @property
    def available_neighbors(self):
        return [neighbor
                for neighbor in self.neighbors
                if neighbor.is_available]

    @property
    def neighbors(self):
        return [neighbor
                for dxy in self.deltas
                if (neighbor := self.at_offset(*dxy))]

Hm. I reckon that this calls for a neighbors and available_neighbors method on the Layout. We should anticipate that as we do this long refactoring in small steps, we’ll often encounter a capability that needs to be moved out of Cell and into the Layout, and that we’ll commonly forward from Cell, at least for a while, until everyone is using the new facility.

So let’s add neighbors to the Layout, with a test:

    def test_neighbors(self):
        layout = DungeonLayout(5, 5)
        c33 = layout.at(3, 3)
        neighbors = layout.neighbors(c33)
        assert len(neighbors) == 4

That’s enough to fail. We provide:

class DungeonLayout:
    def neighbors(self, cell):
        return [self.at_offset(cell.xy, xy) for xy in self.deltas]

    def at_offset(self, start, offset):
        new = (start[0]+offset[0], start[1]+offset[1])
        return self.at(*new)

The test passes. The code is rough. Complete the test:

    def test_neighbors(self):
        layout = DungeonLayout(5, 5)
        c33 = layout.at(3, 3)
        neighbors = layout.neighbors(c33)
        assert len(neighbors) == 4
        assert layout.at(2,3) in neighbors
        assert layout.at(4,3) in neighbors
        assert layout.at(3,2) in neighbors
        assert layout.at(3,4) in neighbors

Green. Commit: layout understands neighbors(cell)

Now let’s see about improving this:

class DungeonLayout:
    def at_offset(self, start, offset):
        new = (start[0]+offset[0], start[1]+offset[1])
        return self.at(*new)

I’d really like a vector class right now, but for now we’ll settle for a static method:

    def at_offset(self, start, offset):
        new = self.vector_add(start, offset)
        return self.at(*new)

    @staticmethod
    def vector_add(start, offset):
        return start[0] + offset[0], start[1] + offset[1]

Also there is no point unwrapping and re-wrapping the tuple:

    def at_offset(self, start, offset):
        new = self.vector_add(start, offset)
        return self.at(*new)

So let’s have this:

    def at(self, x, y):
        return self.at_xy((x,y))

    def at_xy(self, xy):
        return self.cells.get(xy, None)

    def at_offset(self, start, offset):
        new = self.vector_add(start, offset)
        return self.at_xy(new)

It almost seems like we could merge at and at_xy because of how clever Python is about expanding tuples, but I can understand this, so we’ll go with it. Commit: added at_xy.

Where were we?

Oh, right, looking at RoomMaker. I propose that RoomMaker can only be created with a Layout upon which to make its rooms. But, there are 45 uses of RoomMaker, another huge refactoring if this were a large program, and tedious even if it’s just me. I’ll have a look at the creators. One in main, 44 in various tests like this one:

    def test_build(self):
        random.seed(234)
        Cell.create_space(64, 56)
        size = 100
        origin = Cell.at(32, 28)
        room = RoomMaker().cave(number_of_cells=size, origin=origin)
        assert len(room.cells) == 100
        assert origin.room == room
        assert origin in room
        self.verify_contiguous(room)

I kind of feel like I’m out on a ledge here, but in fact we are green. So let’s try this: we’ll add the layout parameter to RoomMaker but allow it to be None and see what we break and when.

class RoomMaker:
    def __init__(self, layout=None):
        self.layout = layout

Then in the cave method, we’ll pass layout to the CaveCollector:

class CaveCellCollector:
    def __init__(self, layout):
        self.layout = layout
        self.cells:list[Cell] = []
        self.growth_candidates:list[Cell] = []
        self.verify_contiguous(room)
    ...
    def find_adjacent_cell(self):
        for cell in sample(self.growth_candidates,
                           len(self.growth_candidates)):
            available = self.layout.available_neighbors(cell)
            if available:
                return choice(available)
            else:
                self.growth_candidates.remove(cell)
        return None

That’s the only method in the collector that needed to change. 14 tests fail. I fix the one above:

    def test_build(self):
        random.seed(234)
        layout = DungeonLayout(64, 56)
        size = 100
        origin = layout.at(32, 28)
        room = RoomMaker(layout).cave(number_of_cells=size, origin=origin)
        assert len(room.cells) == 100
        assert origin.room == room
        assert origin in room
        self.verify_contiguous(room)

Easy enough. There are 13 more that need similar treatment. I can probably find some easy ones to fix but I think that this one is more important:

class TestCellFlood:
    def test_generate_cells(self):
        random.seed(37)
        Cell.create_space(10, 10)
        start = Cell.at(5,5)
        size = 30
        room = RoomMaker().cave(number_of_cells=size, origin=start)
        assert len(room.cells) == size
        count = 0
        for cell, _ in Flooder(start).in_any_room().flood():
            count += 1
        assert count == size
        # demonstrate convenience method
        count = 0
        for cell, _ in Flooder(start).in_any_room().flood():
            count += 1
        assert count == size

The obvious change is this:

    def test_generate_cells(self):
        random.seed(37)
        layout = DungeonLayout(10, 10)
        start = layout.at(5,5)
        size = 30
        room = RoomMaker(layout).cave(number_of_cells=size, origin=start)
        ...

But we still fail, I’m suspecting and kind of hoping in Flooder. Ah, it’s not:

    def available_neighbors(self, cell):
>       return [neighbor for neighbor in self.neighbors(cell) if neighbor.is_available]
                                                                 ^^^^^^^^^^^^^^^^^^^^^
E       AttributeError: 'NoneType' object has no attribute 'is_available'

I think we need to eliminate the None returns in neighbors():

    def neighbors(self, cell):
        neighbors = []
        for xy in self.deltas:
            if n := self.at_offset(cell.xy, xy):
                neighbors.append(n)
        return neighbors

Our fixed test works. Flooder probably needs work but is OK for now, running through Cell to do its work. But we do need to get the remaining 12 fixed before we can commit. I don’t see how to get some running one way and some the other, just gotta do these and see if we can get through it.

Without any rigmarole, all the tests convert easily and run, much like this one:

    def test_room_maker_does_not_reuse_cells(self):
        random.seed(304)
        layout = DungeonLayout(64, 56)
        maker = RoomMaker(layout)
        room_10_start = layout.at(10,10)
        room_10 = maker.cave(number_of_cells=10, origin=room_10_start)
        assert len(room_10.cells) == 10
        room_50_start = layout.at(50,50)
        room_50 = maker.cave(number_of_cells=10, origin=room_50_start)
        assert len(room_50.cells) == 10
        assert len(room_10.cells) == 10

Commit: RoomMaker.cave uses layout.

Enough already, let’s sum up.

Summary

I really needed to fix those 14 tests, because all 14 of them used the cave function of RoomMaker, so they all needed to be passed a viable layout. We might have been able to go incrementally by hacking RoomMaker somewhere, to fetch the current layout, but that’s not a good habit to get into. We should be passing it down, not pulling it out of some orifice somewhere.

We probably could have hacked the tests so that they fetch the current layout after doing their create_space. We might try that next time, as it would perhaps be a somewhat easier change.

I’m pleased with how things are going, because we’ve seen that we can readily wire everything up to the layout, but it did require me to fix 14 tests. If it has been 140 we would have had to come up with some kind of hack, but for 14 we can do it.

But that last fixing cycle took about an hour, counting a few interruptions, about 4 minutes per test. An hour is fine for a change like this, but ten hours would not be. We’ll see if we can do something better next time, although I think we have hit the biggest change in RoomMaker. There is still the Flooder object to be looked at, and, well, I really don’t know what else. But so far, so good, the current Layout idea seems to be holding water.

See you next time!