More Code Signs

Python Asteroids+Invaders on GitHub

A very strange session. Two hours of reading and thinking, one very simple change adding an enum, no major improvement. Wasted? Or valuable? I’m not sure yet.

When I started this exercise yesterday, I was thinking that I’d wind up with a series of code signs, signals, observations, and portents, with some text describing each one in general terms, and discussing how to deal with it in general terms. But if yesterday is any evidence, that’s not how I work. Instead, it seems, I spot something and then work on that bit of code until I like it well enough, performing more than one kind of transformation on it.

I would say that my chief weapon is Rename. Rename and Extract Method, Extract Method and Rename. My two chief weapons are Rename and Extract Method, and Extract Variable. My three chief weapons are Rename, Extract Method, Extract Variable, and an almost fanatical dedication to tiny methods.

But seriously, note that all three of these are about giving names to things, whether those things are methods, variables, or smallish computations.

I am reminded of Kent Beck’s famous four rules for simple design. In priority order:

1. Passes all the tests;
2. Contains no duplication;
3. Expresses all the programmer’s intentions;¹
4. Minimizes elements, such as files, classes, methods, variables.

Our chief weapon for expressing intention really is naming, in all its forms, so it is no wonder that my favorite tools are all about naming.

Down Tuit

Let’s look for some code that would like to be improved. I’m sure there’s some nearby.

Sure enough, here’s something right here:

class InvaderFleet(InvadersFlyer):
    def __init__(self):
        self.step = Vector2(8, 0)
        self.down_step = Vector2(0, 32)
        self.invader_group = InvaderGroup()
        self.origin = Vector2(u.SCREEN_SIZE / 2 - 5*64, 512)
        self.invader_group.position_all_invaders(self.origin)
        self.reverse = False
        self.direction = 1
        self.step_origin()

    def update(self, delta_time, _fleets):
        result = self.invader_group.update_next(self.origin)
        self.process_result(result)

    def process_result(self, result):
        if result == "ok":
            pass
        elif result == "end":
            if self.reverse:
                self.reverse_travel()
            else:
                self.step_origin()
        else:
            assert False

    def step_origin(self):
        self.origin = self.origin + self.direction * self.step

    def reverse_travel(self):
        self.reverse = False
        self.direction = -self.direction
        self.origin = self.origin + self.direction * self.step + self.down_step

It’s the process_result method that caught my attention. It’s very jagged. By that, I mean that its indentation seems to go in and out like a saw blade. I include the rest for context that we’ll probably want while we figure out what to do.

I’m also curious about the result return from self.invader_group.update_next. Let’s have a glance at that.

class InvaderGroup:
    def update_next(self, origin):
        if self._next_invader >= len(self.invaders):
            self._next_invader = 0
            return "end"
        invader = self.next_invader()
        invader.position = origin
        self._next_invader += 1
        return "ok"

Ah, yes. Remember that the invaders only move one at a time Every 60th of a second, we move just one invader over a little bit. This gives them that cool creepy way of moving and probably really reflects the fact that on a 6502 there wasn’t time to move all 45 of them. So update_next returns “ok” when it moves an invader and returns “end” when it has reached the end of the list.

Back in InvaderFleet, we are doing this:

    def process_result(self, result):
        if result == "ok":
            pass
        elif result == "end":
            if self.reverse:
                self.reverse_travel()
            else:
                self.step_origin()
        else:
            assert False

We ignore the “ok” message and carry on. Next time through update, we’ll move another invader. But if the result is “end”, we have work to do. It appears that either we reverse travel, or step the origin. But hey! Look at the two methods we call:

    def step_origin(self):
        self.origin = self.origin + self.direction * self.step

    def reverse_travel(self):
        self.reverse = False
        self.direction = -self.direction
        self.origin = self.origin + self.direction * self.step + self.down_step

Those almost look backward at first and second glance. This code’s names are not helping me at the moment. Let’s see …

Note

I seem to be in a mode of figuring out this code. There is another way that I might proceed, which would be to begin simplifying without really understanding, instead making safe refactoring changes. We might turn to that mode, but for now, I want to do a bit more figuring.

We know that we init the reverse flag to False, and direction to 1. So our normal behavior on “end” is to call step_origin, which just moves the origin by +1 times the step. So usually, at “end”, we keep moving in the same direction. How does reverse ever get set True?

    def at_edge(self, bumper_incoming_direction):
        self.reverse = bumper_incoming_direction == self.direction

That is called only when an individual invader interacts with a bumper:

class Invader:
    def interact_with_bumper(self, bumper, invader_fleet):
        if bumper.intersecting(self.rect):
            invader_fleet.at_edge(bumper.incoming_direction)

We tell the fleet “I hit a bumper, and I was going this way”. Oh, this is nasty. I almost remember this.

Because of the way the invaders move, if one invader in the end column hits the bumper, all the invaders in that column will hit it, when it is their turn to move. Furthermore, they’ll all wind up colliding with the bumper, because we don’t back them off or kill them. So we will get many calls to at_edge while moving into the bumper, and then, after we reverse, many more.

What do I mean by many? Well, imagine that we’re just next to the right hand bumper. We start moving the invaders for the next cycle. After a while, the rightmost invader hits the bumper. It says at_edge. We then move the first invader in the next row. But the first invader is still in the bumper so it says at_edge. After a while, the rightmost invader in the second row hits the bumper and now two of them are saying at_edge. After a bit longer, three are saying it, After a while, four. Finally, we move the fifth rightmost invader, who says at_edge, resulting in five calls and we finally return “end” rather than OK. So we’ll have seen upwards of 100 calls to at_edge. Then we finally reverse direction but we continue to get calls to at_edge until we finally move the last invader out of the bumper. But because we’re moving away from the bumper, the reverse flag is set False rather than True.

Is that hard to understand? Yes, it is.

Am I surprised that it works? Yes, I am. Am I a little bit proud and a little bit ashamed? Yes, I am.

Dilemma²

This “algorithm” is truly weird. In one reversal cycle, we get 280 calls to at_edge. We have two² choices:

1. Improve this code without changing the algorithm;
2. Improve the algorithm and the code;
3. Back away slowly².

I think that a wise person might well back away slowly. A less wise person might improve this code without changing how it works. And a truly unwise person might decide to redo the algorithm entirely.

Let’s see about improving the code, to see if improved clarity makes it more clear how to improve the algorithm itself.

Note

After a long time, we make one small improvement. Carry on.

We’ll start here:

    def process_result(self, result):
        if result == "ok":
            pass
        elif result == "end":
            if self.reverse:
                self.reverse_travel()
            else:
                self.step_origin()
        else:
            assert False

The assert False had better never happen. What we really need is some kind of return that isn’t a free string but an enum or something with only two possible values. We could consider returning a boolean, True if at end and False otherwise.

That changes representation but not the logic. Let’s consider it. But when we look at the function we call, we don’t like the boolean idea:

class InvaderGroup:
    def update_next(self, origin):
        if self._next_invader >= len(self.invaders):
            self._next_invader = 0
            return "end"
        invader = self.next_invader()
        invader.position = origin
        self._next_invader += 1
        return "ok"

Making this return True or False, as it stands, would not make it clear what the values stand for. Let’s think about what we want to know.

We want to know, upon calling update_next, whether we’re starting a new cycle. Careful consideration will tell us that we actually move the last element and then on the next call, return “end”. That is, we “waste” a call, which moves nothing. Certainly mostly harmless.

Now we happen to know that the InvaderFleet doesn’t care about “ok”. But we have to return something. We could change these values. An enum would be the standard way to do this, rather than these strings. This whole thing looks kind of p-baked, doesn’t it?

Interrupt

We are 200 lines into this article, nearly two hours of thinking and writing, and haven’t changed a line of code. We should stop, reverting to Dilemma Item #3, “Back away slowly”. Or maybe we can make it just a tiny bit better. Let’s change the strings to an enum, in two steps.

    def process_result(self, result):
        if result == "continue":
            pass
        elif result == "new cycle":
            if self.reverse:
                self.reverse_travel()
            else:
                self.step_origin()

Tests break, until:

    def update_next(self, origin):
        if self._next_invader >= len(self.invaders):
            self._next_invader = 0
            return "new cycle"
        invader = self.next_invader()
        invader.position = origin
        self._next_invader += 1
        return "continue"

Commit: modify return strings in prep for enum.

Now let’s create the enum. I don’t think I’ve ever done that.

class CycleStatus(Enum):
    CONTINUE = "continue"
    NEW_CYCLE = "new cycle"

class InvaderFleet:
    def process_result(self, result):
        if result == CycleStatus.CONTINUE:
            pass
        elif result == CycleStatus.NEW_CYCLE:
            if self.reverse:
                self.reverse_travel()
            else:
                self.step_origin()

class InvaderGroup:
    def update_next(self, origin):
        if self._next_invader >= len(self.invaders):
            self._next_invader = 0
            return CycleStatus.NEW_CYCLE
        invader = self.next_invader()
        invader.position = origin
        self._next_invader += 1
        return CycleStatus.CONTINUE

Green, after modifying four tests. Commit: convert to use enum CycleStatus.

Summary

Well, there’s two hours wasted. Or were they? We’ve discovered some very odd code in the system. It’s not obvious how it works, and when you realize that the at_edge method is being called hundreds of times inbound to a bumper and hundreds more outbound, there’s a case to be made that the code should be improved.

If I had been doing this alone, which I was, or with a pair, I think we might well stop here for the day, and we might have a Quick Design Session tomorrow with interested team members, to decide what, if anything, to do about this code. We’d do that with the code on the screen of course.

Tomorrow, the team might decide to leave it alone. And that’s a valid result, but we do have some learning built up, and there’s certainly room for improvement here.

What will we do tomorrow? I don’t know, but I’m inclined to see whether we can make this make more sense.

One thing’s for sure: I still don’t have any large-scale what to look for, what to do, general rules. Other than “back away slowly”, which has some value.

See you next time!