What happens when right in the middle of things, what seems like a much better idea appears? Includes: musing on the balance between code quality and shipping.

Ward Cunningham’s original definition of Technical Debt describes the situation where the code we have does not fully reflect the understanding we now have about the problem and solution. We’ve done the best we knewL it’s just that now we know more, and that knowledge is not reflected in the code.

Similarly, Kent Beck’s rules of simple design prescribe code that runs all our tests, expresses all our design ideas, contains no duplication, and minimizes the number of programming entities.

Both of these notions ask us to have all our design knowledge reflected in the code, and, it should go without saying, we want that knowledge to be reflected clearly and cleanly.

Now day in and day out, you’ll have seen me changing the code to keep it fresh and to make it express, as well as I can manage, my design ideas. That’s what refactoring is: “improving the design of existing code”, the subtitle of Martin Fowler’s seminal book, Refactoring.

I am proud to call all three of those gentlemen my friend, and it doesn’t even bother me to recognize that their wisdom and skill exceed mine by a wide margin. They willingly share their ideas, and I’m fortunate to have been able to associate with them starting around the beginning of the “Agile” movement.

But I digress. My point today is that I’m beginning to sense that our design in the dungeon program isn’t quite aligned with our growing understanding of what we’re doing and how to do it.

It all started …

I was thinking about how the Cell object bears so much weight in the current system, despite really representing little more than a pair of coordinates. And it troubles me to have the cell_array as a class variable. It seems very convenient, but it really represents relationships between the cells, and although we only reference it in a couple of places, it just gives me a bit of an icky feeling.

Currently the cell knows its room. We actually use that value, which can be None, to determine whether or not the cell is used in the dungeon or not. So the Cell, really knows two things that my design sense says that it shouldn’t. It knows the primary collection it’s from, and it knows an instance of Room, which is a class that contains Cells, not a thing that Cell really ought to know about.

So I was thinking that maybe Cell should just be a tuple (x,y), perhaps with some added behavior, making it a subclass of tuple (probably not a good idea) or an object containing only a tuple, which is almost the case now, but not quite.

I thought about sets and dictionaries a bit and then somehow it came to me that instead of a dumb object like that, maybe the Cells should be in a DungeonLayout, not in an array or set or dictionary. The Layout, of course, could use those objects at will. As soon as one thinks this thought, it becomes clear that there is merit in it.

A DungeonLayout is a particular set of Cells, arranged into Rooms. If cells are just tokens, tuples or the like, then there could be multiple Layout instances. We might create them randomly, as we do now, or we could hand-craft interesting Layouts as special areas in an otherwise random dungeon game. We could save them as part of the game, even write them out to files and email them to our friends.

We want this …

This seems to me to be a good idea, one that deserves to be in the program. Yesterday at this time, I didn’t have this idea: today I have it. The question is what to do with it. One possibility is to do nothing with it, but the feature implied by allowing multiple separate layouts is clearly a good one and so we probably really want it.

How can we get there?

Here’s a vague hand-waving notion of what we might do: don’t all good ideas start with hand-waving?

We just changed Cell to have a 2d array in class variable cell_array, together with width and height class variables. Those three things are begging to be made into an object anyway, and are perhaps the biggest sign that removing CellSpace was not without design cost.

So there is a case to be made that putting a Layout in there would be a reasonable thing, if the Layout could keep track of cell state properly.

Let’s look at how we use the cell_array now, to get a sense of how hard this might be and what is entailed:

class Cell:
    deltas = [(-1, 0), (1, 0), (0, -1), (0, 1)]
    cell_array = list()
    width = 0
    height = 0

    @classmethod
    def create_space(cls, max_x, max_y):
        cls.cell_array = list()
        cls.width = max_x
        cls.height = max_y
        for x in range(cls.width):
            col = []
            cls.cell_array.append(col)
            for y in range(cls.height):
                cell = Cell(x, y)
                col.append(cell)

    def __init__(self, x, y):
        self._x = x
        self._y = y
        self._room = None

    @classmethod
    def at(cls, x, y):
        if 0 <= x < cls.width and 0 <= y < cls.height:
            return cls.cell_array[x][y]
        else:
            return None

    @classmethod
    def unused_cell_or_none(cls):
        available = [cell for col in cls.cell_array for cell in col if cell.is_available]
        if available:
            return random.choice(available)
        else:
            return None

Those are the only methods that refer directly to cell_array. However, things aren’t that simple. The class method at is used … hold on a moment … 111 times, mostly in tests.

I’m thinking of an evolutionary approach here. We’ll mostly want people who are dealing with Cell today to be dealing directly or indirectly with a Layout, because it will the the Layout that knows whether a cell is in use. We might temporarily put a layout into the Cell class variable.

Or maybe there should be a distinguished global, current_layout that Cells can use, still temporarily.

Or maybe there is still a Cell object, but they are created by the Layout to wrap the tuples that are down at the base of things.

A path forward … ?

What if we did something like this:

  1. Replace the x and y members in Cell with a tuple (x,y).
  2. Replace the cell_array with two sets or dictionaries, used and unused;
  3. … profit!!!

No. I do like the first step, however. Almost tempted to just do it. In fact, just as an experiment, let’s try it.

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

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

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

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

We are green. Commit. Cell stores x,y as tuple.

Why did you commit that?
Because it seems like a step in the direction we want to go.
But you don’t even know how to get where you want to go!
Right, but now we’re closer and if we don’t like it we can change it back readily.

Now we still have only those four methods above that know about cell_array, width, and height. We should be able to tell Layout how to manage the cells and then slip it in there pretty readily.

I’m starting to wish that I had CellSpace back. Of course, if we want it, we can get it back. It only had three methods anyway, __init__, at, and random_available cell.

A sub-idea …

I have a notion of how to go in small steps. If we made new properties, cell_array, width, and height, returning those things, we could then implement them in DungeonLayout and plug it in, then unwind those four methods to refer to the layout directly and then remove those properties.

If there were a lot of methods needing changing, I might try that. As things stand, I think it’ll be easy enough to fix up the four as soon as Layout is ready to be slotted in. So belay that idea at least for now.

Ready to start …

I think we can TDD into DungeonLayout, these methods: at(x,y) and unused_cell_or_none, and then wire the Layout into Cell somehow.

As for “somehow”, I think we might move from a class variable to a distinguished global somewhere.

To tell the truth …

The whole notion of a global or class variable smacks of the Singleton pattern, and some received wisdom in my mind says that whenever one is tempted to use Singleton, one should think twice and then not do it, because you invariably need more than one. And certainly we will be needing more than one Layout … that’s the whole point of the idea. So we’ll do something else, some combination of:

  1. Use methods on Layout to do things now done by Cell;
  2. Pass the current Layout to Cell if we really want Cell to do something with it.

Next steps …

We’ll add those two methods to DungeonLayout, implementing them as simply as we can, expecting to refactor and perhaps to provide some utility objects as we learn more.

    def test_at(self):
        layout = DungeonLayout(5, 5)
        cell =  layout.cell_at(3,3)
        assert cell.xy == (3,3)

I copy-pasta to get this:

class DungeonLayout:
    def __init__(self, max_x=10, max_y=10):
        self.cell_array = list()
        self.width = max_x
        self.height = max_y
        for x in range(self.width):
            col = []
            self.cell_array.append(col)
            for y in range(self.height):
                cell = Cell(x, y)
                col.append(cell)
        self.rooms = []

    def cell_at(self, x, y):
        return self.cell_array[y][x]

Green. Commit: adding cell ownership to DungeonLayout. But let’s make a set instead, just to see how it looks:

class DungeonLayout:
    def __init__(self, max_x=10, max_y=10):
        self.cells = dict()
        self.width = max_x
        self.height = max_y
        for x in range(self.width):
            for y in range(self.height):
                self.cells[(x,y)] = (Cell(x, y))
        self.rooms = []

    def cell_at(self, x, y):
        return self.cells[(x,y)]

Green let’s use it. Commit. So far so good but what about out of range? We test.

    def test_at_range(self):
        layout = DungeonLayout(5, 5)
        assert layout.cell_at(5,5) is None
        assert layout.cell_at(-1, 3) is None
        assert layout.cell_at(2, -1) is None

Now we could check width and height, but why not just let the dictionary decide?

class DungeonLayout:
    def __init__(self, max_x=10, max_y=10):
        self.cells = dict()
        for x in range(max_x):
            for y in range(max_y):
                self.cells[(x,y)] = Cell(x, y)
        self.rooms = []

    def cell_at(self, x, y):
        return self.cells.get((x,y), None)

I hate the name cell_at. We’ll call it just at. Not because it’s shorter but because naming the class we expect is just odd.

class DungeonLayout:
    def __init__(self, max_x=10, max_y=10):
        self.cells = dict()
        for x in range(max_x):
            for y in range(max_y):
                self.cells[(x,y)] = Cell(x, y)
        self.rooms = []

    def at(self, x, y):
        return self.cells.get((x,y), None)

Commit.

Shall we try plugging this in? We could put it into create_space, if we fudge the import:

class Cell:
    deltas = [(-1, 0), (1, 0), (0, -1), (0, 1)]
    layout = None

    @classmethod
    def create_space(cls, max_x, max_y):
        from dungeonlayout import DungeonLayout
        cls.layout =  DungeonLayout(max_x, max_y)

That, plus this:

class Cell:
    @classmethod
    def at(cls, x, y):
        return cls.layout.at(x, y)

    @classmethod
    def unused_cell_or_none(cls):
        return cls.layout.unused_cell_or_none()

class DungeonLayout:
    def unused_cell_or_none(self):
        available = [cell for cell in self.cells.values() if cell.is_available]
        if available:
            return random.choice(available)
        else:
            return None

This has us green. We’ll commit, reflect, and sum up, as I’ve done enough for the morning. Commit: Cell uses DungeonLayout for cell allocation and recording.

Reflection

On the one hand, we have completed wiring the Layout in where the array was when we got here, and where the CellSpace was the day before. In retrospect, we could have skipped the conversion to an array, but at the time we didn’t know better, just had the sense that CellSpace wasn’t quite the thing. So two quick steps and we’re recording things in the Layout, which seems more sensible, since a Layout is basically everything about a particular Dungeon level’s layout.

And we just kind of followed our nose. We have some vague big idea in mind, but we saw small steps that seem to lead in the right direction. As we take each one, the next one appears almost by magic. What’s really happening is that as we get closer to our vague idea, possible approaches begin to become apparent. If we don’t fear to try those possibilities, we kind of wander our way from good to better.

On the other hand, there are things not to love:

  1. We had to import DungeonLayout into Cell, in order to define one. We’d do better to inject it, if we’re going to have it at all, or perhaps to import it as a variable somehow.

  2. We clearly have work to do beyond our single dictionary, especially if we want to get rid of the room member in Cell, leaving us with essentially just a tuple. I’m not locked in on that removal, because I think a DungeonLayout perhaps has all kinds of info associated with a given cell, such as what is in it, who is standing on it and so on. But it does seem that it could just be coordinates.

  3. So many methods are now part of Cell. There are neighbor-related methods, path-related ones, and so on. I have a vague but strong feeling that there might be a tuple-like thing that can compute distances, and neighbor offsets (vector addition) and not much else. Paths would come from some meta-structure perhaps known to the layout.

  4. I’m troubled by the fact that there are now 112 uses of Cell.at. They are mostly in tests but probably should be addressed to the Layout. It might be worth unwinding that but it’s hard to see that as a small step, and if we are to get where we think we’re heading, we’ll want to do it in small steps, since the alternative is to say that some design changes are just not practical. That may be true, but I think it is rare and I pride myself on having generally found a scheme that could do any refactoring in small steps. Perhaps we just forward through Cell as a deprecated scheme, with some better scheme able to be factored in over time.

I think the biggest rap against the past few days is to say that if only we had thought more, we probably could have done this change directly, replacing CellSpace, instead of first replacing CellSpace with an array and then replacing the array with a Layout. However, “be smarter” smacks of Chet Hendrickson’s prime example of useless advice: Be taller.

I think we do well to respond to the forces we feel in the code when we sense them strongly enough. The CellSpace just felt wrong: remove it. Better idea arises, perhaps from that effort, perhaps out of the blue: assess it and if it seems tasty, use it. So long as we are making the code feel better, I think we do fine.

There is, of course, the issue of “shipping it”. Someone on Mastodon yesterday was drawing the distinction between programmers who are all about the making the code right, and those who are all about shipping something. Those are interesting poles, and the way to be is surely somewhere in between them. If we are too focused on shipping, the code won’t work and won’t be maintainable. If we are too focused on code quality, we might never ship.

Except … except … this program is drawing the dungeons we want, creating the paths we need, and so on. It has been doing that from the beginning, and has done so every day for ages. Whenever some new feature idea has come to mind, we have had it working within a couple of sessions.

Good code and shipping are not opposed at all: with a proper balance, they go hand in hand. Keeping the code well designed and flexible lets us add capabilities readily, and it keeps the defects down as well. When we balance doing good work with a focus on features, we produce better features faster.

That’s my story, and I’m sticking to it. See you next time!