We’ll begin the morning adjusting more tests to use the new Layout idea. This iterative incremental stuff? It really works! The Niagara Falls Method.

My starting target for the morning will be tests that use the Cell.create_space(x,y) method, which can be mechanically converted to layout=DungeonLayout(x,y). Now if I’m not mistaken, which assumes facts not in evidence, we can make that change and all the tests will run, even though the test will also be referring to Cell later on, because Cell has a hook to pick up the current Layout. I’ll try a few of these to be sure.

No, you know what, let’s bang them all with a replace. There are only 30 and we can always roll back.

Replaced 30. Five tests fail. Might be easy enough. Yes, we just needed an import in one test file. We are green. There should be no calls to create_space. Commit: remove Cell.create_space.

Reflection

This was pleasing, and it would have worked in a giant code base, because we have wired Cell to be able to find the layout:

class Cell:
    @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()

This connection is not desirable: we don’t really want Cell to be aware of its container, a DungeonLayer, but as a temporary bridge while we refactor, this lets us go in smaller steps.

Such as …

We should be in good shape now to change those references to the Cell class methods at and unused_cell_or_none, at least in our tests, because all our tests must have a Layout by now. There are perhaps some calls to those methods that are internal to Cell itself. We’ll be careful at first and then perhaps fire off another replace, depending what we see.

A look at the list encourages me to try the global replace again. We can always revert.

8 tests out of 56 fail. One has a second call to DungeonLayout in it. And this method in DungeonLayout shouldn’t have been changed to layout but to self:

    def add_from_dictionary(self, name_to_xy):
        for name, tuples in name_to_xy.items():
            cells = [ self.at(x,y) for x,y in tuples ]
            self.add_room(Room(cells, name))

Two tests failing. Both referred to dungeon where we had layout. Rename the temps. Green. Commit: removing references to Cell.at. We need to test main and it has a problem.

Along the way I found that main wasn’t passing the Layout to the RoomMaker. Fixed that. Then I’m changing RoomMaker to require the argument (we were allowing None.) The tests will find the other places. Quickly fixed.

Reflection

Here again, if there had been many such cases, we could have wired RoomMaker to fetch the current Layout, similarly to how we hacked create_space. That would keep both schemes working as long as needed. The point being, we seem always to be able to find simple changes that allow us to proceed in small steps even if there are many users of the old scheme.

Since long delays in delivery tend to be frowned upon, we can therefore undertake important design changes, but deliver them incrementally while working on features as well. Had I known this fact a few decades ago, all our lives might have been quite different. As it turns out, my current life is pretty fine, so I wouldn’t go back and change things even if it were allowed. But for those of us who are still working for a living, this is good to know.

What now?

I think there are no calls to Cell.at. Let’s check that. Ah, wrong, we have this in Cell:

class Cell:
    def at_offset(self, dx, dy):
        new_x, new_y = self.x + dx, self.y + dy
        return self.at(new_x, new_y)

That’s used a few times, all within Cell. Let’s inline the at here, removing the class method.

class Cell:
    def at_offset(self, dx, dy):
        from dungeonlayout import current
        new_x, new_y = self.x + dx, self.y + dy
        return current.at(new_x, new_y)

Green. Commit: remove class method Cell.at entirely. Oops, forgot to commit up before “Reflection”, picked it up here.

We have one more class method in Cell that refers to current:

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

Here again, this could remain in place for a long time, deprecated, perhaps rigged to send nasty emails to users after a month or so. We only have a few uses and we can fix them right up.

One is a test, referring to layout is just fine. The rest are in main, mostly like this example:

def make_a_round_room(layout: DungeonLayout, maker: RoomMaker):
    if origin := layout.unused_cell_or_none():
        rad = random.randint(5, 6)
        room = maker.round(radius=rad, origin=origin)
        layout.add_room(room)

That first if line used to refer to Cell, but layout is right there. There are no more references to Cell.unused_cell_or_none. Remove it. Commit: remove Cell.unused_cell_or_none.

Reflection

Cell class now has just one reference to current, which, I think, is perhaps the only reference to it anywhere. Yes, that is the case. So when we remove that one remaining reference, Cell will be entirely disconnected from the Layout, which will be a good thing. We’ll not undertake that change this morning, but we’ll take a look at it, to let the implications kind of seep into the crevices of our mind.

What we have seen over the past few sessions is a series of almost 200 changes which could have been done one at a time over a long period of time, slowly moving the design toward Cell being an independent low-level object, instead of an object that knows too much about its owners. We did it in a few days, and some of it was amenable to a global replace strategy, and that could have worked on a much larger code base, although it would have been much more scary and probably less likely to really work. In a real world situation, we might quickly try a global replace, but when it didn’t work, we could readily revert and go step by step.

I believe that—with a small epsilon—any refactoring that we need can be done in small steps, over arbitrary periods of time, interspersed with feature development. Therefore, I believe that we can always keep our code well-designed as we work incrementally and iteratively.

You shouldn’t just believe me. But you might take my belief and examples as encouragement to try the notion in your own space. It can really pay off to be able to deliver features and keep the code fresh. And, honestly, I get a much better feeling making the code more nearly right than I do from just slamming in another feature. Whee!

Preparing for next time

Let’s glance at the at_offset issue, to get a sense of how we might address it.

    def at_offset(self, dx, dy):
        from dungeonlayout import current
        new_x, new_y = self.x + dx, self.y + dy
        return current.at(new_x, new_y)

Now a thing we could do right now would be to forward the actual operation to DungeonLayout, instead of doing any work here. We do have that method in the layout. We’ll do that.

    def at_offset(self, dx, dy):
        from dungeonlayout import current
        return current.at_offset(self.xy, (dx, dy))

Green. Commit: forward at_offset to current (layout)

But we were here to look at the users of this method, so to continue with the review:

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"

This one should be right out. It’s really part of the view code and should be moved out of cell, either into the RoomView or perhaps Layout.

class Cell:
    def attempt_move(self, dx, dy):
        cell = self.at_offset(dx, dy)
        if not cell or cell.is_available:
            return self
        return cell

Again something that belongs to the layout. In the longer term, trying to move will involve looking for doors and all kinds of things, while this just checks to see if we’re not trying to step out of the layout. Needs to move.

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

Right. Layout can do this already. Forward it:

class Cell:
    @property
    def neighbors(self):
        from dungeonlayout import current
        return current.neighbors(self)

One more, I think. Ah, it’s in PathHelper:

class PathHelper:
    def cell_at_offset(self, step_number, step_size, y_offset) -> Cell:
        sign = -1 if step_number % 2 == 0 else 1
        x_rot, y_rot = self.rotate(step_number * step_size, sign * y_offset, self.theta)
        return self.source.at_offset(x_rot, y_rot)

That’s kind of experimental: it’s the object that tries to make a straight path wander, by computing offsets from the path and then drawing paths between those points instead of the original points. We’ll want to think about that one. Fixing that up will probably be easy: we’ll create it with a layout member, but I think that deserves its own session.

Summary

We’re moving toward a design where the Layout holds the cells and manages relationships such as neighbors, and Cells no longer have any notion of the containers that hold them, or of their neighbors. I’ve always felt squidgy about objects knowing their collections, despite the fact that I have fallen into that structure more than a few times. It generally gets weird after a while, when suddenly more than one collection holds the same instance.

It hasn’t really gotten weird here, so far, but as soon as we tried to have more than one layout at a time, we might have run into trouble. Of course, we may never need more than none layout at a time, unless of course we’re working on a multi-player game where the parties can be in different layouts at the same time or something. We’ll really never go there, but if this were a real game being built, we just might.

But the squidgy feeling is almost gone now. The Cell barely knows anything about its owner, and only forwards messages to it. Its connection is through a global / singleton kind of hookup that it knows, and Cell is the only user of that connection, so we are two or three small steps from disconnecting it.

There is another issue in Cell, which is that it knows the room it is in, but that is really no more than a token, if I’m not mistaken, used at the convenience of the layout that owns the cells. I think we’ll remove that.

That will leave the cell containing nothing but the x-y tuple defining where it is, and I’d bet a nickel that we may not even need that. After all, if we are accessing via Layout, the Layout already knows the x-y of any cell we might deal with. I don’t know where that will take us, but we’ll work the code until we know the answer.

Cell’s methods right now are:

_border_at, at_offset, attempt_move, available_neighbors, borders, deltas, distance, east, find_path, furthest_cells, is_available, is_in_a_room, manhattan_distance, neighbors, north, path_map, random_neighbors, room, south, vector_diff, west, x, xy, y.

24, if my count is correct. Still quite a few, and my intuition says that there should be more like ten than 24. We’ll see.

Slowly the code turns better, step by step, inch by inch …

We’ll call this the Niagara Falls Method. Er, maybe not. See you next time!