Let’s see if we can make our SuiteFinder object more expressive. Arguably, it needs it. Arguably, we’ll go too far. Too far? We went hog-wild!

One of Kent Beck’s Rules of Simple Design says that the code should express all of our ideas about the code. I don’t think that includes “Who the **** wrote this???”. He had in mind design ideas, if I’m not mistaken. Our SuiteFinder is compact, but there are a lot of ideas embedded in there and not expressed.

So today we’ll go as far as we can toward making little SuiteFinder more expressive. Here it is now:

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

    def find_suite(self):
        while self.frontier:
            cell = self.frontier.pop(0)
            for neighbor_cell in cell.neighbors_in_rooms():
                if neighbor_cell not in self.reached:
                    self.suite.add(neighbor_cell.room())
                    self.reached.add(neighbor_cell)
                    self.frontier.append(neighbor_cell)
        return self.suite

I propose to go way too far with this, and I’m not sure myself how much I’ll like the result. My basic notion is to take every line from this method, and try to make it more expressive.

Let’s begin. Note that under our if above, there are multiple statements. We like to have methods that make decisions, and methods that do things, and not so many methods that do both. So we’ll extract those three lines. What are they doing? They are processing a newly-discovered cell.

    def find_suite(self):
        while self.frontier:
            cell = self.frontier.pop(0)
            for neighbor_cell in cell.neighbors_in_rooms():
                if neighbor_cell not in self.reached:
                    self.process_newly_discovered_cell(neighbor_cell)
        return self.suite

    def process_newly_discovered_cell(self, new_cell):
        self.suite.add(new_cell.room())
        self.reached.add(new_cell)
        self.frontier.append(new_cell)

Having come up with the notion of “newly_discovered” induces me to move a line and rename reached:

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

    def find_suite(self):
        while self.frontier:
            cell = self.frontier.pop(0)
            for neighbor_cell in cell.neighbors_in_rooms():
                if neighbor_cell not in self.discovered:
                    self.process_newly_discovered_cell(neighbor_cell)
        return self.suite

    def process_newly_discovered_cell(self, new_cell):
        self.discovered.add(new_cell)
        self.suite.add(new_cell.room())
        self.frontier.append(new_cell)

I forgot to commit. I plan to see the tests green at every point this morning, and to commit: “improving expression” every time. Done.

So many opportunities. Let’s see … let’s rename frontier. It contains known cells that have not yet been examined. That’s too long a name. Let’s try unexamined. Ah but then we want to rename discovered to examined. Do both. That induces me, all in the same thought segment, to rename our new method accordingly:

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

    def find_suite(self):
        while self.unexamined:
            cell = self.unexamined.pop(0)
            for neighbor_cell in cell.neighbors_in_rooms():
                if neighbor_cell not in self.examined:
                    self.examine(neighbor_cell)
        return self.suite

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

Commit.

For some reason, I want to reorder the examine method’s three lines:

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

Commit. Why did I want to do that? Because those two lines seem to contradict each other. unexamined, formerly frontier, is the collection of cells whose neighbors need to be explored. It’s the cells from which we want to explore further. For now, I’ll rename it to ex;plore_further. I’ll spare you the full class paste, we’ll see the new name soon enough.

We give this a hard look:

    def find_suite(self):
        while self.explore_further:
            cell = self.explore_further.pop(0)
            for neighbor_cell in cell.neighbors_in_rooms():
                if neighbor_cell not in self.examined:
                    self.examine(neighbor_cell)
        return self.suite

That if statement with its subordinate statement. What is that? It examines any cell that has not already been examined. examine_cells_as_needed? examine_fresh_cells? examine_unvisited? I hesitate to offer examine_unexamined.

I often find conditions hard to name because, well, they are conditional and saying examine_if_not_already_examined seems not to be all that helpful.

Ah! What if the for loop never saw examined cells. Extract a method there:

    def find_suite(self):
        while self.explore_further:
            cell = self.explore_further.pop(0)
            for neighbor_cell in self.unexamined_neighbors(cell):
                if neighbor_cell not in self.examined:
                    self.examine(neighbor_cell)
        return self.suite

    def unexamined_neighbors(self, cell) -> Any:
        return cell.neighbors_in_rooms()

Now I have to actually do some work rather than let PyCharm have all the fun.

    def find_suite(self):
        while self.explore_further:
            cell = self.explore_further.pop(0)
            for neighbor_cell in self.unexamined_neighbors(cell):
                self.examine(neighbor_cell)
        return self.suite

    def unexamined_neighbors(self, cell) -> Any:
        return (cell
                for cell in cell.neighbors_in_rooms()
                if cell not in self.examined)

Commit.

Rename neighbor_cell, I think:

    def find_suite(self):
        while self.explore_further:
            cell = self.explore_further.pop(0)
            for unexamined in self.unexamined_neighbors(cell):
                self.examine(unexamined)
        return self.suite

We could extract the entire inside of the while, and we are going for the extreme here, so here goes:

    def find_suite(self):
        while self.explore_further:
            self.examine_next_unexamined_cell()
        return self.suite

    def examine_next_unexamined_cell(self):
        cell = self.explore_further.pop(0)
        for unexamined in self.unexamined_neighbors(cell):
            self.examine(unexamined)

Commit, but we already see an improvement, that word explore could be better.

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

    def find_suite(self):
        while self.examine_further:
            self.examine_next_unexamined_cell()
        return self.suite

    def examine_next_unexamined_cell(self):
        cell = self.examine_further.pop(0)
        for unexamined in self.unexamined_neighbors(cell):
            self.examine(unexamined)

Commit.

Let’s go wild here, if we weren’t already. That cell = line could be better. I see two steps. First a method covering the pop:

    def examine_next_unexamined_cell(self):
        cell = self.next_cell_to_examine()
        for unexamined in self.unexamined_neighbors(cell):
            self.examine(unexamined)

    def next_cell_to_examine(self):
        return self.examine_further.pop(0)

Then, I think, inline:

    def examine_next_unexamined_cell(self):
        for unexamined in self.unexamined_neighbors(self.next_cell_to_examine()):
            self.examine(unexamined)

No, that line becomes unbearably long. Undo that.

Reflection

I think I’m expressed out. Let’s see what we have wrought here:

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

    def find_suite(self):
        while self.examine_further:
            self.examine_next_unexamined_cell()
        return self.suite

    def examine_next_unexamined_cell(self):
        cell = self.next_cell_to_examine()
        for unexamined in self.unexamined_neighbors(cell):
            self.examine(unexamined)

    def next_cell_to_examine(self):
        return self.examine_further.pop(0)

    def unexamined_neighbors(self, cell):
        return (cell
                for cell in cell.neighbors_in_rooms()
                if cell not in self.examined)

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

With all that, we have added 13 lines to a 15 line class. I would say that, so far, I’ve expressed most of the ideas that I have about this code, and that I am still not satisfied that some of those are expressed as well as I’d like.

  • The basic algorithm is a standard elementary flood fill. At least some readers would be familiar with that notion and it is not expressed here.

  • The whole purpose of the thing is to produce that list of rooms that is accumulated in self.suite. That’s somewhat expressed in the find_suite method but the actual update is kind of buried in examine. I think we might want to split that method further, into the housekeeping part that updates examined and examine_forther, and the part that updates suite.

  • The difference between examined and examine_further isn’t as well expressed as I’d like. The former collection is there to avoid searching beyond a cell that has already been discovered and dealt with. The latter is there to list cells that have not been searched beyond at all.

  • I suspect that there are really two objects trying to exist here, or at least two separable ideas, the notion of flooding all the adjacent rooms, and the notion of recording just the rooms involved, even though we must flood all the cells to do it.

And of course, there is the matter of doing this much refactoring, or doing it at all. There are many programmers who are used to huge functions, huge methods, huge classes, and they often find it difficult to deal with classes like this one, where the code skips around calling methods that call methods that call methods.

What we who lean toward this style are trying to do is not to drill down through the details, instead providing names that invite us to understand what is going to happen and only dig down if we’re actually working on that code rather than just passing by in a quest for something else.

In that light, I have to question our top-level method:

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

    def find_suite(self):
        while self.examine_further:
            self.examine_next_unexamined_cell()
        return self.suite

What is a suite? The code doesn’t say and the only other hint is really down at the very bottom, and it’s not very helpful:

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

Darn. Over 300 lines in this article and I’m still not satisfied. Let me try one more change …

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

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

Maybe. I’ll put a final copy of the code after the summary. Which I’ll write right now.

No, wait, one more try:

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

Maybe better and it seems to fit in my code margins. Possibly silly?

Summary

YMMV. By all means, you may prefer something like this or you may hate it with a passion formerly reserved for whatever you formerly reserved your hatred for. (If I were to advise, I would advise against hatred. As someone said, hatred is like drinking poison and hoping the other person dies. But I digress.)

As I reflect above, I am not entirely satisfied with what we have here, as I do have design ideas that are not expressed, and some of what is expressed is still not expressed as well as I might like. However, I have come to prefer classes with very small methods consisting of just one or two or three lines, and of late, I have been leaning away from that preference a bit, perhaps from a sense of needing to hurry, or just because the first way I see to do something is pretty procedural or because what I look up to find a way is like that. That’s certainly the case with the flood fill, which I adapted from Red Blob Games, by which I mean reused, by which I mean lifted almost verbatim like some kind of graying fleshy LLM or something. Of course Amit did post all that stuff for us to learn from and use, so I don’t feel at all badly about that.

If you have stuck with me thus far, and have an opinion you’d like to express, I welcome messages on Mastodon, or emails. Knocks at the door are not so welcome and may be met with deadly … silence, as I ignore such things quite often. I jest. But no, please don’t stop by unannounced. Toots, emails, that’s the ticket.

So this was fun for me and if you read it, I hope it was something like fun for you as well. See you next time!


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

    def examine_next_unexamined_cell(self):
        cell = self.next_cell_to_examine()
        for unexamined in self.unexamined_neighbors(cell):
            self.examine(unexamined)

    def next_cell_to_examine(self):
        return self.examine_further.pop(0)

    def unexamined_neighbors(self, cell):
        return (cell
                for cell in cell.neighbors_in_rooms()
                if cell not in self.examined)

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