We want to change border calculations to create a permanent structure. We proceed with a few very tiny objects. I think it goes quite well, though the article gets rather long.

I’ll mark some places for you to take a break. Of course you can read, scan, whatever. I’m not the boss of you.

Let’s get back to the borders work from yesterday. Our mission is to create some kind of persistent border structure that we can create once and for all after the dungeon layout is defined, and then just draw the borders during the draw cycle, instead of recomputing them, as we are doing now.

I abandoned the new border work in the middle, so let’s review what we have. It’s not much, but it’s enough to run the tests:

    def test_cell_borders(self):
        layout = DungeonLayout(10, 10)
        cell = Cell(1, 1)
        borders = self.get_borders(cell, layout)
        assert len(borders) == 4
        for side in [(1,0), (-1,0), (0, -1), (0,1)]:
            assert any(border.side == side for border in borders)
        for border in borders:
            assert border.border == 'wall'

    def test_two_rooms(self):
        layout = DungeonLayout(10, 10)
        cell_west = Cell(1, 1)
        cell_east = Cell(2, 1)
        room_west = Room([cell_west])
        room_east = Room([cell_east])
        layout.add_room(room_west)
        layout.add_room(room_east)
        west_borders = self.get_borders(cell_west, layout)
        east_border = next(border for border in west_borders if border.side == (1,0))
        assert east_border.border == 'partition'

The first test was enough to “fake it till we make it”, and we produced the core of the get_borders method, and this little Border class:

@dataclass
class Border:
    side: tuple
    border: str

The side there is the offset of one of the four walls of the cell, (1,0) for the east wall and so on. And border is just a string, intended to be ‘wall’, ‘partition’, or ‘open’. I’m planning to have a fourth string, ‘door’, in due time. The ‘partition’ value means that we have a wall between two rooms, while ‘wall’ is a wall between a room and solid rock.

The second test requires a return that isn’t faked, and we elaborated the get_items, which now looks like this:

    def get_borders(self, cell, layout):
        borders = []
        my_room = layout.get_room(cell)
        for side in [(1,0), (-1,0), (0, -1), (0,1)]:
            neighbor = layout.at_offset(cell.xy, side)
            if neighbor is None:
                border = 'wall'
            else:
                neighbor_room = layout.get_room(neighbor)
                if neighbor_room is None:
                    border = 'wall'
                elif neighbor_room == my_room:
                    border = 'open'
                else:
                    border = 'partition'
            borders.append(Border(side, border))
        return borders

That code just checks the cell at the neighbor that corresponds to a side, and classifies it as wall, partition, or open, adding the border to a list.

As I often do when working out a new algorithm, I’m writing all the code right in the test file. I believe that I borrowed this idea from Keith Braithwaite, to whom I owe more than I can ever repay. Keith, among other contributions, provided a learning session “TDD as if you really meant it” that included this idea. I’m probably not doing it justice, but anyway I do find working things out inside the test file to be useful. One very good reason for that is that we can always commit, because there’s nothing going on outside our test file, so we can’t break anything else. Naturally, I would generally not commit other than on green, as it would just confuse me the next day.

I think we should at least test for ‘open’. We could enhance the existing test for that, but instead, I’m not sure why, let’s write another separate test.

    def test_one_room_gets_open(self):
        layout = DungeonLayout(10, 10)
        cell_west = Cell(1, 1)
        cell_east = Cell(2, 1)
        room_west = Room([cell_west, cell_east])
        layout.add_room(room_west)
        west_borders = self.get_borders(cell_west, layout)
        east_border = next(border for border in west_borders if border.side == (1,0))
        assert east_border.border == 'open'

Instead of two rooms west and east, there’s just one room with two cells west and east the border between west and east should be open, and it is. Green. Shall we check all the others? Could do:

    def test_one_room_gets_open(self):
        layout = DungeonLayout(10, 10)
        cell_west = Cell(1, 1)
        cell_east = Cell(2, 1)
        room_west = Room([cell_west, cell_east])
        layout.add_room(room_west)
        west_borders = self.get_borders(cell_west, layout)
        east_border = next(border for border in west_borders if border.side == (1,0))
        assert east_border.border == 'open'
        west_borders.remove(east_border)
        for border in west_borders:
            assert border.border == 'wall'

Still green. Forgot to commit above, I got so excited. Commit now: working on Borders.

What’s next?

We need a permanent structure containing a Borders list for each cell in the layout, or—when we’re done—all the non-open ones. I really think a simple dictionary from cell to a list of borders will do. But let’s go whole hog here, and make a BorderList and a BorderMap object.

I’m going to code the BorderList into the current get thing, let the tests break, and then fix them up to use the new list object.

I code this much:

class BorderList:
    def __init__(self):
        self.borders = []

    def append(self, border):
        self.borders.append(border)

And then use it, which is easy and probably breaks my tests:

    def get_borders(self, cell, layout):
        borders = BorderList()
        ...

The tests all break. One is checking the len, two are trying to iterate. I think we’ll find that useful, so we’ll just implement __len__, and __iter__. One test asks for remove, so I do that as well:

class BorderList:
    def __init__(self):
        self.borders = []

    def append(self, border):
        self.borders.append(border)

    def remove(self, border):
        self.borders.remove(border)

    def __len__(self):
        return len(self.borders)

    def __iter__(self):
        return iter(self.borders)

You may be wondering why I’ve gone to the trouble of essentially re-implementing list. Both received wisdom and personal experience have told me repeatedly that making specialized collections pays off. So we’re trying it. Tests are green. Commit.

Now for the structure that maps cell to BorderList. We test:

    def test_border_map(self):
        layout = DungeonLayout(10, 10)
        cell_west = Cell(1, 1)
        cell_east = Cell(2, 1)
        room_west = Room([cell_west])
        room_east = Room([cell_east])
        layout.add_room(room_west)
        layout.add_room(room_east)
        border_map = self.get_border_map(layout)
        west_borders = border_map[cell_west]
        for border in west_borders:
            expected = 'partition' if border.side == (1,0) else 'wall'
            assert border.border == expected

This test expects the border_map to be able to be indexed by Cell. Yes, a dictionary would suffice. I think we’ll see later why we want not to do that, although possibly a default dictionary would still serve. We’re rolling our own here.

    def get_border_map(self, layout: DungeonLayout):
        border_map = BorderMap()
        for room in layout.rooms:
            for cell in room:
                borders = self.get_borders(cell, layout)
                border_map.assign(cell, borders)
        return border_map

And the class:

class BorderMap:
    def __init__(self):
        self.borders = {}

    def assign(self, cell, borders):
        self.borders[cell] = borders

    def __getitem__(self, cell):
        return self.borders[cell]

We are green. Commit.

Good Place for a Break

Reflection

Time to reflect. We have three classes involved in this border idea. The BorderMap contains a BorderList for each cell in the map, while the BorderList contains a Border instance for each of the four cardinal directions, while the Border contains a side (the direction to a side) and a string, ‘wall’, ‘partition’, or ‘open’ (and perhaps someday, ‘door’).

Given what we have so far, this could all have been done with a dictionary pointing to a list of tuples. Yet we have three classes instead. Can this possibly be a good idea? I will say that it is the way I was taught to do things, and the way that I have found is generally best. I have regretted direct use of system collections far more times than I have ever regretted building my own little objects.

Right now, we are nearly in a position to move these three little classes over to the production side of things, since their tests all run. I think that, for now, we’ll keep them together in one file.

But before we do that, we should deal with moving those get methods somewhere inside these classes. It has to live somewhere, and certainly the tests aren’t going to cut it.

Both our methods are constructors, really, so let’s see how they fit in as class methods on our new little classes.

class TestBorders:
    def get_border_map(self, layout: DungeonLayout):
        border_map = BorderMap()
        for room in layout.rooms:
            for cell in room:
                borders = self.get_borders(cell, layout)
                border_map.assign(cell, borders)
        return border_map

No. I started with this but it got messy. We’ll start at the bottom:

class TestBorders:
    def get_borders(self, cell, layout):
        borders = BorderList()
        my_room = layout.get_room(cell)
        for side in [(1,0), (-1,0), (0, -1), (0,1)]:
            neighbor = layout.at_offset(cell.xy, side)
            if neighbor is None:
                border = 'wall'
            else:
                neighbor_room = layout.get_room(neighbor)
                if neighbor_room is None:
                    border = 'wall'
                elif neighbor_room == my_room:
                    border = 'open'
                else:
                    border = 'partition'
            borders.append(Border(side, border))
        return borders

We’ll move that to BorderList. I’m starting to suspect we may discover something soon. Not sure what. Anyway:

Change signature on BorderList:

class BorderList:
    def __init__(self, layout, cell):
        self.borders = []

Cut the test get_borders code and expect to get the right thing back from BorderList:

    def get_borders(self, cell, layout):
        return BorderList(layout, cell)

Edit the code in BorderList.__init__:

class BorderList:
    def __init__(self, layout, cell):
        self.borders = []
        my_room = layout.get_room(cell)
        for side in [(1,0), (-1,0), (0, -1), (0,1)]:
            neighbor = layout.at_offset(cell.xy, side)
            if neighbor is None:
                border = 'wall'
            else:
                neighbor_room = layout.get_room(neighbor)
                if neighbor_room is None:
                    border = 'wall'
                elif neighbor_room == my_room:
                    border = 'open'
                else:
                    border = 'partition'
            self.borders.append(Border(side, border))

Now in the test we just have this:

    def get_borders(self, cell, layout):
        return BorderList(layout, cell)

So we inline that. The tests all now access BorderList directly, for example:

    def test_cell_borders(self):
        layout = DungeonLayout(10, 10)
        cell = Cell(1, 1)
        borders = BorderList(layout, cell)
        assert len(borders) == 4
        for side in [(1,0), (-1,0), (0, -1), (0,1)]:
            assert any(border.side == side for border in borders)
        for border in borders:
            assert border.border == 'wall'

Green. Commit.

I think we’ll want to improve that init. But let’s do the other get method first:

class TestBorders:
    def get_border_map(self, layout: DungeonLayout):
        border_map = BorderMap()
        for room in layout.rooms:
            for cell in room:
                borders = BorderList(layout, cell)
                border_map.assign(cell, borders)
        return border_map

Again, change the signature, this time on the BorderMap:

class TestBorders:
    def get_border_map(self, layout: DungeonLayout):
        border_map = BorderMap(layout)
        for room in layout.rooms:
            for cell in room:
                borders = BorderList(layout, cell)
                border_map.assign(cell, borders)
        return border_map

Now cut the code from the get and move it over to BorderMap.__init__:

class BorderMap:
    def __init__(self, layout):
        self.borders = {}
        for room in layout.rooms:
            for cell in room:
                borders = BorderList(layout, cell)
                self.assign(cell, borders)

    def assign(self, cell, borders):
        self.borders[cell] = borders

    def __getitem__(self, cell):
        return self.borders[cell]

class TestBorders:
    def get_border_map(self, layout: DungeonLayout):
        border_map = BorderMap(layout)
        return border_map

Inline all the things:

class BorderMap:
    def __init__(self, layout):
        self.borders = {}
        for room in layout.rooms:
            for cell in room:
                borders = BorderList(layout, cell)
                self.borders[cell] = borders

    def __getitem__(self, cell):
        return self.borders[cell]

class TestBorders:
    def test_border_map(self):
        layout = DungeonLayout(10, 10)
        cell_west = Cell(1, 1)
        cell_east = Cell(2, 1)
        room_west = Room([cell_west])
        room_east = Room([cell_east])
        layout.add_room(room_west)
        layout.add_room(room_east)
        border_map = BorderMap(layout)
        west_borders = border_map[cell_west]
        for border in west_borders:
            expected = 'partition' if border.side == (1,0) else 'wall'
            assert border.border == expected

Green. Commit. (Should have committed about three times above. Tempted to automate that.)

Good Place for a Break

Reflection

Where are we? We have three little classes handling borders for us, and the tests are no longer doing any of the work, just the checking. We can move those three classes over to the production side any time we want to, and that time is coming. But I have a concern with this code:

class BorderList:
    def __init__(self, layout, cell):
        self.borders = []
        my_room = layout.get_room(cell)
        for side in [(1,0), (-1,0), (0, -1), (0,1)]:
            neighbor = layout.at_offset(cell.xy, side)
            if neighbor is None:
                border = 'wall'
            else:
                neighbor_room = layout.get_room(neighbor)
                if neighbor_room is None:
                    border = 'wall'
                elif neighbor_room == my_room:
                    border = 'open'
                else:
                    border = 'partition'
            self.borders.append(Border(side, border))

This code is making the BorderList, but it is spending all its time making Border instances. Feature Envy. More, maybe most of this work should be done by the Border object.

It seems to me that we should pass the cell, the side and the layout to Border and have it do the decoding. Or, perhaps, we would get the neighbor cell and classify the border based on the two cells, which we can do. Let’s first extract a method for that.

class BorderList:
    def __init__(self, layout, cell):
        self.borders = []
        my_room = layout.get_room(cell)
        for side in [(1,0), (-1,0), (0, -1), (0,1)]:
            neighbor = layout.at_offset(cell.xy, side)
            border = self.classify_border(layout, my_room, neighbor)
            self.borders.append(Border(side, border))

    def classify_border(self, layout, my_room, neighbor):
        if neighbor is None:
            border = 'wall'
        else:
            neighbor_room = layout.get_room(neighbor)
            if neighbor_room is None:
                border = 'wall'
            elif neighbor_room == my_room:
                border = 'open'
            else:
                border = 'partition'
        return border

I’m not as fond of that as I might be. Let’s reset that. I think I’d like to have the lower level code working on cells. But let’s think about this a bit. No, it’s about rooms, isn’t it? Let’s modify the init so that the bottom bit focuses on rooms, like this:

class BorderList:
    def __init__(self, layout, cell):
        self.borders = []
        my_room = layout.get_room(cell)
        for side in [(1,0), (-1,0), (0, -1), (0,1)]:
            neighbor = layout.at_offset(cell.xy, side)
            neighbor_room = layout.get_room(neighbor) if neighbor else None
            if neighbor_room is None:
                border = 'wall'
            elif neighbor_room == my_room:
                border = 'open'
            else:
                border = 'partition'
            self.borders.append(Border(side, border))

Green. Commit? Let’s hold off, I’m still not sure I like this. Extract method:

class BorderList:
    def __init__(self, layout, cell):
        self.borders = []
        my_room = layout.get_room(cell)
        for side in [(1,0), (-1,0), (0, -1), (0,1)]:
            neighbor = layout.at_offset(cell.xy, side)
            neighbor_room = layout.get_room(neighbor) if neighbor else None
            border = self.classify_border(my_room, neighbor_room)
            self.borders.append(Border(side, border))

    def classify_border(self, my_room, neighbor_room):
        if neighbor_room is None:
            border = 'wall'
        elif neighbor_room == my_room:
            border = 'open'
        else:
            border = 'partition'
        return border

Let’s return instead of saying border = all over.

class BorderList:
    def __init__(self, layout, cell):
        self.borders = []
        my_room = layout.get_room(cell)
        for side in [(1,0), (-1,0), (0, -1), (0,1)]:
            neighbor = layout.at_offset(cell.xy, side)
            neighbor_room = layout.get_room(neighbor) if neighbor else None
            border = self.classify_border(my_room, neighbor_room)
            self.borders.append(Border(side, border))

    def classify_border(self, my_room, neighbor_room):
        if neighbor_room is None:
            return 'wall'
        elif neighbor_room == my_room:
            return 'open'
        else:
            return 'partition'

I think we’ll commit this and move the code to prod. Commit: border-making classes.

I think we’ll move them all to one file for now, as they are so closely related. PyCharm move will do it. Commit: move border classes to src.

Here they are in all their glory:

@dataclass
class Border:
    side: tuple
    border: str


class BorderList:
    def __init__(self, layout, cell):
        self.borders = []
        my_room = layout.get_room(cell)
        for side in [(1,0), (-1,0), (0, -1), (0,1)]:
            neighbor = layout.at_offset(cell.xy, side)
            neighbor_room = layout.get_room(neighbor) if neighbor else None
            border = self.classify_border(my_room, neighbor_room)
            self.borders.append(Border(side, border))

    def __len__(self):
        return len(self.borders)

    def __iter__(self):
        return iter(self.borders)

    def classify_border(self, my_room, neighbor_room):
        if neighbor_room is None:
            return 'wall'
        elif neighbor_room == my_room:
            return 'open'
        else:
            return 'partition'

    def append(self, border):
        self.borders.append(border)

    def remove(self, border):
        self.borders.remove(border)


class BorderMap:
    def __init__(self, layout):
        self.borders = {}
        for room in layout.rooms:
            for cell in room:
                borders = BorderList(layout, cell)
                self.borders[cell] = borders

    def __getitem__(self, cell):
        return self.borders[cell]

I find that append is unused, and I think remove is used only in a test. Remove append. Green. Commit. Remove remove and fix the test:

    def test_one_room_gets_open(self):
        layout = DungeonLayout(10, 10)
        cell_west = Cell(1, 1)
        cell_east = Cell(2, 1)
        room_west = Room([cell_west, cell_east])
        layout.add_room(room_west)
        west_borders = BorderList(layout, cell_west)
        east_border = next(border for border in west_borders if border.side == (1,0))
        assert east_border.border == 'open'
        west_borders = [border for border in west_borders if border.side != (1,0)]
        for border in west_borders:
            assert border.border == 'wall'

I just selected the rest. Green. Commit.

Break if you must but we’re nearly done!

We’re left with this bump:

class BorderList:border(self, my_room, neighbor_room):
        if neighbor_room is None:
            return 'wall'
        elif neighbor_room == my_room:
            return 'open'
        else:
            return 'partition'

This sure seems like Feature Envy on Room, doesn’t it? Let’s move it. It’s a bit GUI but maybe abstract enough. Something goes wrong: I’m getting a None room on the my_room cell. Ah, the test is inadequate:

    def test_cell_borders(self):
        layout = DungeonLayout(10, 10)
        cell = Cell(1, 1)
        borders = BorderList(layout, cell)
        assert len(borders) == 4
        for side in [(1,0), (-1,0), (0, -1), (0,1)]:
            assert any(border.side == side for border in borders)
        for border in borders:
            assert border.border == 'wall'

Need to have a room there:

    def test_cell_borders(self):
        layout = DungeonLayout(10, 10)
        cell = Cell(1, 1)
        layout.add_room(Room([cell]))
        borders = BorderList(layout, cell)
        assert len(borders) == 4
        for side in [(1,0), (-1,0), (0, -1), (0,1)]:
            assert any(border.side == side for border in borders)
        for border in borders:
            assert border.border == 'wall'

That’s running on this code now:

class BorderList:
    def __init__(self, layout, cell):
        self.borders = []
        my_room = layout.get_room(cell)
        for side in [(1,0), (-1,0), (0, -1), (0,1)]:
            neighbor = layout.at_offset(cell.xy, side)
            neighbor_room = layout.get_room(neighbor) if neighbor else None
            border = my_room.border_type(neighbor_room)
            self.borders.append(Border(side, border))

class Room:
    def border_type(self, neighbor_room):
        if neighbor_room is None:
            return 'wall'
        elif neighbor_room == self:
            return 'open'
        else:
            return 'partition'

Let’s sum up.

Summary

We started with code in a test, building first a simple Border object, then a BorderList, containing four Borders, and then a BorderMap, containing the BorderLists for a bunch of cells. The Map is built starting from Rooms, because that seems like the right place to start creating borders between all the cells of the rooms. We have no need for borders on cells that are not in rooms.

After a bunch of squidging and scrunching, we have those classes over in the ‘src’ tree, and none of them have any actually public methods. We can subscript the Map by cell, and iterate the BorderList to get the Border instances we need. We’ll see if that is just what we want when we try to apply the classes. I am open to the possibility that we’ll want to have them work a bit differently, because, for whatever reason, I started building them without regard to a particular use. It might have been better to start that way, but I had what I had, and what I had was the idea to build the things the way we did.

Moving the test algorithms for our get_ methods ultimately resulted in there being less code everywhere, and also drove out the need for a convenience method on Room to classify the borders, since the question of border is all about what kind of room is adjacent to us.

This took place over a couple of sessions, with over a dozen commits. We could probably have committed more like twenty times, but often I forget and at least once I held off for uncertainty.

Now we are in a position to set up a permanent structure defining our borders, and to use it, replacing the code that thrashes it all out 60 times a second.

Was it worth it?

Was it worth our time to build those three little classes? Certainly the building itself went very smoothly, and refactoring from the get methods into the little classes went smoothly (and would not have been possible had we used system collections: no place for the code to go). I think having a place for those inits to live other than in user code pretty much settles the issue for me.

And wait until we install them. I think we’ll find that they plug in readily, and if, as I suspect we might, we want some small changes, we’ll have a place to put them. YMMV, but I’m glad I went this way.

No excitement, no debugging sessions, just step by step. About as good as it gets.