We couldn’t get in the straightforward way. Is there a more sneaky way to do this? Tiny steps, many commits, very tedious. One semi-interesting refactoring.

We’re trying to get the room out of Cell. Replacing this:

class DungeonLayout:
    def available_neighbors(self, cell):
        return [neighbor
                for neighbor in self.neighbors(cell)
                if neighbor.is_available]

… with this, which is supposed to be equivalent:

class DungeonLayout:
    def available_neighbors(self, cell):
        return [neighbor
                for neighbor in self.neighbors(cell)
                if self.is_available(neighbor)]

… breaks tests. The reason seems to be that there’s code like this going on:

class RoomMaker:
    def _build_cave(self, number_of_cells, origin):
        new_cell: Cell = origin
        for _ in range(number_of_cells):
            self.take(new_cell)
            self.growth_candidates.append(new_cell)
            new_cell = self.find_adjacent_cell(new_cell)
            if new_cell is None:
                return

    def take(self, new_cell: Cell):
        self.cells.append(new_cell)
        new_cell.room = self

    def find_adjacent_cell(self, previous):
        if random.random() < 1.0:
            available = self.layout.available_neighbors(previous)
            if available:
                return choice(available)
        for cell in sample(self.growth_candidates,
                           len(self.growth_candidates)):
            available = self.layout.available_neighbors(cell)
            if available:
                return choice(available)
            else:
                self.growth_candidates.remove(cell)
        return None

In the code above, we are accumulating Cells in self.cells, and relying on the layout to know whether they are available. Normally it would not know that, but because we have bashed self into the cell, the Cell code does know that the cell shouldn’t be treated as available.

Let’s first not do the bashing, and then make things work. I’ve reset and we are green.

I remove that line from the take and we continue to pass. I’m looking in the wrong place. Commit anyway, because we are green.

The same code exists in CaveCellCollector. The above was ExperimentalCellCollector which—for shame—has no tests. Note made. Same change in Cave… should break, and it does.

Now to fix this:

    def find_adjacent_cell(self):
        for cell in sample(self.growth_candidates,
                           len(self.growth_candidates)):
            available = self.layout.available_neighbors(cell)
            if available:
                return choice(available)
            else:
                self.growth_candidates.remove(cell)
        return None

I think this should fix it:

    def find_adjacent_cell(self):
        for cell in sample(self.growth_candidates,
                           len(self.growth_candidates)):
            available = [cell
                         for cell in self.layout.available_neighbors(cell)
                         if cell not in self.cells]
            if available:
                return choice(available)
            else:
                self.growth_candidates.remove(cell)
        return None

That is in fact the righteous code: we are keeping a secret collection of cells that we intend to put into a Room later. We are green and commit.

Now about there being no tests for Experimental..., we could repurpose one that calls cave to call experimental, or duplicate one. Let’s do that, because I think it should fail without the same fix as above.

I do exactly that, just copying a test and calling experimental, and get the failure. Make the fix, which I have to use twice:

    def find_adjacent_cell(self, previous):
        available = [cell
                     for cell in self.layout.available_neighbors(previous)
                     if cell not in self.cells]
        if available:
            return choice(available)
        for cell in sample(self.growth_candidates,
                           len(self.growth_candidates)):
            available = [cell for cell in self.layout.available_neighbors(cell)
                         if cell not in self.cells]
            if available:
                return choice(available)
            else:
                self.growth_candidates.remove(cell)
        return None

Green. Commit. Let’s remove some duplication here:

    def find_adjacent_cell(self, previous):
        available = self.available_and_not_reserved(previous)
        if available:
            return choice(available)
        for cell in sample(self.growth_candidates,
                           len(self.growth_candidates)):
            available = self.available_and_not_reserved(cell)
            if available:
                return choice(available)
            else:
                self.growth_candidates.remove(cell)
        return None

    def available_and_not_reserved(self, previous):
        return [cell
                for cell in self.layout.available_neighbors(previous)
                if cell not in self.cells]

Let’s rename previous in the parameter.

    def available_and_not_reserved(self, some_cell):
        return [cell
                for cell in self.layout.available_neighbors(some_cell)
                if cell not in self.cells]

Commit. Should have already done once.

I think that larger method could use a little improvement. It’s really rather tricky. We have two ways of trying to get a non-empty list of available cells, and we return choice of that list, otherwise None if we find nothing. We’ll add an else ahead of the for:

    def find_adjacent_cell(self, previous):
        available = self.available_and_not_reserved(previous)
        if available:
            return choice(available)
        else:
            for cell in sample(self.growth_candidates,
                               len(self.growth_candidates)):
                available = self.available_and_not_reserved(cell)
                if available:
                    return choice(available)
                else:
                    self.growth_candidates.remove(cell)
        return None

THen just save available instead of returning:

    def find_adjacent_cell(self, previous):
        available = self.available_and_not_reserved(previous)
        if available:
            pass
        else:
            for cell in sample(self.growth_candidates,
                               len(self.growth_candidates)):
                available = self.available_and_not_reserved(cell)
                if available:
                    break
                else:
                    self.growth_candidates.remove(cell)
        return choice(available) if available else None

Remove the pass bit:

    def find_adjacent_cell(self, previous):
        available = self.available_and_not_reserved(previous)
        if not available:
            for cell in sample(self.growth_candidates,
                               len(self.growth_candidates)):
                available = self.available_and_not_reserved(cell)
                if available:
                    break
                else:
                    self.growth_candidates.remove(cell)
        return choice(available) if available else None

Can we extract the for loop usefully? I suspect PyCharm will refuse. Right, it wants to pass available in as a parameter. I don’t think I like that. With a little help from me, we have this:

    def find_adjacent_cell(self, previous):
        available = self.available_and_not_reserved(previous)
        if not available:
            available = self.find_candidate_with_useful_neighbors()
        return choice(available) if available else None

    def find_candidate_with_useful_neighbors(self):
        available = []
        for cell in sample(self.growth_candidates,
                           len(self.growth_candidates)):
            available = self.available_and_not_reserved(cell)
            if available:
                break
            else:
                self.growth_candidates.remove(cell)
        return available

Let’s invert that if.

    def find_candidate_with_useful_neighbors(self):
        available = []
        for cell in sample(self.growth_candidates,
                           len(self.growth_candidates)):
            available = self.available_and_not_reserved(cell)
            if not available:
                self.growth_candidates.remove(cell)
            else:
                break
        return available

I think we’re as good as we’re going to get, and anyway this is the experimental one. Commit, then come up for air.

The main thing we did toward progress here was to remove the shady use of Cell.room to signify that a cell was in use, instead looking into the object’s local collection of soon-to-be-room cells.

Let’s see who else is setting Cell.room. There are a lot. We will have to back them out incrementally, which is fine, because we like to proceed incrementally anyway.

Massive Deletion Here

I’m removing about a billion lines of replacing cell.something with layout.something(cell) and adjusting tests to always add test rooms to the Layout. Nothing to see here.

The only interesting thing was that I mis-typed a line in a test and spent an hour trying to debug the code and finally reset and found the problem instantly.

Summary

Bottom line is a dozen commits this afternoon, each one fixing one or a few references to Cell.room, or cell.is_available, adjusting tests to add the rooms if needed, going green, and committing.

What this shows, and I don’t claim this is a big discovery, is that we could do this removal of room from Cell over the course of days or weeks, removing just a few occurrences when we had time.

We all know that I believe this is always possible, and I’m sure you all trust that if I ever get to a point where I can’t find a way to do a “big” refactoring in small steps, I’ll write it up. So far, that has not happened.

I think I’ll do a bunch of the rest without writing anything, unless it’s inherently interesting somehow.

See you next time!