More Slogging

We’re engaged in a long series of small steps, moving capability from the Cell up to the classes that hold onto Cells. The good news: small steps can be done over a long period of time. Bad: it is often not very interesting. Did not use the word "gallimaufry". Could have.

In this article, I’ll try to focus on any interesting bits that do arise. The less-interesting items might get a small mention or a single example from a batch of similar changes.

Let me emphasize what I think is the key discovery behind all this step-by-step slogging:

No Need for Large Refactorings!

I’ve encountered many teams whose design got away from them, and they concluded that the only hope was a huge rewrite or a very long refactoring with no progress in capability. I have fallen into that trap myself.

After years of programming myself into trouble, I now believe that almost¹ any design change can be done in small steps, spread over time, without getting in the way of adding capability to the program.

We are making a very radical change to our design, from one where the Cells knew all the interesting things about themselves and their neighbors, and one were the Cells only know their own business and all the group-related knowledge is stored in DungeonLayer, Room, or such other classes as we may from time to time create.

We have made that radical change in very small steps, tens of them, each one of which left the program working and running all its tests.

This amazingly verbose site is full of examples of design evolution. I am confident that small steps can do the job.

Don’t Be Stupid

Of course, it is possible to wait too long, ignoring problems, until they are overwhelming. It is certainly possible to obfuscate a program to the point where all meaning has been erased. Such a program might well serve as a counter-example to my thesis.

But if our mission is to produce a program that can grow and change as needed, obfuscating the code, or waiting for ages before addressing the design problems we see, would be, well, a very bad idea.

That’s My Story

… and I’m sticking to it. Now let me get to work here.

What We’re Up To

The Cell object includes an instance variable called room, which is intended to contain the Room instance in which the Cell resides. (It is sometimes used otherwise, which is an abomination for which I apologize: it seemed to make sense at the time.) We desire to get rid of that instance variable. When that is done, the Cell will be little more than a coordinate pair saying where it is in the DungeonLayout.

Our general approach mostly comes down to identifying a class of senders of room, and providing a corresponding capability in DungeonLayout (or occasionally some other class), and then, step by step, refactoring those senders to use the new capability.

In some cases we have arranged for a number of DungeonLayout’s helper classes to have a pointer to their layout and to use it to call Layout’s methods. In other cases, we pass the layout to a method that would previously have used the cell’s capability, enabling it to use that of the layout.

A Criticism

I think there is a very valid criticism of the situation so far. While Cell class is getting much simpler, DungeonLayout is getting to be a repository for all kinds of odd methods. In other articles I have called this a Horrendous Mess™, and I think that’s fair.

I Answer That

The previous situation had the HM™ at the bottom, in Cell, with nowhere to go and everyone depending on it. The new situation has the HM™ at the top. I predict that it will be straightforward to divide up the HM™ among smaller objects, resulting in higher cohesion and lower coupling and all that jazz.

I could be wrong — I frequently am. We’ll see.

Round Tuit

Let’s see what we might actually do this morning. Looking at Cell, we see these three properties that we’d like to be rid of:

class Cell:
    @property
    def room(self):
        return self._room

    @room.setter
    def room(self, room):
        self._room = room

    @property
    def is_available(self) -> bool:
        return self.room is None

I would have hoped that no one was sending is_available to Cell. Look for senders. Here’s one:

class Dungeon:
    def place_player_at_offset(self, cell, offset):
        new_cell = self.layout.at_offset(cell.xy, offset)
        if new_cell is None or new_cell.is_available:
            new_cell = cell
        self.place_player_at(new_cell)

We have a replacement method ready to use:

    def place_player_at_offset(self, cell, offset):
        new_cell = self.layout.at_offset(cell.xy, offset)
        if new_cell is None or self.layout.is_available(new_cell):
            new_cell = cell
        self.place_player_at(new_cell)

Green. Commit: removing users of Cell.is_available. There is one other, in the layout itself:

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

When we make the obvious change, a test breaks. It is not following our required protocol, which is that you have to actually create a Room and put it in the layout, not just fling random integers into Cell.room:

    def test_constrained_room(self):
        random.seed()
        layout = DungeonLayout(10, 10)
        for c in range(10):
            cell = layout.at(0, c)
            cell.room = 666
            cell1 = layout.at(9, c)
            cell1.room = 666
            cell2 = layout.at(c, 0)
            cell2.room = 666
            cell3 = layout.at(c, 9)
            cell3.room = 666
        origin = layout.at(5, 5)
        size = 100
        room = RoomMaker(layout).cave(number_of_cells=size, origin=origin)
        assert len(room.cells) == 64

I think that is drawing a square room around the outside edge of the 10x10 layout. We’ll accumulate all the cells:

    def test_constrained_room(self):
        random.seed()
        layout = DungeonLayout(10, 10)
        cells = []
        for c in range(10):
            cell = layout.at(0, c)
            cell1 = layout.at(9, c)
            cell2 = layout.at(c, 0)
            cell3 = layout.at(c, 9)
            cells.extend([cell, cell1, cell2, cell3])
        layout.add_room(Room(cells, '666'))
        origin = layout.at(5, 5)
        size = 100
        room = RoomMaker(layout).cave(number_of_cells=size, origin=origin)
        assert len(room.cells) == 64

We just saved up all the cells, built a Room like you’re supposed to, added it to the layout like you’re supposed to, and the test is green. Commit.

Reflection

It happens that this was a test that we had to change. It could have been production code just as well. Either way, a simple change in one method takes a step toward our objective. I think we can remove that is_available property now. Green. Commit: removing Cell.is_available.

Carrying On

Now we’ll look for senders of our room property. One is a test, converted to use its layout. Committed. Another similar. Commit. Yet another similar. Commit. Still another. Commit.

We needed the standard change in Suite, here:

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

Commit.

The only remaining reference is in Room, which inits its cells’ room:

class Room:
    def __init__(self, cells, name=''):
        self._cells:set[Cell] = set(cells)
        self.name = name
        self.color = "grey22"
        for cell in cells:
            cell.room = self

Remove those last two lines. Green. Commit. Return to Cell. Remove the member variable and properties. Green. Commit: Cell no longer knows its room! Callooh! Callay!

Success! We have trimmed Cell down from our largest class to just this much:

class Cell:
    def __init__(self, x, y):
        self._xy = (x,y)

    def __repr__(self):
        return f"Cell({self.x}, {self.y})"

    @property
    def x(self):
        return self._xy[0]

    @property
    def y(self):
        return self._xy[1]

    @property
    def xy(self):
        return self._xy

    def vector_diff(self, target):
        return self.x - target.x, self.y - target.y

    def manhattan_distance(self, target):
        return abs(self.x - target.x) + abs(self.y - target.y)

    def distance(self, target):
        return math.hypot(*self.vector_diff(target))

We’ll call it a day and celebrate with our traditional granola bar and iced Chai latte.

Summary

Over 30 commits just today and yesterday. Well over a hundred since we started cleaning up Cell. Each time, we had green tests, the program working.²

We have moved from a design which seemed obviously wrong, with too much weight being borne by Cell, to one which is more nearly right by our design standards and those of our friends. But “more nearly” is key here. There are now just over 200 lines in DungeonLayout, and, perhaps worse, its methods are in no particular order, so that it looks even more like a hodgepodge than it actually is — and it actually is a hodgepodge, a veritable motley patchwork, indeed a jumbled farrago of miscellany.

In addition, DungeonLayout is a bit messy.

We will, of course, and you can count on this, be improving it as time goes on.

See you next time!