Cells know what room they are in. What will it take to undo this? The bear gets really close to taking a bite today.

The Cell still contains a pointer to its room, which is used to determine whether it is available, and which can be fetched and set directly:

class Cell:
    @property
    def room(self):
        return self._room

    @room.setter
    def room(self, room):
        self._room = room

    @property
    def is_available(self) -> bool:
        return self.room is None

    @property
    def is_in_a_room(self):
        return not self.is_available

We have methods corresponding to the latter two in DungeonLayout, if memory serves:

class DungeonLayout:
    def is_available(self, xy):
        return not self.is_in_a_room(xy)

    def is_in_a_room(self, xy):
        cell = self.at(*xy)
        return any([cell in room for room in self.rooms])

It seems to me that there are at least two steps to clearing this up. One will be to ensure that all users of the is_available and is_in_a_room methods of Cell are unused, and the other will be to ensure that rooms are only set and fetched via DungeonLayout. Perhaps that is four things. Perhaps it will be many more.

As we do this, my plan is to give a nod to using very small steps, but that if there are a lot of lines that need similar changes, I plan to do them in batches if possible, even though we might, in a large enough system, need to do them over a longer time.

We’ll just let PyCharm tell us about senders. Oh! Right away we find something that may be tricky, in the tests and code for flooding. There are three uses like this one:

    def test_default_cond(self):
        layout = DungeonLayout(10, 10)
        start = layout.at(5,5)
        size = 45
        room = RoomMaker(layout).cave(number_of_cells=size, origin=start)
        assert len(room.cells) == size
        room_count = 0
        other_count = 0
        all = set()
        for cell, _ in Flooder(layout=layout, origin=start).flood():
            all.add(cell)
            if cell.is_in_a_room:
                room_count += 1
            else:
                other_count += 1
        assert len(all) == 100
        assert room_count == size
        assert other_count == 100 - size

And in Flooder itself, we have this:

class Flooder:
    def available(self):
        self.select(lambda cell: cell.is_available)
        return self

    def in_any_room(self):
        self.select(lambda cell: cell.is_in_a_room)
        return self

Ah, not so bad. Flooder has the layout so this should work:

    def available(self):
        self.select(lambda cell: self._layout.is_available(cell.xy))
        return self

I forgot the .xy and seven tests failed. I think that changing the signature was weird, and requiring everyone to rip it out of the cell seems wrong anyway. We’ll commit this much, as we are green: removing room refs to cell is_available.

Let’s rename those two methods in the Layout to reflect the xy aspect. My plan is to provide two that use a Cell, as we expect, and the existing two will use xy and do the work. First the rename.

Arrgh, PyCharm renames too much, the calls using Cell as well as the Layout. 24 tests fail. Reset.

Is there a way to do it incrementally with PyCharm’s help? Ah, yes, you can right click a line and select Exclude! I didn’t know that. That’s nearly good, well done JetBrains!

I almost got it right. Two tests fail. Missed this one, now fixed:

    def unused_cell_or_none(self):
        available = [cell for cell in self.cells.values() if self.xy_is_available(cell.xy)]
        if available:
            return random.choice(available)
        else:
            return None

Now let’s rename the other one, giving us this:

class DungeonLayout:
    def xy_is_available(self, xy):
        return not self.xy_is_in_a_room(xy)

    def xy_is_in_a_room(self, xy):
        cell = self.at(*xy)
        return any([cell in room for room in self.rooms])

Green. Commit: moving room status methods to layout.

Now we add the two methods to Layout that use Cell:

    def is_available(self, cell) -> bool:
        return self.xy_is_available(cell.xy)

    def is_in_a_room(self, cell) -> bool:
        return self.xy_is_in_a_room(cell.xy)

We don’t write tests for those. Once we’re done here, all the tests will be coming through here.

Back to senders into Cell. I change Flooder again:

class Flooder:
    def in_any_room(self):
        self.select(lambda cell: self._layout.is_in_a_room(cell))
        return self

Three tests break. This is not pleasing. But there are people who set room without informing a layout. If that’s what’s going on, we may need to address that first. What’s failing? Here’s one:

    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)
        assert len(room.cells) == size
        count = 0
        for cell, _ in Flooder(layout=layout, origin=start).in_any_room().flood():
            count += 1
        assert count == size
        # demonstrate convenience method
        count = 0
        for cell, _ in Flooder(layout=layout, origin=start).in_any_room().flood():
            count += 1
        assert count == size

Hm. Is RoomMaker offending here? I think it is. I think we need a reconception, if that’s a word, of the room definition.

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

We build the room, but we do not inform the layout. OK, I think we need to reset again.

Reflection

Right. Before we can rely on the Layout knowing what the rooms are, we have to ensure that it finds out what the rooms are. What might we do?

An easy thing might be to change the Room constructor to require a layout, and to submit the room to the layout right in its constructor.

We could do something similar with a class method, maybe Room.create_in_layout(layout, cells).

We could reserve the Room class to Layout, with a method there like DungeonLayout.create_coom(cells).

We could just let the Room be created and demand that if you want it recognized, you have to register it with DungeonLayout.add_room.

Let’s do that last thing for now. We’ll be refactoring the Layout later. Remind me to talk about that below.

Back to the Code

We’ll put that change back and fix up the RoomMaker.

I am more than a little confused now. 16 tests break. It was three last time.

I don’t see what’s up. Oh. I failed to commit the methods in layout:

class DungeonLayout:
    def is_available(self, cell) -> bool:
        return self.xy_is_available(cell.xy)

    def is_in_a_room(self, cell) -> bool:
        return self.xy_is_in_a_room(cell.xy)

Perfect, back to three failing. A few tests just need a line added:

    layout.add_room(room)

We’re back at green. Commit: Refactoring toward no room in Cell.

Where were we?

Oh, right, removing references to this:

class Cell:
    @property
    def is_in_a_room(self):
        return not self.is_available

Find them. Just do the canned change from cell.is_in_a_room to layout.is_in_a_room(cell). Green. Commit. Remove the cell method. Green. Commit.

Repeat for is_available. Curiously, this change:

    def available_neighbors(self, cell):
        return [neighbor
                for neighbor
                in self.neighbors(cell)
                if self.is_available(neighbor)]

Breaks tests. Something weird is happening: With that change in place, we create a room of 15 cells, where without it, the code creates a room of 30 cells, which the test expects.

OK, reset again.

Enough Trouble for Today

The issue is this: some code, including RoomMaker, is setting the room member of a Cell to self or, in some cases, any old thing. RoomMaker is marking the cell as “used”, belonging to it, for its own bookkeeping. But since the cell is not yet in a Room, the Layout cannot correctly answer whether the cell is available.

What we’re going to do — well, I’m not sure, but what we might do — is to have RoomMaker keep its own notes on what cells are used. It could just use a simple set, most likely. Another possibility would be to allow the Layout to keep track of such tentative uses, but my feeling is that that sort of thing should be done closer to the object that wants to keep track.

Be that as it may, we have reset at least three, maybe four times today. We’ll take that as a sign that we’re not trying good paths and need to re-orient. So we’ll call a time out, have a chai, and come back either later today or tomorrow morning.

Summary

I wouldn’t say that the bear bit me today, but I think she got a nibble of the back of my pants. Some sessions are like that, which is why estimation is so profoundly difficult, and why I rarely estimate how long something will take: I just don’t know what is going to happen.

However, we’re making progress: a few improvements were made to the code and tests. And I feel sure that despite the trouble, we’re doing the right thing trying to remove the room member from Cell. It’s just that things are a bit more tangled than I realized, so that I grabbed the thread in the wrong places a few times.

But the point isn’t the destination, it’s the learning along the way, and we have learned more about what we need to do.

That said, I am feeling the need to do something other than refactor this code. It seems that we should be making it more capable, not just a better form of just as capable as it was a week or so ago. I’m not sure, however, whether I want to push more in the direction of an actual game. That’s quite a commitment, and I’m not convinced that it’s what I want to do.

I can’t wait to find out. See you next time!