Refactoring I

This code isn’t very good. I’ll tell you why, and we’ll try to do something about it.

Well, the FGNO¹ session didn’t yield much. I don’t know if the gang was gobsmacked by the glory of my code, or simply not in the mood, but I think the best notion I got was the observation that “room” and “continent” don’t really fit in the same metaphor and perhaps “suite” is more what we’re looking for. Admittedly one does not think of a cavern has having suites of rooms, but certainly one thinks even less of continents of rooms.

Be that as it may, my focus this morning will be on improving the code in the continent suite generation and elsewhere.

I invite you to pause at each displayed chunk of code, think about what needs improvement, how you might improve it, and then see what I say and do. I suggest this because, for me, with one mind-set in place, the code we’ll look at today looks kind of “OK”, a lot like code we find all over. It is not, however, the kind of code my betters taught me about, nor the kind of code I have come to prefer, which is as close to what my betters have taught me as I can generally get.

Let’s get to it. I’ll just browse around until I find something to fix.

class Cell:
    @property
    def neighbors(self):
        deltas = [(-1, 0), (1, 0), (0, -1), (0, 1)]
        neighbors = []
        for dx, dy in deltas:
            neighbor = self.at_offset(dx, dy)
            if neighbor is not None:
                neighbors.append(neighbor)
        return neighbors

Take a look. What do you see? What would you improve?

My focus today will be on making the code more “object-oriented”, and in particular to have methods that tend to meet the Composed Method criteria, which would have all the statements in a method at the same level of abstraction.

One key signal, to me, is to see nesting in the code. So the double indentation there is what caught my eye. However, I can see at least two ways I might improve this code, one that might make it more Pythonic, and one that might make it more object oriented.

Let’s try the Pythonic one. I think we can write that loop as a comprehension, and if we can,, that might be good.

I don’t think PyCharm has any fancy ways to do this for me, although its “AI” feature might be able to do it, if I were to use it, which I will not. I am here to program, not to have someone else program for me.

    @property
    def neighbors(self):
        deltas = [(-1, 0), (1, 0), (0, -1), (0, 1)]
        return [self.at_offset(dx, dy) 
            for dx, dy in deltas 
            if self.at_offset(dx, dy)]

That’s passing the tests. I don’t like having to call at_offset twice. Is there a way to pass forward a temp result, such as from that if clause? Yes, with walrus := and extra parens. I tried without and it was no go.

    @property
    def neighbors(self):
        deltas = [(-1, 0), (1, 0), (0, -1), (0, 1)]
        return [neighbor for dx, dy in deltas if (neighbor := self.at_offset(dx, dy))]

We can do better.

    @property
    def neighbors(self):
        deltas = [(-1, 0), (1, 0), (0, -1), (0, 1)]
        return [neighbor
                for dxy in deltas
                if (neighbor := self.at_offset(*dxy))]

That’s a bit less busy, I think.

Probably that deltas list should be a class variable. No point creating it over and over.

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

    @property
    def neighbors(self):
        return [neighbor
                for dxy in self.deltas
                if (neighbor := self.at_offset(*dxy))]

Would it be better to retain the is not None in the if clause? Judgment call. I think we won’t do that.

So, that’s the Pythonic way, to the best of my ability. What about an OO way? Let’s try, starting back where we were:

    @property
    def neighbors(self):
        deltas = [(-1, 0), (1, 0), (0, -1), (0, 1)]
        neighbors = []
        for dx, dy in deltas:
            neighbor = self.at_offset(dx, dy)
            if neighbor is not None:
                neighbors.append(neighbor)
        return neighbors

Switch to the class variable first this time, since I really have it already:

    @property
    def neighbors(self):
        neighbors = []
        for dx, dy in self.deltas:
            neighbor = self.at_offset(dx, dy)
            if neighbor is not None:
                neighbors.append(neighbor)
        return neighbors

Change the loop to use the dxy trick.

    @property
    def neighbors(self):
        neighbors = []
        for dxy in self.deltas:
            neighbor = self.at_offset(*dxy)
            if neighbor is not None:
                neighbors.append(neighbor)
        return neighbors

Extract the inside of the loop:

    @property
    def neighbors(self):
        neighbors = []
        for dxy in self.deltas:
            self.add_real_neighbor(dxy, neighbors)
        return neighbors

    def add_real_neighbor(self, dxy: tuple[int, int], neighbors: list[Any]):
        neighbor = self.at_offset(*dxy)
        if neighbor is not None:
            neighbors.append(neighbor)

Maybe extract again?

    @property
    def neighbors(self):
        neighbors = []
        self.add_all_real_neighbors(neighbors)
        return neighbors

    def add_all_real_neighbors(self, neighbors: list[Any]):
        for dxy in self.deltas:
            self.add_real_neighbor(dxy, neighbors)

Inline?

    @property
    def neighbors(self):
        return self.get_all_real_neighbors([])

    def get_all_real_neighbors(self, neighbors: list[Any]):
        for dxy in self.deltas:
            self.add_real_neighbor(dxy, neighbors)
        return neighbors

    def add_real_neighbor(self, dxy: tuple[int, int], neighbors: list[Any]):
        neighbor = self.at_offset(*dxy)
        if neighbor is not None:
            neighbors.append(neighbor)

This isn’t making things better, is it? So far, the Pythonic list comprehension seems far better for our purpose.

An issue here is the need to filter out the None, the result we get when we try to fetch a cell that doesn’t exist, that is outside the dimensions of the CellSpace:

class CellSpace:
    def at(self, x, y):
        if 0 <= x < self.width and 0 <= y < self.height:
            return self.cell_array[x][y]
        else:
            return None

There is a pattern Missing Object or Null Object, that suggests returning a dummy object instead of a None when there is no legitimate thing to return. I’ve made good use of that, occasionally, but the new object requires a lot of care to ensure that it is harmless elsewhere in the system. Would it be available or not? Depends on what use people make of that flag. Would it be in a room or not? If a cell is unavailable we currently assume that it is in a room. So we might try to add that room to a continent (suite?) or something.

That way lies complexity that we don’t need.

The jury is in. In this case, the Pythonic version with the list comprehension wins.

    @property
    def neighbors(self):
        return [neighbor
                for dxy in self.deltas
                if (neighbor := self.at_offset(*dxy))]

Green. Commit: refactoring.

Reflection

Well, as usual, things do not go as I anticipated. I had expected to find code that I didn’t love—that part worked just fine—and I expected to refactor it to a better object-oriented style. In this case, the OO style clearly isn’t as nice as the list comprehension, at least according to me.

Do you agree? Do you see things that I missed? Let me know.

I’m going to do another method but this article is long enough to put anyone to sleep, the primary use, I am told, of my articles.

Summary

I was surprised at first to find that the list comprehension was better, in my eyes, than an object-oriented refactoring. Only before I started: if you had asked me to place a bet as soon as I say the possibility, I’d have bet on the Pythonic style for that case.

We’re going to see another situation in the next article and it’s going to be a bit more object oriented. And in the one after that … I think we’ll see something actually interesting.

See you anon …