Our cell generator looks pretty good. Let’s test it on a copy of our suite test, then see how to put it in play.

Along the way, we’ll of course be looking for ways to improve the generator, or whatever else we may stumble across. Let’s begin by moving the existing generator to the “src” tree. I think we’ll call the file “flood”, and probably rename generate to flood as well. Or maybe not.

Commit: move generate to flood,py in src.

Let’s see how things are done now, to decide what to do and where to do it. In Dungeon we have:

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

SuiteFinder, you’ll recall, is that class that we refactored down to flinders a day or so ago. It starts out like this:

class SuiteFinder:
    def __init__(self, room):
        self.suite = {room}
        self.examine_further = [room.cells[0]]
        self.examined = set()

    def find_suite(self):
        self.build_suite_of_all_rooms_adjacent_to_starting_room()
        return self.suite

    def build_suite_of_all_rooms_adjacent_to_starting_room(self):
        while self.examine_further:
            self.examine_next_unexamined_cell()

And then it goes along through a raft of other methods and finally, at the very bottom:

    def examine(self, new_cell):
        self.examined.add(new_cell)
        self.examine_further.append(new_cell)
        self.suite.add(new_cell.room())

That very last line is the purpose of the entire class: adding each newly discovered room to the suite. To tell the truth, which I prefer to do because it requires less record-keeping, it seems quite wrong to have the single point of a class occur in the very last of 32 lines.

I don’t want to mess with that class, at least not yet. Let’s write a test for our new generator that replicates the behavior of our test for SuiteFinder. I’ll start with a blatant copy and then remove the SuiteFinder reference. I copy it over to the generator test file, and it happily passes there as well as in its original home. I love it when I get feedback that things are going well.

Here is the crux of that test:

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

After setting up with 20 lines or so, carefully placing rooms, there’s just that. My cunning plan is to replace that first line with open code that does the …

I was going to say that does the suite-finding job but that is a two part thing. We are going to use the generator only to determine one suite at a time. So I’ll trim down this test to just find one suite.

    def test_make_suite(self):
        space = CellSpace(10, 10)
        dungeon = Dungeon()
        o_0 = space.at(3,1)
        r_0 = Room(space, 1, o_0, '0')
        dungeon.add_room(r_0)

        o_1 = space.at(4,1)
        r_1 = Room(space, 1, o_1, '1')
        dungeon.add_room(r_1)

        o_3 = space.at(5,1)
        r_3 = Room(space, 1, o_3, '3')
        dungeon.add_room(r_3)

        suite = set()
        # then a miracle occurs
        assert suite == {r_0, r_1, r_3}

I think we just need to set up our generator to start at cell o_0 and give it the right condition and we’ll be all set. The condition we want is one we have used before, is_in_a_room, because we want to search all the adjacent cells to our starting one, so long as they are in some room.

        suite = set()
        for cell in generate(o_0, lambda c: c.is_in_a_room()):
            suite.add(cell.room())
        assert suite == {r_0, r_1, r_3}

The test goes green. I am not surprised, but I am certainly pleased.

The question arises: given this nice new thing, where should we really put it and access it? It seems to me that it might best be a method on Cell class. It starts with a Cell and it only yields Cells. Sounds like Cell to me.

I think we might like to say the following, and I’ve edited the test to say it:

        suite = set()
        for cell in o_0.generate(lambda c: c.is_in_a_room()):
            suite.add(cell.room())
        assert suite == {r_0, r_1, r_3}

Let’s give Cell a generate:

    def generate(self, cond):
        return generate(self, cond)

Green. I’m going to move generate to “cell.py”. Green. Commit: adding generate to cell.

Now let’s see if we can modify Dungeon to use cell.generate. I think that means we should change SuuiteFinder, not Dungeon, because:

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

First I jam it in:

class SuiteFinder:
    def __init__(self, room):
        self.suite = {room}
        self.examine_further = [room.cells[0]]
        self.examined = set()

    def find_suite(self):
        start = self.examine_further[0]
        for cell in start.generate(lambda c: c.is_in_a_room()):
            self.suite.add(cell.room())
        return self.suite

I wanted to make minimal changes and that was the best I could come up with. We are green. Commit: refactoring SuiteFinder to use cell.generate.

Remove all those lovely refactored methods from SuiteFinder, as they are no longer called. Commit.

Now all we really need is a cell from the room. But I don’t see any reason not to generate the suite right on creation … we’ll leave that for now, I guess. That leaves us with this:

class SuiteFinder:
    def __init__(self, room):
        self.start = room.cells[0]
        self.suite = set()

    def find_suite(self):
        for cell in self.start.generate(lambda c: c.is_in_a_room()):
            self.suite.add(cell.room())
        return self.suite

Commit. Back in Dungeon, we have this:

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

At this point one has to ask oneself whether SuiteFinder is carrying its own weight. We could replace it with a method in Dungeon, something like this:

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

    def find_suite(self, 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

The only clue that this might be amiss is that find_suite does not refer to self and PyCharm points out that it could be static. Is that signal strong enough to justify a separate class with just an init and one operational method that looks like what we have here? I say no.

Leave this, mark the method static, remove SuiteFinder and its tests. There are no tests, since the tests all drove Dungeon, which now works the new way.

Commit: removing SuiteFinder as unused.

I think we’re about done here but before we sum up, let’s take a look at PathFinder class, which I think has some flood action in it:

class PathFinder:
    def __init__(self, bank):
        self.bank = bank
        self.queue: list[Cell] = []
        self.reached: dict[Cell, Cell] = dict()
        self.room_list = []

    def find_path_between_rooms(self, source_room: Room, target_room: Room):
        self.room_list = [source_room, target_room]
        return self.find_path(source_room.origin, target_room.origin)

    def find_path(self, source: Cell, target: Cell) -> list[Cell]:
        self.put(source, None)
        self.flood(target)
        path = []
        start = target
        while start is not None:
            path.append(start)
            start = self.reached.get(start)
        return path

    def flood(self, target):
        while self.queue:
            self.expand(target)

    def expand(self, target):
        reached = self.reached
        next_cell = self.queue.pop(0)
        for neighbor in next_cell.neighbors:
            if neighbor not in reached and self.can_use(neighbor):
                self.put(neighbor, next_cell)
            if neighbor == target: # <====
                self.queue = []
                return

    def can_use(self, neighbor) -> bool:
        if neighbor.is_available():
            return True
        for room in self.room_list:
            if neighbor in room:
                return True
        return False

    def put(self, cell: Cell, former=None):
        self.queue.append(cell)
        self.reached[cell] = former

Yes, it is doing a cell-based flood, suggesting that it could perhaps use our generate. Note however the arrow in expand. I’m not sure whether we could fit checking that into the generate condition, but I don’t think so: it terminates the searching early. The thing corresponding to generate’s condition is can_use, which I think we could replicate.

However, since we have a generator in generate, if we just stopped calling it, it would work just fine.

I suspect that we can at least simplify PathFinder using generate, and, who knows, perhaps replace it as we did with SuiteFinder. We’ll leave that for another session. Let’s sum up.

Summary

In an astounding flurry, over the past few days, we have implemented suite finding, refactored it down to nubbins, then built a generator sample, then showed that we could find suites with it, and finally replaced SuiteFinder altogether, with just a new (static) method in Dungeon.

Perfection is not an option.

Was that a massive waste of time? Well, compared to a timeline that didn’t happen, where somehow I knew on the first day that a generator was just the thing, and somehow saw how to structure the more complicated suite-finding operation into a series of calls to a cell generator that accepted an arbitrary condition on the cells generated, yes, it was a waste of time.

Compared to some platonic idea approach, this one wandered a bit. But I only thought of generator because I had immersed myself in improving the SuiteFinding code. I grant that that was an exercise to show what happens when you refactor down to tiny slivers of code, but it was that exercise that caused me to think of alternatives like a generator.

I don’t think it’s reasonable to expect that we’d think of this very compact solution right off the bat.

Waiting is an option.

But there is another possible timeline to consider: the one where we left SuiteFinder alone. After we got it working, there it was, a nice little 23 line class with a single purpose and a brave dedication to doing that task well. We could have stopped there.

If we had done that, we would have SuiteFinder and PathFinder and perhaps at some future time OtherThingFinders, and they’d have been similar, and we might have found that commonality and refactored to get it out. We might have wound up with Cell.generate. We might have wound up with something else, nearly or equally as good. Maybe even better, who knows?

I think a more likely outcome is that we would have wound up with a number of Finder-like classes, similar but slightly different, created sometimes by copy-pasta, perhaps sometimes written from scratch. That wouldn’t be awful, but it would be a bit of bloat in the code base.

As we stand, we have a very good shot at reducing bloat: we have already reduced it some, and PathFinder is likely to get improved if we try. But had we not done the refactoring, we might have done something similar later on, and even if not, it wouldn’t be the end of the world.

There are many paths.

We do need to keep the code from too much bloat, so we do need to take opportunities to improve things from time to time. Here chez Ron, I try to take most of the opportunities to improve the code, because my purpose is to show what happens, and hopefully to show that we can improve things safely and in small steps.

I take those opportunities so often because I think that’s an important thing to know: it’s not code and done, it’s change and change again and again and again, ad infinitum.

What should you do?

I’m not here to tell you that. I’m not your boss or even your uncle. I’m just here to do things and show you what happens. You get to decide what to do and when to do it. Even if I can only serve as a bad example, I’m OK with that.

I am pleased.

What happened over the past few days was, in my view, quite nice. We saw some decent code, we looked at ways to make it “better” by various lights, and we have wound up (so far) with a nice littler multi-purpose generator all packaged up in a single method.

I’m quite pleased with that … and please, if you enjoy these things, toot me a toot on Mastodon or something. And if you don’t … well, toot me anyway. Maybe tell me what would interest you more.

See you next time!