Cleanup

Python Asteroids+Invaders on GitHub

Let’s have a look at the new code for invaders and edges, see what it’s telling us, and do something about those hundred-plus calls. Bear bites man. Man survives.

We have, in essence, rebalanced the code between InvaderFleet, InvaderGroup, and Invader. The original aim was simple and small, and then it sort of got away from us, but I think the results are decent, at least in the InvaderFleet.

As regards invader motion, the Fleet’s main responsibility is to maintain the “origin”, the position of the first invader in the rack, stepping that origin sideways after each movement cycle, stepping downward when the invaders hit the edge, and reversing direction. The Fleet does not know how many invaders there are, so in each game cycle, it asks the Group to update.

The Group holds all the live Invaders, and on each update, updates just one of them, which gives the Invaders’ motion an interesting rippling effect. The Group can return one of three values at this time, CONTINUE, NEW_CYCLE, and REVERSE. It returns CONTINUE except at the end of a cycle. After it has moved the last Invader, it generally returns NEW_CYCLE, which informs the Fleet to update the origin by stepping sideways. However, if one or more invaders have hit the edge, Group returns REVERSE, which informs the Fleet to reverse direction and move the origin downward one step.

The Invaders do not really know that they are moving. The Group sets the origin of each Invader, in its turn, to the current Fleet origin, which brings that individual Invader up to date with the current origin, moving it a tiny step. The Invaders are responsive to two interaction messages, detecting hits from player shots, and detecting that they have hit a bumper. When they hit a bumper, they send a message back to the Group, which remembers that fact and will, when the cycle is over, return REVERSE.

This seems, when you tell the story, to be a reasonable division of responsibilities. One issue is that once one invader hits the bumper, it will continue to send its at_edge message on every cycle, and if more than one invader hits the edge before the cycle is over, it, too, will start sending at_edge, and the upshot of that is that the Group may be informed over one hundred times. This is harmless, in the the at_edge code just sets a flag:

    def at_edge(self, bumper_direction):
        self.should_reverse = self.current_direction == bumper_direction

Now one might worry that the flag might be prematurely turned off. The programmer knows that that won’t happen, because it will only be turned off if the Invaders are moving away from the edge, that is, after the reversal has started. I think we can do better.

Improving Things

Let’s only set the should_reverse flag in at_edge, never clear it.

    def at_edge(self, bumper_direction):
        if self.current_direction == bumper_direction:
            self.should_reverse = True

My idea here is that we are less likely to be confused by this code, because now it it clear that we aren’t in danger or changing the flag back.

But where do we set it back? As things stand now: nowhere. The Invaders will move to one edge and then stay there, marching downward but never visibly reversing.

We can reset the flag at the end of a cycle, here:

    def end_cycle(self):
        self._next_invader = 0
        return CycleStatus.REVERSE if self.should_reverse else CycleStatus.NEW_CYCLE

We’ll have to cache our result.

    def end_cycle(self):
        self._next_invader = 0
        result = CycleStatus.REVERSE if self.should_reverse else CycleStatus.NEW_CYCLE
        self.should_reverse = False
        return result

I am sure this works, though I haven’t tried it in the game yet, but I note that no tests fail when we don’t clear should_reverse. We need a more robust test. What do we have for REVERSE?

Note

I may have been sure, but I was also dead wrong. It does not work. And after I improve the tests, I do not test the game. The tests run but the game does not.

    def test_reversal_on_entry(self):
        group = InvaderGroup()
        bumper = Bumper(u.BUMPER_RIGHT, +1)
        invader = group.invaders[0]
        _pos_x, pos_y = invader.position
        invader.position = (u.BUMPER_RIGHT, pos_y)
        group.interact_with_bumper(bumper, None)
        result = group.end_cycle()
        assert result == CycleStatus.REVERSE

If we were to run another full cycle here, we should be able to get down to a NEW_CYCLE, not another REVERSE.

Let’s try a somewhat aggressive test:

    def test_reversal_on_entry(self):
        group = InvaderGroup()
        bumper = Bumper(u.BUMPER_RIGHT, +1)
        invader = group.invaders[0]
        _pos_x, pos_y = invader.position
        invader.position = (u.BUMPER_RIGHT, pos_y)
        group.interact_with_bumper(bumper, None)
        result = group.end_cycle()
        assert result == CycleStatus.REVERSE

        origin = (0, 0)
        result = CycleStatus.CONTINUE
        while result == CycleStatus.CONTINUE:
            invader.position = (u.BUMPER_RIGHT, pos_y)
            result = group.update_next(origin, -1)
        assert result == CycleStatus.NEW_CYCLE

The second bit runs a full update cycle and expects the result to be NEW_CYCLE. With the line that clears the flag present, this test passes. With it absent, it fails, again returning REVERSE.

We can commit: at_edge no longer clears should_reverse, only sets it.

Note:

This commit commits a bug: the invaders do not move back to the left as they should. The test runs because it is not processing the final interactions before the new cycle. In the game, the final interaction cycle sets the reverse flag again.

Now, however, once that flag is set, we no longer need to ask the question: we know we must reverse. So let’s stop asking. We have this:

    def interact_with_bumper(self, bumper, _fleet):
        for invader in self.invaders:
            invader.interact_with_bumper(bumper, self)

If we should reverse, we need not run this loop. And, if we are in the loop, and we suddenly should reverse, we can stop asking. Let’s cover both cases:

    def interact_with_bumper(self, bumper, _fleet):
        if self.should_reverse:
            return
        for invader in self.invaders:
            if self.should_reverse:
                return
            invader.interact_with_bumper(bumper, self)

That’s rather odd, isn’t it? But it’s correct. (This code, yes. All the code, no, as I’m about to discover.)

I Am Mistaken

I do truly hate when this happens. I have out-smarted myself. The tests all run and the Invaders still get over to the right and then just sidle right down, never moving back to the left.

The bug is in latching that flag:

    def at_edge(self, bumper_direction):
        if self.current_direction == bumper_direction:
            self.should_reverse = True

Presently I am clearing the flag in the end_cycle:

    def end_cycle(self):
        self._next_invader = 0
        result = CycleStatus.REVERSE if self.should_reverse else CycleStatus.NEW_CYCLE
        self.should_reverse = False
        return result

That’s too soon, because after we have updated, it’s only then that we’ll get the interactions messages, and since we have cached the direction of motion, it is still what it was, and the invaders who are in the bumper will again cry out, and we’ll set the reverse flag again.

I could roll back but I have a bad commit out there and I prefer to move forward, even if it’s backward, if you know what I mean. We’ll go back to this:

    def end_cycle(self):
        self._next_invader = 0
        return CycleStatus.REVERSE if self.should_reverse else CycleStatus.NEW_CYCLE

    def at_edge(self, bumper_direction):
        self.should_reverse = self.current_direction == bumper_direction

That’s breaking a test, which surprises me a bit. It’s the extended part that I just did, in this test:

    def test_reversal_on_entry(self):
        group = InvaderGroup()
        bumper = Bumper(u.BUMPER_RIGHT, +1)
        invader = group.invaders[0]
        _pos_x, pos_y = invader.position
        invader.position = (u.BUMPER_RIGHT, pos_y)
        group.interact_with_bumper(bumper, None)
        result = group.end_cycle()
        assert result == CycleStatus.REVERSE

        origin = (0, 0)
        result = CycleStatus.CONTINUE
        while result == CycleStatus.CONTINUE:
            invader.position = (u.BUMPER_RIGHT, pos_y)
            result = group.update_next(origin, -1)
        assert result == CycleStatus.NEW_CYCLE

It’s returning REVERSE there at the end.

After more thinking than I’d prefer to need, I see that the test is not robust enough, because it’s not doing the interactions.

I can make the test run with this code:

    def update_next(self, origin, current_direction):
        if self.current_direction != current_direction:
            self.should_reverse = False
        self.current_direction = current_direction
        if self._next_invader >= len(self.invaders):
            return self.end_cycle()
        invader = self.next_invader()
        invader.position = origin
        self._next_invader += 1
        return CycleStatus.CONTINUE

    def end_cycle(self):
        self._next_invader = 0
        return CycleStatus.REVERSE if self.should_reverse else CycleStatus.NEW_CYCLE

    def at_edge(self, bumper_direction):
        self.should_reverse = self.current_direction == bumper_direction

    def interact_with_bumper(self, bumper, _fleet):
        if self.should_reverse:
            return
        for invader in self.invaders:
            if self.should_reverse:
                return
            invader.interact_with_bumper(bumper, self)

It’s clearing the should_reverse in update_next that makes the test run. The game is OK as well. Commit: fix bad game commit.

Now can I go back to not clearing the flag in at_edge? I think yes.

    def at_edge(self, bumper_direction):
        if self.current_direction == bumper_direction:
            self.should_reverse = True

Reflection

OK, I’ve made a serious mistake, exacerbated by believing my tests were more robust than they actually are, and therefore not trying the game before a commit. It seems like cheap insurance to run it once before committing. But I very much want to be able to rely on my tests to find the problems. That is clearly unwise with this code, these tests, and this programmer, and very likely always unwise, though maybe your tests are that solid. If so, my hat’s off to you.

My gut feeling is that while we have improved InvaderFleet, and made its relationship with InvaderGroup better, using the CycleStatus enum, the relationship between InvaderGroup, Invader, and Bumper is a bit more complicated than we’d like.

I do think that it’s solid now, where we only ever set should_reverse to True during interactions, and clear it explicitly at the beginning of the next cycle, in update_next_invader. Overall, however, it’s too tricky behind the curtains, because after we have returned REVERSE, the Invaders are actually calling back reporting that we’re at the edge. It’s just that we ignore their effect by clearing the flag in the update.

I think what I’d do is make a card for this and post it wherever we post cards about code that could benefit from improvement. At this moment, I don’t see what would make it better.

What if we had some kind of “token” that the Group created at the beginning of a cycle, and that got saved and passed to the invaders during the interaction part of the cycle, in which they could record the fact of their collision with the bumper? It would behave like the flag, but it would be a real, if small object that could encapsulate a bit of behavior. Might be a good idea.

Or maybe we should enlist the aid of the bumper somehow. It doesn’t process any interactions now, but it could do.

Needs thought.

Let’s take a scan at what we’ve changed in InvaderGroup and see if there’s any small tidying to be done.

    def update_next(self, origin, current_direction):
        if self.current_direction != current_direction:
            self.should_reverse = False
        self.current_direction = current_direction
        if self._next_invader >= len(self.invaders):
            return self.end_cycle()
        invader = self.next_invader()
        invader.position = origin
        self._next_invader += 1
        return CycleStatus.CONTINUE

    def end_cycle(self):
        self._next_invader = 0
        return CycleStatus.REVERSE if self.should_reverse else CycleStatus.NEW_CYCLE

    def at_edge(self, bumper_direction):
        if self.current_direction == bumper_direction:
            self.should_reverse = True

    def next_invader(self):
        return self.invaders[self._next_invader]

I think that update_next method is too jagged. In reference to my remarks on Composed Method the other day, [GeePaw Hill] said that he likes to say that a method should contain at most one control structure. So let’s clean that up. First, we can move the setting of current_direction inside the if:

    def update_next(self, origin, current_direction):
        if self.current_direction != current_direction:
            self.should_reverse = False
            self.current_direction = current_direction
        if self._next_invader >= len(self.invaders):
            return self.end_cycle()
        invader = self.next_invader()
        invader.position = origin
        self._next_invader += 1
        return CycleStatus.CONTINUE

Commit: tidying.

Extract Method:

    def update_next(self, origin, current_direction):
        self.handle_direction_change(current_direction)
        if self._next_invader >= len(self.invaders):
            return self.end_cycle()
        invader = self.next_invader()
        invader.position = origin
        self._next_invader += 1
        return CycleStatus.CONTINUE

    def handle_direction_change(self, current_direction):
        if self.current_direction != current_direction:
            self.should_reverse = False
            self.current_direction = current_direction

Commit: tidying.

Now add an else to the remaining if:

    def update_next(self, origin, current_direction):
        self.handle_direction_change(current_direction)
        if self._next_invader >= len(self.invaders):
            return self.end_cycle()
        else:
            invader = self.next_invader()
            invader.position = origin
            self._next_invader += 1
            return CycleStatus.CONTINUE

Commit: tidying.

Extract Method:

    def update_next(self, origin, current_direction):
        self.handle_direction_change(current_direction)
        return self.perform_update_step(origin)

    def perform_update_step(self, origin):
        if self._next_invader >= len(self.invaders):
            return self.end_cycle()
        else:
            invader = self.next_invader()
            invader.position = origin
            self._next_invader += 1
            return CycleStatus.CONTINUE

Commit. Extract Method:

    def perform_update_step(self, origin):
        if self._next_invader >= len(self.invaders):
            return self.end_cycle()
        else:
            self.move_one_invader(origin)
            return CycleStatus.CONTINUE

    def move_one_invader(self, origin):
        invader = self.next_invader()
        invader.position = origin
        self._next_invader += 1

Let’s invert that if and see if we like it better. Commit.

    def perform_update_step(self, origin):
        if self._next_invader < len(self.invaders):
            self.move_one_invader(origin)
            return CycleStatus.CONTINUE
        else:
            return self.end_cycle()

We do like it better. Commit. I’ll show you where we stand, and then we’ll sum up.

    def update_next(self, origin, current_direction):
        self.handle_direction_change(current_direction)
        return self.perform_update_step(origin)

    def handle_direction_change(self, current_direction):
        if self.current_direction != current_direction:
            self.should_reverse = False
            self.current_direction = current_direction

    def perform_update_step(self, origin):
        if self._next_invader < len(self.invaders):
            self.move_one_invader(origin)
            return CycleStatus.CONTINUE
        else:
            return self.end_cycle()

    def move_one_invader(self, origin):
        invader = self.next_invader()
        invader.position = origin
        self._next_invader += 1

    def end_cycle(self):
        self._next_invader = 0
        return CycleStatus.REVERSE if self.should_reverse else CycleStatus.NEW_CYCLE

    def at_edge(self, bumper_direction):
        if self.current_direction == bumper_direction:
            self.should_reverse = True

    def next_invader(self):
        return self.invaders[self._next_invader]

Summary

So the bear bit me today: I briefly committed a bug. The cause was failure to quickly try the game, even though I had sufficient reason to think that my tests were adequate. Or, the cause was over-confidence. Or, the cause was impatience. Or, the cause was a bad habit, committing without running the game.

For best results, I need better tests, despite having written some pretty good ones over the past few days. No real news there, we all need better tests, every day. What, only me? Probably not only me.

The final refactoring makes the code’s behavior more clear to me, so I am pleased with that. The object is a bit more robust, a bit better tested, a bit more clear, and a bit more efficient.

But it has taken me a couple of hours, and I feel that the payoff was a bit low for that investment. That said, this may be the most complicated thing in either of the two games, because of the two-stage movement of the Invaders, where we move just one every 60th of a second, while the entire rest of the universe moves everything every 60th.

However, the fact that it is complicated does not justify breaking it, and calls for the code to be more clear, not less clear, so I’d argue that rather than back away slowly, we do owe it to ourselves to bear down on this and get it as solid as we can.

Is there code in your system that the team fears to change? Code that seems to work but is so fragile that you’re tempted to work around it rather than in it? What should be done about it? Should you work carefully to make it better, or is it better to put up an electric fence and stay away?

Today’s brief release of a bug suggests running away. And sometimes, running away is the best thing we can do. My own desire is to make it better, but I have the luxury that the worst that can happen to me is that someone sees me make a mistake, since I work in public. But I’m used to making mistakes: they are part of our work. The real issue is what we do in response to the mistakes.

What I’ve done so far here is improve the tests, although I think more improvement is necessary, and I’ve improved the logic of reversal a bit, although I think that, too, could be improved.

What can we do to make it better still? I don’t know … one more idea comes to mind … what it instead of a two-state process, reflected in a should_reverse flag and a REVERSE return, we had a three-state setup, where we consider two events, invaders entering bumper and invaders leaving bumper, with states “before”, “during”, and “after”?

I do not know. I have set myself the task of, one day, knowing. Today is not that day. Perhaps tomorrow will be.

Something will happen. See you then!