Let’s look around, see what we might do that might improve things.

I am sorely tempted to try an intermediate way of not allocating all the cells, keeping allocated ones in the dictionary but not available ones. I’ll try to hold off on that, instead looking for other improvements.

DungeonLayout still has a raft of methods: _border_at, _limit_to_available, _make_path_rooms, add_from_dictionary, add_room, add_rooms_from_map, at, at_offset, at_xy, available_neighbors, borders, cells_in_room, define_suites, deltas, ensure_connected, find_path, find_suite, furthest_cells, get_room, is_available, is_fully_connected, is_in_a_room, neighbors, number_of_rooms, path_map, random_neighbors, remove_room, string_map_to_dictionary, unused_cell_or_none, vector_add. 35, counting some internals that I didn’t list here.

Some of these rather clearly belong somewhere else. Cell has vector_diff, and you’d think vector_add would fit right in there. Let’s see about that. Ah, maybe not:

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

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

This is adding two xy tuples. We’ll just inline, with a helpful rename of the args:

    def at_offset(self, start_xy, offset_xy):
        new = start_xy[0] + offset_xy[0], start_xy[1] + offset_xy[1]
        return self.at_xy(new)

Green. Commit: tidying.

I remove the class variable deltas, inlining it in its single use:

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

Commit.

Could we do that with a list comprehension? We can:

    def neighbors(self, cell):
        return [n
                for xy in [(-1, 0), (1, 0), (0, -1), (0, 1)]
                if (n := self.at_offset(cell.xy, xy))]

Rename n:

    def neighbors(self, cell):
        return [neighbor
                for xy in [(-1, 0), (1, 0), (0, -1), (0, 1)]
                if (neighbor := self.at_offset(cell.xy, xy))]

Commit.

I confess to a bit of uncertainty over that. It’s shorter, which I do like. But it may not be as easy to read as the long-hand one. Both use the walrus :=, so we can’t complain about that. We’ll let it ride.

These two methods are questionable:

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

They are used only here:

    def draw_border(self, cell, my_cells, screen):
        borders = self.layout.borders(cell)
        colors = {"open":"black", "door":"green", "wall":"red"}
        directions = {'n':(0,-1), 's':(0,1), 'e':(1,0), 'w':(-1,0)}
        lines = self.make_lines(cell)
        for direction, neighbor_offset in directions.items():
            color = colors[borders[direction]]
            self.line(*lines[neighbor_offset], color, screen)

And in this test:

    def test_border_characterization(self):
        layout = DungeonLayout(5, 5)
        cell = layout.at(3,3)
        cell1 = layout.at(2,3)
        layout.add_room(Room([cell, cell1], '1'))
        cell2 = layout.at(4,3)
        layout.add_room(Room([cell2], '2'))
        c_33 = layout.at(3,3)
        borders = layout.borders(c_33)
        assert len(borders) == 4
        assert borders["n"] == "wall"
        assert borders["e"] == "door"
        assert borders["w"] == "open"
        assert borders["s"] == "wall"

Let’s extract a method in RoomView:

    def draw_border(self, cell, my_cells, screen):
        borders = self.borders(cell)
        colors = {"open":"black", "door":"green", "wall":"red"}
        directions = {'n':(0,-1), 's':(0,1), 'e':(1,0), 'w':(-1,0)}
        lines = self.make_lines(cell)
        for direction, neighbor_offset in directions.items():
            color = colors[borders[direction]]
            self.line(*lines[neighbor_offset], color, screen)

    def borders(self, cell):
        return self.layout.borders(cell)

Now in the test, let’s get a RoomView and call the method there. That will keep the test running when we move the code.

    def test_border_characterization(self):
        layout = DungeonLayout(5, 5)
        cell = layout.at(3,3)
        cell1 = layout.at(2,3)
        room_1 = Room([cell, cell1], '1')
        layout.add_room(room_1)
        cell2 = layout.at(4,3)
        layout.add_room(Room([cell2], '2'))
        c_33 = layout.at(3,3)
        dungeon = Dungeon(layout)
        view = RoomView(dungeon, room_1)
        borders = view.borders(c_33)
        assert len(borders) == 4
        assert borders["n"] == "wall"
        assert borders["e"] == "door"
        assert borders["w"] == "open"
        assert borders["s"] == "wall"

That’s green. Commit: update test to use RoomView

Now to move the methods. I’ll just paste them over and fix up. That amounts to inserting layout a lot:

class 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"
        elif self.layout.get_room(neighbor) is None:
            return "wall"
        elif self.layout.get_room(neighbor) == self.layout.get_room(cell):
            return "open"
        else:
            return "door"

We can avoid a bit of duplication there.

    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"

Remove the methods from DungeonLayer. Green. Commit: move border calculation to RoomView.

There are these two properties:

class DungeonLayout:
    @property
    def is_fully_connected(self):
        suites = self.define_suites()
        return len(suites) == 1

    @property
    def number_of_rooms(self):
        return len(self.rooms)

I’m not comfortable with these being properties as they are computed rather “large” properties of the layout, not simple accessor-like things. Slightly tedious, as PyCharm can’t help, but quickly done. Commit: make properties into methods.

After this, I did a bit of grouping of methods together with a header comment saying what they are about or suggesting where they might belong. Here’s one of the latter ones:

class DungeonLayout:
# probably should be separate object.
    def add_rooms_from_map(self, string_map):
        d = self.string_map_to_dictionary(string_map)
        self.add_from_dictionary(d)

    @staticmethod
    def string_map_to_dictionary(string_map):
        string_map = textwrap.dedent(string_map)
        strings = string_map.split('\n')
        strings = [string for string in strings if string]
        room_cells = dict()
        for y, line in enumerate(strings):
            for x, char in enumerate(line):
                if char != '.':
                    if not char in room_cells:
                        room_cells[char] = []
                    room_cells[char].append((x, y))
        return room_cells

    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))

That’s the code that lets us create a layout by drawing a map like this:

        map = '''
        ..1..
        ..1..
        11111
        ..1..
        ..1..
        '''

It seems very unlikely that anyone would ever do that in production. So probably that whole thing should be made an object in the testing folder. There are two or three other groups of methods where I added similar comments:

# SuiteMaker class?
...
# how does this relate to PathHelper?
...
# testing only, so far. Possible LayoutAnalyzer class?
...

The other groupings, which seem to me to probably belong in DungeonLayout, are “cell access”, “cell info”, “cell neighbors”, “rooms”, and “connectivity”.

It is “interesting” that so much of DungeonLayout is about cells, which are little more than xy coordinates, and possibly some of this info should be pushed down to the Room class. We could imagine that a Layout would be a collection of rooms, and rooms would be collections of cells. At this writing, however, I am not comfortable with that notion. We’ll see what the code tells us.

Summary

We have moved a bit of code out of DungeonLayout, and simplified a bit more. Half a dozen commits, decent improvements.

In case you’re wondering, no, I don’t think I could do this forever. I would probably starve or die of dehydration. I have to pause sometimes.

Seriously, I suspect there is always something that could be just a tiny bit better. We abandon refactoring rather than finish it. But we’re not going to abandon this yet.

See you next time!