I think about my code while writing it, and at other times. Here’s what I think today.

As I wake up in the morning, I often think, often in an almost dreamlike state, about my current programming project. And often I get what seems like a good idea. And sometimes, it actually is a good idea. The primary means for finding out is typically to try it.

This morning I was reflecting on the grouping comments that I put into DungeonLayout, identifying methods with headings cell access; cell neighbors; rooms; connectivity; SuiteMaker class?; how does this relate to PathHelper?; probably should be separate object; and testing only so far.

You might note that some of those are more specific than others. That’s how my thinking works: some things pretty crisp, some still quite vague.

This morning’s thoughts were considering how the DungeonLayout really knows (only?) two things:

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

It knows all the cells and it knows the rooms that users provide.

As a rule, both received and experiential, I think a class is ideally about one thing, one notion. It might be highly complex inside, but seen from outside, it should be about just one idea. The Layout seems to be about two things. In addition, the Layout seems to have a sort of two-phase operation. It goes through a building phase, which is about allocating Cells to Rooms, and then it enters a very static phase where it just holds onto rooms, which get displayed.

An experiment yesterday showed that DungeonLayout does not need to hold on to cells at all. The experiment involved creating twelve million cells per minute, but Layout had no cells.

This morning’s thinking suggested that the Layout is about Rooms, more than it is about cells. And furthermore, I was thinking — and here is where we’ll start working — that the Room object isn’t helping quite enough.

Recall that yesterday we changed the code so that when a Room is added to the Layout, it is only allowed to keep the cells that are not already allocated to rooms already in the Layout. It’s first come, first served, when it comes to Cells, and no two rooms can contain the same cells.

Here’s the code that does that:

class DungeonLayout:
    def add_room(self, room):
        self._limit_to_available(room)
        self.rooms.append(room)

    def _limit_to_available(self, room):
        for cell in room.cells.copy():
            if self.is_in_a_room(cell):
                room.forget(cell)

That last method suffers from what Beck and/or Fowler called “feature envy”. It is more concerned about what’s inside Room than it is about itself. So I was thinking something like:

We should do that limiting thing by subtracting the existing rooms from the incoming room.

Subtracting, of course, would consist of removing all the cells of the subtrahend from the minuend.

So, and we’re finally getting around to it, we’re going to begin the morning by doing just that thing.

Let’s TDD Room.subtract.

    def test_subtract_cells(self):
        coords = [(x, 0) for x in range(10)]
        cells = [Cell(xy[0], xy[1]) for xy in coords]
        even_cells = [cells[i] for i in range(10) if i%2 == 0]
        even_room = Room(even_cells)
        big_room = Room(cells)
        big_room.subtract(even_room)
        for cell in big_room.cells:
            assert cell.x%2 == 1

Fails for want of subtract. We code:

class Room:
    def subtract(self, room):
        self._cells = self._cells - room._cells

And we are green. Commit: Room.subtract.

And now we would like to be able to subtract a collection of Rooms. Let’s do this one by wishful thinking:

class DungeonLayout:
    def add_room(self, room):
        room.subtract_all(self.rooms)
        self.rooms.append(room)

And:

class Room:
    def subtract_all(self, rooms):
        for room in rooms:
            self.subtract(room)

Green. Remove the _limit method from layout. We’ll commit this but I want another test, one for subtract_all. Commit: Room.subtract_all.

I write this test and it fails!

    def test_subtract_all(self):
        all_room = Room([Cell(x,0) for x in range(10)])
        low_room = Room([Cell(x,0) for x in range(3)])
        high_room = Room([Cell(x,0) for x in range(7, 10)])
        all_room.subtract_all([low_room, high_room])
        assert len(all_room.cells) == 4

Why? Because the cells in the three rooms are not identical, and Cell doesn’t understand ==.

I think we should fix that.

class Cell:
    def __eq__(self, other):
        return self.xy == other.xy
    def __hash__(self):
        return hash(self.xy)

Test passes. Commit: test for subtract_all. Cells now == on equality of xy.

Reflection

We have simplified DungeonLayout by providing a subtract_all method on Room, allowing cells to be removed from a room if they’re already included in another Room. We’ve done it with set operations in Room, which is nice and avoids a loop over cells. With any luck set.- is faster than the loop, but it is more expressive in any case.

The notion of subtract makes sense, I think, at the outer level, and the way it’s done is specific to the implementation of Room, which happens to use the set.

Along the way, we observed, not for the first time, that Cell comparison for equality was identity. We had encountered that and changed it in our experiment yesterday, and had noted it other times. In fact, I think the whole notion of DungeonLayout.at came about to ensure that everyone got the same identical Cell. When Cell held other things beside its coordinates, that may have made more sense. Now, there is no real reason to do it.

Hmm …
So, is this telling us that we can simplify a lot of code by replacing layout.at(x,y) with Cell(x,y)? Interesting. Let’s look at some use of at.

There are 119 uses, of which one is the definition, 116 are in tests, and two are actual use:

main.py
    ...
    dungeon.populate()
    target = layout.at(32, 28)
    options = layout.furthest_cells(target)


def make_diamond_in_round_room(layout: DungeonLayout, maker: RoomMaker):
    origin = layout.at(32, 28)
    diamond = maker.diamond(number_of_cells=13, origin=origin)
    round = maker.round(radius=5, origin=origin)
    layout.add_room(diamond)
    layout.add_room(round)

Let’s change those two and see what main does. All seems to go well. Commit main.

I am tempted to replace them all. Instead, let’s do this as an experiment:

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

    def at_xy(self, xy):
        return Cell(xy[0], xy[1])

That seems to put an infinite loop into the tests. I think I know what that is. I think it is “is”. I think we have places where we check whether a cell is another. We need == for those cases. I think we probably ran into that yesterday as well.

Note
The above change is needed, but it turns out not to be the cause of the loop.

Replacing all those does not help. We can certainly just reset but I’d like to understand what’s happening. This test is interrupted when I stop the loop:

    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)
        layout.add_room(room)
        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 layout.is_in_a_room(cell):
                room_count += 1
            else:
                other_count += 1
        assert len(all) == 100
        assert room_count == size
        assert other_count == 100 - size

Fortunately, I needed a short break, and realizing that Flooder was surely looping, I realized that our path will return cells with any index at all, not limited to the size of the layout. (I surely discovered that yesterday as well, but didn’t memorize it.)

We could fix DungeonLayout.at to check the limits. But we cannot, in general, create Cells with arbitrary coordinates. It’s semi-OK, but we really need to keep them within the limits of the layout, especially if we’re going to flood, which will go everywhere, plus minus above below everywhere.

I think we’ll stick to preferring not to use is on cell comparison, but we can’t leave our patch in to at. Commit the tests, roll back the layout.

Reflection II

As I think about this, I’m wondering why we were creating cells willy-nilly during drawing the game. It makes sense that we’d create them during creation of the layout, but after that why would we even be fetching a cell that didn’t exist?

Ah. Happened to be looking at it in RoomView:

    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.layout.at_offset(cell.xy, offset)
        if neighbor is None:
            return "wall"
        neighbor_room = self.layout.get_room(neighbor)
        if neighbor_room is None:
            return "wall"
        elif neighbor_room == self.layout.get_room(cell):
            return "open"
        else:
            return "door"

That call to layout.at_offset would create a cell every time. We could do that differently, and would have to if we ever want to completely free the Cell from the layout.

I think that to really do that we’d want to put a couple of class variables on Cell, max_x and max_y, and probably override __new__ or whatever it is in Python.

That said, it might be the case that that border code is the only place that would be a problem. And the fact is, we shouldn’t really be computing those values every time through the display loop. In a real layout, we’d compute the walls and doors once and for all.

Hm!

Now walls and doors might be an interesting new feature to work on. I suspect we would want to start with come kind of larger-scale display of a room, one showing the flooring and the treasures and monsters and such. Maybe it’s time to turn this into an actual game.

That might be quite interesting and put some interesting stress on our current little objects. I’ll add that to my thinking list.

Summary

The “big” lesson here, I think, was the Room taking over subtracting cells to ensure no overlap. The code that originally did that was in the Layout and it referred to the room and cells more than it did to itself. That’s a solid clue that the code belongs elsewhere. In Room, we have cells and we refer to them directly. Much nicer.

Very small change, but it simplifies what is currently our largest class, and didn’t add even that much complexity to the target Room class.

I am ambivalent about moving away from identity for Cell comparison of equality. In general, though, I suspect we should use == in favor of is unless the target is None or True/False. It’s going to be safer.

The brief experiment with returning fresh Cell instances drove me to make the == change, and that’s good. And it again caused me to discover that Flood will run hog-wild if no one ever delivers None when it asks for a Cell. Probably I’ll forget again. Maybe not.

So a bit of improvement, a bit of learning, a bit of re-learning. It’s all good.

I just love refactoring. It’s mostly soothing, with moments of terror.

See you next time!