Having completed the basics of the contents vs layout design, it’s time to look around and see what needs improvement.

If I were willing to work when tired, or when an article exceeds 500 multi-line lines, we might have done more tidying yesterday, but that is not my way. With a day or two behind us, we probably do a better job of recognizing bumps and dips in the code that would elude us right after a big chunk of coding or refactoring.

I see something already, on the PyCharm tab that is open right in front of me:

class Dungeon:
    def populate(self):
        dot_room = random.choice(self.rooms)
        dot_cell = random.choice(dot_room.cells)
        self.adventurer_cell = dot_cell

    def place_adventurer_at(self, cell):
        self.adventurer_cell = cell

Somewhere in my past there is the idea that the populate should call the place, rather than do the member assignment directly. Doing so:

    def populate(self):
        dot_room = random.choice(self.rooms)
        dot_cell = random.choice(dot_room.cells)
        self.place_adventurer_at(dot_cell)

    def place_adventurer_at(self, cell):
        self.adventurer_cell = cell

That removes the duplication, and helps us ensure that if we ever need to change how placing the adventurer works, there will be one place and only one place to change. I’m not saying that I have reasoned to this practice, and I certainly haven’t proven that it’s better: it’s just built into how I work.

There are other direct references. I’ll fix all four. Here’s one:

    def move_adventurer_north(self):
        self.place_adventurer_at(self.adventurer_cell.north())

I notice the names dot_room etc. The adventurer is jokingly called Dot in these articles, because she displays as a big dot on the screen. But a programmer discovering those names will be just a bit disrupted by those names.

While we’re thinking about the names, I propose that we rename ‘adventurer’ to ‘player’, which is a bit more common and easier to type, a win-win. Things look like this now:

class Dungeon:
    def __init__(self, layout):
        self.layout = layout
        self.player_cell = None

    @property
    def rooms(self):
        return self.layout.rooms

    def populate(self):
        dot_room = random.choice(self.rooms)
        dot_cell = random.choice(dot_room.cells)
        self.place_player_at(dot_cell)

    def place_player_at(self, cell):
        self.player_cell = cell

    def move_adventurer_north(self):
        self.place_player_at(self.player_cell.north())
    ...

Green. Commit: tidying.

We should take a look at DungeonLayout, our new / old class. Immediately we notice that this class seems rather large. We just scan it to make that assessment, before even trying to understand it.

class DungeonLayout:
    def __init__(self):
        self.rooms = []
        self.suite_sets = []

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

    @property
    def is_fully_connected(self):
        suite = self.find_suite(self.rooms[0])
        return len(suite) == len(self.rooms)

    def ensure_connected(self):
        if self.is_fully_connected:
            return
        room = self._straight_path_room()
        self.add_room(room)

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

    def add_room(self, room):
        self.rooms.append(room)

    def remove_room(self, room):
        room.return_cells()
        self.rooms.remove(room)

    def define_suites(self):
        return [Suite(s) for s in self.define_suite_sets()]

    def define_suite_sets(self):
        self.suite_sets = []
        unexplored = self.rooms.copy()
        while unexplored:
            suite = self.find_suite(unexplored[0])
            self.suite_sets.append(suite)
            unexplored = [room for room in unexplored if room not in suite]
        return self.suite_sets

    def add_path_room(self):
        room = self._straight_path_room()
        if room:
            self.add_room(room)

    def _straight_path_room(self):
        cells = self._straight_path_cells()
        if cells:
            return Room(cells, 'path')
        else:
            return None

    def _straight_path_cells(self):
        cells = []
        suites = self.define_suites()
        for s1, s2 in zip(suites, suites[1:]):
            cells.extend(s1.find_path_cells(s2))
        return list(set(cells))

    @staticmethod
    def find_suite(room):
        suite = set()
        start = room.cells[0]
        for cell in start.generate(lambda c: c.is_in_a_room):
            suite.add(cell.room)
        return suite

Do we ever use the suite_sets member? We do, but oddly:

    def define_suite_sets(self):
        self.suite_sets = []
        unexplored = self.rooms.copy()
        while unexplored:
            suite = self.find_suite(unexplored[0])
            self.suite_sets.append(suite)
            unexplored = [room for room in unexplored if room not in suite]
        return self.suite_sets

I think that if we used a temp in that method everything would continue to work and we wouldn’t need the instance variable any more.

    def define_suite_sets(self):
        suite_sets = []
        unexplored = self.rooms.copy()
        while unexplored:
            suite = self.find_suite(unexplored[0])
            suite_sets.append(suite)
            unexplored = [room for room in unexplored if room not in suite]
        return suite_sets

Green, and main works. Remove the member. Commit.

I notice add_path_room, which has no usages. Remove it. Commit.

Did you notice?
After taking a look at the whole class, I didn’t begin by planning out how to refactor it overall. Instead, I looked for opportunities to make small changes, especially small deletions. After those low-hanging fruit are dealt with, we’ll look for larger changes. When we do that, the problem we face will be smaller and thus easier to resolve.

My next move is going to be to put methods that call each other closer to each other. Things seem a bit random here. First this case:

    def define_suite_sets(self):
        suite_sets = []
        unexplored = self.rooms.copy()
        while unexplored:
            suite = self.find_suite(unexplored[0])
            suite_sets.append(suite)
            unexplored = [room for room in unexplored if room not in suite]
        return suite_sets

    @staticmethod
    def find_suite(room):
        suite = set()
        start = room.cells[0]
        for cell in start.generate(lambda c: c.is_in_a_room):
            suite.add(cell.room)
        return suite

In the second method there, suite is a set of cells, not a Suite. (What is a Suite, anyway? I think we need to look at that.) Here, we rename …

No, I’m mistaken. suite is a set of Rooms. I do a bit of renaming and type hinting:

    def define_suites(self):
        return [Suite(s) for s in self.define_suite_sets()]

    def define_suite_sets(self):
        suites: list[set[Room]] = []
        unexplored = self.rooms.copy()
        while unexplored:
            suite_rooms: set[Room] = self.find_suite(unexplored[0])
            suites.append(suite_rooms)
            unexplored = [room for room in unexplored if room not in suite_rooms]
        return suites

    @staticmethod
    def find_suite(room) ->set[Room]:
        suite_rooms: set[Room] = set()
        start = room.cells[0]
        for cell in start.generate(lambda c: c.is_in_a_room):
            suite_rooms.add(cell.room)
        return suite_rooms

I remain curious about what a Suite is and what it does. This may all be somewhat moot. But why aren’t we returning a Suite from the bottom method here instead of that last conversion at the top?

I think this code may be a case of backing away slowly. Ah, no, that’s not quite it: there is a test for define_suite_sets and one for find_suite. First this one:

    def test_make_suite_lists(self):
        \
        #massive setup skipped here

        suite_sets = dungeon.define_suite_sets()
        assert len(suite_sets) == 3
        assert {r_0, r_1, r_3} in suite_sets
        assert {r_2} in suite_sets
        assert {r_4, r_5} in suite_sets

        suites = dungeon.define_suites()
        assert len(suites) == 3

We can change this not to use define_suite_sets by checking the suites after define_suites. It’s just a bit harder, because we can’t be sure what order they’re in. But that order won’t change so once we get the test right it should keep running.

        suites = dungeon.define_suites()
        assert len(suites) == 3
        assert suites[0].room_set == {r_0, r_1, r_3}
        assert suites[1].room_set == {r_2}
        assert suites[2].room_set == {r_4, r_5}

Green. Commit: refactoring tests in prep for object refactoring.

Now who’s calling find_suite? Ah, that’s used twice inside DungeonLayout:

    @property
    def is_fully_connected(self):
        suite = self.find_suite(self.rooms[0])
        return len(suite) == len(self.rooms)

    def define_suite_sets(self):
        suites: list[set[Room]] = []
        unexplored = self.rooms.copy()
        while unexplored:
            suite_rooms: set[Room] = self.find_suite(unexplored[0])
            suites.append(suite_rooms)
            unexplored = [room for room in unexplored if room not in suite_rooms]
        return suites

I think that if we actually start returning suites from find_suite, is_fully_connected may fail for want of len on Suite. Is there a test for that? Yes, there is. So we can proceed.

Looking at this sequence:

    def define_suites(self):
        return [Suite(s) for s in self.define_suite_sets()]

    def define_suite_sets(self):
        suites: list[set[Room]] = []
        unexplored = self.rooms.copy()
        while unexplored:
            suite_rooms: set[Room] = self.find_suite(unexplored[0])
            suites.append(suite_rooms)
            unexplored = [room for room in unexplored if room not in suite_rooms]
        return suites

    @staticmethod
    def find_suite(room) ->set[Room]:
        suite_rooms: set[Room] = set()
        start = room.cells[0]
        for cell in start.generate(lambda c: c.is_in_a_room):
            suite_rooms.add(cell.room)
        return suite_rooms

My plan is to return a Suite directly from find_suite. Makes sense, right? Then we won’t need to do it in define_suites and I think we rename define_suite_sets to define_suites. Or maybe we can inline. Let’s try that now:

First pull out a variable:

    def define_suites(self):
        sets = self.define_suite_sets()
        return [Suite(s) for s in sets]

We could commit. Now inline method:

    def define_suites(self):
        suites: list[set[Room]] = []
        unexplored = self.rooms.copy()
        while unexplored:
            suite_rooms: set[Room] = self.find_suite(unexplored[0])
            suites.append(suite_rooms)
            unexplored = [room for room in unexplored if room not in suite_rooms]
        sets = suites
        return [Suite(s) for s in sets]

    @staticmethod
    def find_suite(room) ->set[Room]:
        suite_rooms: set[Room] = set()
        start = room.cells[0]
        for cell in start.generate(lambda c: c.is_in_a_room):
            suite_rooms.add(cell.room)
        return suite_rooms

Now return a Suite from find_suite and clean up the hints. This will break a test for a moment.

    def define_suites(self):
        suites: list[Suite] = []
        unexplored = self.rooms.copy()
        while unexplored:
            suite: Suite = self.find_suite(unexplored[0])
            suites.append(suite)
            unexplored = [room for room in unexplored if room not in suite]
        return suites

    @staticmethod
    def find_suite(room) -> Suite:
        suite_rooms: set[Room] = set()
        start = room.cells[0]
        for cell in start.generate(lambda c: c.is_in_a_room):
            suite_rooms.add(cell.room)
        return Suite(suite_rooms)

Six tests break. I should have committed a save point. Quick look at the tests to see if we can fix things quickly. Ah, we’re in luck:

>           unexplored = [room for room in unexplored if room not in suite]
                                                         ^^^^^^^^^^^^^^^^^
E           TypeError: argument of type 'Suite' is not iterable

We can just fetch the rooms, or make it iterable. I think the former is more conservative.

    def define_suites(self):
        suites: list[Suite] = []
        unexplored = self.rooms.copy()
        while unexplored:
            suite: Suite = self.find_suite(unexplored[0])
            suites.append(suite)
            unexplored = [room for room in unexplored if room not in suite.room_set]
        return suites

Now we still have one test failing. It’s the one I expected:

>       return len(suite) == len(self.rooms)
               ^^^^^^^^^^
E       TypeError: object of type 'Suite' has no len()

    @property
    def is_fully_connected(self):
        suite = self.find_suite(self.rooms[0])
        return len(suite.room_set) == len(self.rooms)

That check is kind of indirect: we are fully connected if the suite containing our first room has the same length as the number of rooms in the layout. Better checks might be:

  1. Is there only one Suite in define_suites?
  2. Does the Suite containing room one contain all the rooms?

Let’s use scheme 1 here.

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

Yikes, I haven’t been committing. Commit: refactor define_suites, plus tidying.

The remaining “big” thing here is this:

    def ensure_connected(self):
        if self.is_fully_connected:
            return
        room = self._straight_path_room()
        self.add_room(room)

    def _straight_path_room(self):
        cells = self._straight_path_cells()
        if cells:
            return Room(cells, 'path')
        else:
            return None

    def _straight_path_cells(self):
        cells = []
        suites = self.define_suites()
        for s1, s2 in zip(suites, suites[1:]):
            cells.extend(s1.find_path_cells(s2))
        return list(set(cells))

We’ll let that one ride for this morning, as we are approaching the limit of our article size, our patience, and various other personal limits. I want to call out Suite, in relation to the above, since it goes like this:

class Suite:
    def __init__(self, room_set):
        self.room_set = room_set

    @property
    def cells(self):
        for room in self.room_set:
            for cell in room.cells:
                yield cell

    def find_path_cells(self, suite) -> list[Cell]:
        source, target = self.find_closest_pair(suite)
        cells = source.find_path(target, [source.room, target.room])
        on_path = cells[1:-1]
        return on_path

    def find_closest_pair(self, suite):
        my_best = None
        his_best = None
        distance = 1_000_000
        for mine in self.cells:
            for his in suite.cells:
                dist = mine.manhattan_distance(his)
                if dist < distance:
                    my_best = mine
                    his_best = his
                    distance = dist
        return [my_best, his_best]

There’s something funky going on here, with find_path and find_path_cells in Cell, plus some path-finding in Suite and some in DungeonLayout. I suspect that we may be able to find a better division of labor here, ideally something like Layout to Suite to Room to Cell, or perhaps direct from Suite to Cell, with no path-related stuff in Room at all.

All that is for another day.

Summary

It is no surprise that we found things to improve, after having moved Dungeon and DungeonLayout into place. Some of that was debris left over from our various experiments with ensuring dungeons are connected: remember that we have that interesting trick of tossing in random rooms with high wandering coefficients until everything connects. Clearly that needs to be better integrated. Some kind of path-making scheme with strategies or a selection of approaches or something.

We proceeded without a grand plan, even though the situation might well have seemed like we should have one. Instead, we kind of chip away everything that doesn’t look quite right and almost magically a better-organized set of classes emerges.

One good example of that was pushing the creation of the Suite down to the bottom of the method sequence, to the first method that had the collection needed to create a Suite, instead of collecting the collections and then Suiterizing them. That did require us to refactor a test, but it made the test both shorter and more safe.

The test got the way it was because when we originally built the Suite idea, I didn’t see clearly how to do it, so created a set of rooms first, which is necessary, but I tested that bit, which kind of nailed it in place. We had to undo that test to release the code to be what it really wanted to be.

I missed at least one possible commit, and it nearly bit me when six tests broke, but the fix turned out to be easy. (In fact, I think that PyCharn had highlighted the troublesome line, but I didn’t notice.) But committing Every Single Time we’re green is a good idea, at least here at my place.

So we have made a noticeable improvement to tests and to our Layout, Dungeon, and Suite classes. Very useful 90 minutes, including writing the article.

See you next time!