Small Refactoring

I veer into philosophy and LLM/AI, based on thinking about many small changes, and how they add to understanding.

The subordinate worker classes are currently named, e.g. RoundRoomMaker, and when they make cells, we can’t call them room makers, now, can we? In addition, whenever you create a FooRoomMaker, PyCharm guesses that you mean a subclass of RoomMaker, which messes up its prompts a bit. Good guess, that’s how I was taught to name subclasses. I think maybe we’ll call them RoundCellMaker or something like that. But first to the task of changing them. Here’s one example:

class RoomMaker:
    def __init__(self, space: CellSpace):
        self.space = space

    def round(self, radius:int, origin: Cell, name ='round'):
        return RoundRoomMaker(self.space).round(radius, origin, name)

class RoundRoomMaker:
    def __init__(self, space: CellSpace):
        self.space = space
        self.cells:list[Cell] = []

    def round(self, radius:int, origin: Cell, name =''):
        for c in origin.generate(lambda c: c.is_available and origin.distance(c) <= radius):
            self.cells.append(c)
        return Room(self.cells, origin, name)

So the change should be simple enough, and it goes like this:

class RoomMaker:
    def round(self, radius:int, origin: Cell, name ='round'):
        cells = RoundRoomMaker(self.space).round(radius, origin, name)
        return Room(cells, origin, name)

class RoundRoomMaker:
    def __init__(self, space: CellSpace):
        self.space = space
        self.cells:list[Cell] = []

    def round(self, radius:int, origin: Cell, name =''):
        for c in origin.generate(lambda c: c.is_available and origin.distance(c) <= radius):
            self.cells.append(c)
        return self.cells

That’s green. I am seeing other things that I dislike. In this particular case, there is no reason to put the cells into an instance variable. Let’s change that:

class RoundRoomMaker:
    def __init__(self, space: CellSpace):
        self.space = space

    def round(self, radius:int, origin: Cell, name =''):
        cells: list[Cell] = []
        for c in origin.generate(lambda c: c.is_available and origin.distance(c) <= radius):
            cells.append(c)
        return cells

Hm, we have no need for space. Remove it. This will kind of begin to address another thing that I didn’t like. We’ll come to that.

    def round(self, radius:int, origin: Cell, name ='round'):
        cells = RoundRoomMaker().round(radius, origin, name)
        return Room(cells, origin, name)

class RoundRoomMaker:
    def round(self, radius:int, origin: Cell, name =''):
        cells: list[Cell] = []
        for c in origin.generate(lambda c: c.is_available and origin.distance(c) <= radius):
            cells.append(c)
        return cells

We don’t need the __init__. I considered having one with just pass but decided against it. We’re green. Commit: refactoring toward RoomMaker doing instance creation.

DiamondRoomMaker doesn’t uses space either, so I change signature, remove its init, green, commit.

Now it looks like this:

class DiamondRoomMaker:
    def diamond(self, number_of_cells: int, origin: Cell, name='diamond'):
        cells = self._build_diamond(number_of_cells, origin)
        return Room(cells, origin, name)

    def _build_diamond(self, number_of_cells, origin):
        cells:list[Cell] = []
        count = 0
        for cell in origin.generate(lambda c: c.is_available):
            cells.append(cell)
            cell.room = self
            count += 1
            if count == number_of_cells:
                break
        return cells

A few steps to take here. Also I just noticed the name coming in, which is not needed. A bit careless but I was interrupted. Go back and fix signature on Round. PyCharm tells me the method could be static and I agree. I’ll mark it, mostly because we’re getting a signal here and I want to remember it. This class is down to one method that is static. Not much of a class.

Anyway green, commit. By the way, all the methods of RoomMaker can be static, and will probably remain so. The code is trying to tell me something here.

Back to Diamond. Change the call, make the test run. Then there will be an opportunity.

class DiamondRoomMaker:
    def diamond(self, number_of_cells: int, origin: Cell, name='diamond'):
        cells = self._build_diamond(number_of_cells, origin)
        return cells

    def _build_diamond(self, number_of_cells, origin):
        cells:list[Cell] = []
        count = 0
        for cell in origin.generate(lambda c: c.is_available):
            cells.append(cell)
            cell.room = self
            count += 1
            if count == number_of_cells:
                break
        return cells

This works. But let’s inline:

class DiamondRoomMaker:
    def diamond(self, number_of_cells: int, origin: Cell):
        cells:list[Cell] = []
        count = 0
        for cell in origin.generate(lambda c: c.is_available):
            cells.append(cell)
            cell.room = self
            count += 1
            if count == number_of_cells:
                break
        return cells

Green. Commit. Disappointingly, PyCharm flubbed the in-lining very badly. Bug report submitted.

Just one more, the one we started with, cave:

class RoomMaker:
    def cave(self, number_of_cells: int, origin: Cell, name =''):
        return CaveRoomMaker(self.space).cave(number_of_cells, origin, name)

class CaveRoomMaker:
    def __init__(self, space: CellSpace):
        self.space = space
        self.cells:list[Cell] = []
        self.growth_candidates:list[Cell] = []

    def cave(self, number_of_cells: int, origin: Cell, name =''):
        self._build_cave(number_of_cells, origin)
        return Room(self.cells, origin, name)

    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()
            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):
        for cell in sample(self.growth_candidates,
                           len(self.growth_candidates)):
            available = cell.available_neighbors
            if available:
                return choice(available)
            else:
                self.growth_candidates.remove(cell)
        return None

OK, this one deserves to be a class. Anyway, change RoomMaker.cave to break it, change CaveRoomMaker.cave to fix it:

class RoomMaker:
    def __init__(self, space: CellSpace):
        self.space = space

    def cave(self, number_of_cells: int, origin: Cell, name =''):
        cells = CaveRoomMaker(self.space).cave(number_of_cells, origin, name)
        return Room(cells, origin, name)

class CaveRoomMaker:
    def __init__(self, space: CellSpace):
        self.space = space
        self.cells:list[Cell] = []
        self.growth_candidates:list[Cell] = []

    def cave(self, number_of_cells: int, origin: Cell, name =''):
        self._build_cave(number_of_cells, origin)
        return self.cells

Green. Commit. We don’t use space, change signature.

I am tempted to inline take here:

    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()
            if new_cell is None:
                return

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

That reminds me of a minor kludge: these room-makers really need to “reserve” the cells they take, which is signified here by assigning its room to the maker. When a Room is created, it is given a list of cells and it unilaterally assigns all those rooms to itself, wiping out the record of the maker. It might be better to actually “reserve” the cell and when assigning, check to be sure that it is reserved, not in use by another actual Room.

I notice that RoundRoomMaker does not reserve the cells. This is OK, because we’re not multi-threading our makers, but it seems to me that we should reserve anyway.

class RoundRoomMaker:
    def round(self, radius: int, origin: Cell):
        cells: list[Cell] = []
        for c in origin.generate(lambda c: c.is_available and origin.distance(c) <= radius):
            c.room = self
            cells.append(c)
        return cells

That conveniently makes the method no longer static. I like that, it lowers a concern. Commit: RoundRoomMaker reserves cells properly. DiamondRoomMaker already does it right.

I think we’re getting to a stopping point. Let’s decide what to name these things. The convention is that they all return a collection of cells. (A list: a smarter collection might be wise, might not.)

Hm, before we do that, let’s create some duplication. Rename the called method in each maker to build.

class RoomMaker:
    def __init__(self, space: CellSpace):
        self.space = space

    def cave(self, number_of_cells: int, origin: Cell, name ='cave'):
        cells = CaveRoomMaker().build(number_of_cells, origin, name)
        return Room(cells, origin, name)

    def diamond(self, number_of_cells: int, origin: Cell, name ='diamond'):
        cells = DiamondRoomMaker().build(number_of_cells, origin)
        return Room(cells, origin, name)

    def round(self, radius:int, origin: Cell, name ='round'):
        cells = RoundRoomMaker().build(radius, origin)
        return Room(cells, origin, name)

If we ignore the fact that the initial integer parameter is number of cells in two cases and a radius in another, these methods are entirely duplicates aside from the detail of the class name. It is tempting, oh so tempting, to remove that duplication somehow. Perhaps we would call a single RoomMaker.build with an enum or string parameter, and it would look up the class name to use. Or possibly, these little classes aren’t carrying their weight. Were it not for the Cave one, which is kind of complex, I’d be arguing for only one class. But then I’d be back where I was with the one and not liking that. I like that each of these small classes just does one thing.

Enough waffling, this is better than what we had. Let’s rename the little guys though. How about CellFinder? CaveCellFinder, DiamondCellFinder? CellMaker? No, they make collections. CellCollector? CaveCellCollector, DiamondCellCollector? OK we’ll try that. They are all local to RoomMaker anyway. We’ll try those names and see if they grate on us later.

Green. Commit: done refactoring RoomMaker for now.

Let’s sum up and go see about bacon bacon bacon.

Summary

This little file is better than it was. The classes are smaller but no less clear. There are fewer methods to understand or wonder about. Names are a bit better. On the other hand, I’ve spent about an hour doing this, and perhaps a real world developer is so busy vibe coding that they would never have time to do this. In fact, of course, vibe coding is never going to do this.

Musing About the Work

My work here is of course intentionally detailed: I am in the business of creating and writing about artisanal hand-crafted code that starts out rough and gets smoother over time. This is a rare profession: in the particular style that I use, rough getting better, I may be the only person doing it.

If I were working for a living developing code, would I do the same thing? Well, in fact, I would, because I’d be working on a code base that needed improvement over a long period of time, and the only way to keep that base alive is to continually weed it and clean it up. I have been on projects where the code became unmanageable and there is no good way out of that situation, only more or less bad ways.

But there may be very few teams that work toward keeping the code base fresh all the time: I don’t know, none of them ever drop me a line. But I think the future of “AI-written” code is fraught.

When I work the way I do, I build up a deep understanding of what the code does, and I embed as much of that understanding as I can back into the code, with good names, small classes and methods, and so on. So I, as the decision-maker about how to make necessary changes, am equipped to make those decisions.

Where is the knowledge in a vibe-coded, AI-written code base? It sure isn’t in the heads of the prompters. Some people are even suggesting that we don’t need developers at all, just product managers who prompt “AIs”. All due credit to product managers, they are not equipped to understand details of design even if they had the details and if the “AI” does it … where are the details?

Guess what: it isn’t in the “AI” either. The “AI” doesn’t have concepts, and it doesn’t remember things in terms of concepts. That’s why you have to keep reminding them of things. So next month, next year, when it’s time to make changes, the “AI” isn’t going to remember all the details that it was given, even if at one point it was given all the necessary details.

We are, I suspect, on the verge of a period where we have more software, which is less carefully crafted, and far less well understood than we have now — and let’s face it, a lot of what is out there now is not well understood.

On top of the environmental impact, on top of the pumping of more money into the oligarchy who own the giant corporations, on top of the massive job losses already created, and which will be created, software is very likely going to get worse, in some very nasty ways, such as security.

There may even come a day when we few artisanal software creators will be sought after. It could happen.