Spike Removal

Python Asteroids+Invaders on GitHub

I promised to throw this spike away and do it over. Can we talk about this? Includes words like ‘folderol’ and ‘bugaboo’.

The desire was to improve the code (and logic) for moving the invaders. The approach was to spike in some changes, to learn how to do it. The promise was to remove the spike when done, and do it again.

I don’t know who created the rule that you always have to remove a spike solution and do it over. Whoever it was, I would like to have a few words with them, but of course they would take the moral high ground and say that of course I can work forward from the spike if I really want to, but that their experience is that they get even better solutions when they revert and solve the problem a second time.

As frequent readers know, I generally do not tell you what you should do, nor even advise you on what you might do. I don’t like to be told what to do, so in an attempt to seem fair, I try not to tell others what they should do, much less what they must do in order to be members of the Fancy Code Club or whatever group one might represent. Anyway, no one really aspires to be a member of the Incredibly Old To Still Be Programming Club, at least not any time soon.

Now the fact is, I do have a lot of experience with spiking in a solution, learning from it, reverting, and doing it again, and my experience is that it never takes very long and the result is generally better than the spike. I just don’t want to. I’ve developed the habit of always going forward from wherever the code is, and rarely if even reverting, other than the very occasional quick Command+Z or the even more infrequent rolling back of a half-hour or hour of really heading in the wrong direction.

But I did promise, so I’m going to do it. But one of things I’ve learned in this spike is that I need more tests. I propose to write them, keep them, revert the production code, and then make the tests work as part of the deal.

To figure out what new tests to write, let’s review what we’ve changed.

Review

Invader motion is odd. The InvaderFleet holds an InvaderGroup, which holds all the live invaders. The InvaderFleet acts like a standard “Flyer” in our design, receiving all the main cycle messages. It forwards the ones it cares about to the InvaderGroup. The InvaderGroup holds a collection of all the actual Invaders.

The reason for all this folderol is that Invaders move one at a time, one every 60th of a second, which gives their motion a sort of rippling effect on the screen. The Fleet has an “origin” point, the position of the zeroth Invader. When the Fleet gets an update every 60th of a second, it forwards to the Group, which moves just one Invader to use the current origin.

When all the invaders have been moved to the new origin, the Group returns NEW_CYCLE to the Fleet, which will only then advance the origin and the rippling starts all over again.

The bugaboo comes in determining when the invaders should reverse, marching back across the screen, having hit the edge. In the design we’re about to revert to, each invader that finds itself hitting a side bumper sends a message directly to the Fleet, saying at_edge, and including a value from the bumper that it hit. The fleet knows which direction the invaders are moving, and comparing that with the value from the bumper, it determines whether to reverse or not.

However, since the invaders move one at a time, there are many cycles where at least one invader is in the bumper, and all the invaders who are in there keep sending the at_edge message.

So it’s messy and we’re here to fix it.

Our new scheme has the Group keeping track of whether a reverse will be needed at the end of the cycle, and communicating that explicitly to the Fleet. In our present spike, the many at_edge messages go to the Group. We’d like to reduce that as well, but can consider it a separate problem to be solved later.

Plan

The spike we have and the plan for what we’re about to do are quite similar. We need to

Pass the Group to the Invader on the call to interact_with_bumper, to
Allow the InvaderGroup to be sent the at_edge messages, so that
InvaderGroup can provide a new return from update+next, REVERSE;
Which InvaderFleet can use directly to decide whether to reverse.

We would like to do this in small steps, breaking nothing along the way. I believe that to do that safely, we need some more tests.

Test Review

With the spike in place, the current tests do not even notice if the at_edge message is entirely ignored. We have tests for how the InvaderFleet responds to REVERSE and the other returns, but none that actually check whether the Invaders turn around.

We have these tests:

    def test_remove_last_invader(self):
        group = InvaderGroup()
        for count in range(55):
            group.kill(group.invaders[0])
        result = group.update_next(Vector2(0, 0), 1)
        assert result == CycleStatus.NEW_CYCLE

    def test_update_next(self):
        group = InvaderGroup()
        origin = Vector2(100, 100)
        for i in range(55):
            result = group.update_next(origin, 1)
            assert result == CycleStatus.CONTINUE
        result = group.update_next(origin, 1)
        assert result == CycleStatus.NEW_CYCLE

    def test_end_at_edge_steps_down_and_left(self):
        fleet = InvaderFleet()
        origin = fleet.origin
        direction = fleet.direction
        fleet.process_result(CycleStatus.REVERSE)
        assert fleet.direction == -direction
        assert fleet.origin == origin - fleet.step + fleet.down_step

    def test_end_increments_step(self):
        fleet = InvaderFleet()
        origin = fleet.origin
        fleet.process_result(CycleStatus.NEW_CYCLE)
        assert fleet.origin == origin + fleet.step

    def test_ok_leaves_step_alone(self):
        fleet = InvaderFleet()
        origin = fleet.origin
        fleet.process_result(CycleStatus.CONTINUE)
        assert fleet.origin == origin

These tests do check that the InvaderFleet will do the right behavior given the right cycleStatus. We do not have tests for the Group, showing that it will return REVERSE at the right time. I think that if we had that, we would have sufficient tests to drive out our new (re-) implementation of this feature.

What is the simplest test that could possibly work? How about this:

Set up an InvaderGroup. No invaders will be in the bumper.
Interact with a bumper. No one is colliding.
Check that at cycle end, the return is NEW_CYCLE
Set an invader to colliding with the bumper.
Interact with bumper again. One is colliding.
Check that at cycle end, the return is REVERSE.

We do not really have a cycle end in Group. What do we have now?

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

We can see there that we should be able to extract the end of cycle logic pretty easily. Let’s write the test first.

This is enough to drive out the method:

    def test_reversal(self):
        group = InvaderGroup()
        bumper = Bumper(u.BUMPER_RIGHT, +1)
        group.interact_with_bumper(bumper, None)
        result = group.end_cycle()
        assert result == CycleStatus.NEW_CYCLE

Failing now, for want of end_cycle. We extract:

    def update_next(self, origin, current_direction):
        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
        if self.should_reverse:
            self.should_reverse = False
            return CycleStatus.REVERSE
        else:
            return CycleStatus.NEW_CYCLE

Test runs so far. Test the other side. I think I’ll do a separate test, renaming the one we have.

These two pass:

    def test_no_reversal(self):
        group = InvaderGroup()
        bumper = Bumper(u.BUMPER_RIGHT, +1)
        group.interact_with_bumper(bumper, None)
        result = group.end_cycle()
        assert result == CycleStatus.NEW_CYCLE

    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

This is not really quite enough. Recall that the invaders really stay in collision with the bumper for a long time as the ripple takes them in one at a time and back out one at a time. And when the ripple is taking them out, we do not want another REVERSE. Presently we handle that by passing the current direction from the Fleet, down to the Group, which uses it to detect whether a colliding invader is moving toward the bumper, or away.

I can certainly write the other test. Let’s do that and then think about its implications.

    def test_no_reversal_on_exit(self):
        group = InvaderGroup()
        group.current_direction = -1
        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.NEW_CYCLE

Test is green. All tests are green. The objection to this one is that it assumes the solution to the problem is for the group to know its direction, which is a very detailed thing to decide at this point.

I think we’ll allow it, in the spirit of having decent tests. So let’s commit the test files: tests for new implementation of update cycle. I commit them, but I do not push. No, we should in fact push, these tests are legit.

And now I roll back my other updates.

Done. That kind of hurt. Six tests now fail.

Darn, I wish I had not pushed those tests. Now I can’t push my code until all the tests run. It would have looked better to have pushed new tests and new code, always keeping things going. Bummer.

Oh well, we’ll do our best. Some tests that are failing are the ones calling for end_cycle. Here is update_next as it now stands:

    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

Well, I can certainly extract end_cycle, so let’s do:

    def update_next(self, origin):
        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.NEW_CYCLE

One test passes. One that is failing gives me a useful hint:

    def test_update_next(self):
        group = InvaderGroup()
        origin = Vector2(100, 100)
        for i in range(55):
            result = group.update_next(origin, 1)
            assert result == CycleStatus.CONTINUE
        result = group.update_next(origin, 1)
        assert result == CycleStatus.NEW_CYCLE

Currently, update_next only wants one argument, but the tests are sending two. The second one is the direction. I can follow the hint and put that in.

    def update_next(self, origin, current_direction):
        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

One more test passes.

We have some failing, because now the Fleet isn’t passing in the current direction.

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

Now only three tests failing, down from six. Most, if not all, will have to do with REVERSE, I’m guessing.

Note: Some of my betters would tell us that we “should” always predict what tests will fail and—I guess—feel sad if we get it wrong. I prefer to trust the tests to figure that out and generally do not make such a prediction. Would I be better, or better off, if I did? Perhaps.

This test seems quite helpful:

    def test_end_at_edge_steps_down_and_left(self):
        fleet = InvaderFleet()
        origin = fleet.origin
        direction = fleet.direction
        fleet.process_result(CycleStatus.REVERSE)
        assert fleet.direction == -direction
        assert fleet.origin == origin - fleet.step + fleet.down_step

There is no such CycleStatus. We’ll add it.

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

Three tests still failing. One of them is the same one, reporting that we do not step down. We need to improve process_result, which looks like this:

    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()

We would like to deal with REVERSE, and harm nothing else. So:

    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()
        elif result == CycleStatus.REVERSE:
            self.reverse_travel()

Reflection

Let’s pause a moment to notice something. I have done no design, no thinking about who goes where or does what. I’ve been guided entirely by the tests that aren’t working. Yes, I have paid some attention to easy ones, but so far nothing that feels like deep design thinking has crossed my attention.

I am honestly a bit surprised. I had thought that I was going to have to recreate my thinking process and do it all carefully and such. So far, not the case. Just following the tests. Will that continue? Let’s find out.

Carrying On …

Only two tests failing.

    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

    def test_no_reversal_on_exit(self):
        group = InvaderGroup()
        group.current_direction = -1
        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.NEW_CYCLE

These are the two new ones written to drive out the new Group behavior. They are both failing because I have passed no Fleet to the interact_with_bumper and so the Invader, when it tries to call back, has no one to talk with.

But we want it to talk to the group. So in InvaderGroup, we currently have this:

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

And we want them to call us back, not the Fleet, so:

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

Still two failures, because the InvaderGroup does not implement at_edge.

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

Here I actually had to think for a moment to decide what to do, but basically this is the same as what is in the Fleet now. We want to reverse if we are colliding with a bumper and moving toward it.

I think this code is a small crock, but we’ll allow it. We now have just one failing 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

Expected :<CycleStatus.REVERSE: 'reverse'>
Actual   :<CycleStatus.NEW_CYCLE: 'new cycle'>

And in InvaderGroup end_cycle, we have this:

    def end_cycle(self):
        self._next_invader = 0
        return CycleStatus.NEW_CYCLE

And require this:

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

Love me some ternaries. We are green. Must test in game, I’m not a complete fool. We are good.

We can remove at_edge from InvaderFleet, I should think. Hm, something failed. The test is no longer applicable:

    def test_fleet_motion(self):
        fleet = InvaderFleet()
        # fleet.step = Vector2(30, 0)
        pos = fleet.testing_only_invaders[0].position
        fleet.update(1.0, None)
        new_pos = fleet.testing_only_invaders[0].position
        assert fleet.direction == +1
        assert new_pos - pos == fleet.step
        fleet.at_edge(+1)
        assert fleet.reverse

Remove it.

We can commit these new changes: invader edge processing moved to InvaderGroup, returning new REVERSE status.

But wait, that reminds me of this, in 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()
        elif result == CycleStatus.REVERSE:
            self.reverse_travel()

Now we do not need to check reverse, nor do we need the variable.

    def process_result(self, result):
        if result == CycleStatus.CONTINUE:
            pass
        elif result == CycleStatus.NEW_CYCLE:
            self.step_origin()
        elif result == CycleStatus.REVERSE:
            self.reverse_travel()

Green. Commit: invader edge processing moved to InvaderGroup, returning new REVERSE status.

This article is getting long. Let’s sum up.

Summary

Yesterday morning, I did a lot of thinking, and only made one tiny change, the conversion to an enum for cycle status.

In the afternoon, I spiked in code that allowed the InvaderGroup to field the at_edge messages and return the proper result to the InvaderFleet, allowing it to just reverse or not as needed. I did that work “by intention”, in that the first change I made was to require the new REVERSE status to work before it was even implemented I wrote the above method first, just exactly as it stands now … done last.

What followed was a lot of analysis and some test enhancements, followed by changing the Group to pass itself to the Invader bumper logic, requiring it to implement at_edge, the second actual code change. That was nearly the last change we made this morning. Yesterday, changing things so that Group received aat_edge, we discovered the need for the direction to come into the group, and provided that. And basically at that point the spike worked.

This morning, I started with some careful thinking, not about the design, but about the tests, and I wrote some new ones to check the Group’s behavior. Those tests were, I freely grant, a bit specific, in that they knew that the Group needed to handle at_edge and that it needed to know the direction of movement.

In essence, though I wasn’t really thinking that way, those tests nailed down some design specifics about what the objects would know and when they would know it.

Then we reverted and just stepped through making the tests run. My mental process rather surprised me. I had been thinking about the design while away from the computer and I was expecting to need to think about it during the implementation, in order to find a clever way to proceed. Instead, I just looked at the broken tests and picked one that seemed easy to make work, and basically ticked through them.

Each individual step made sense on its own, and when the tests all ran, we were good to go. I didn’t need to concentrate or think deeply, it was just tick tick tick.

Now of course, part of the not thinking deeply was that I had just done the spike, so I was quite familiar with the code, and pretty familiar with what went where and when it went there, so in a sense the deep thinking was yesterday and I got the benefit of it today.

But … it went so very smoothly and with no stress, no wondering, no confusion … bada boom bada bing, in and done.

Certainly there is more that could be done. In particular, I think ripping the incoming direction out of the bumper and passing it upward to the group is a bit naff. We can look at that in another session. And we haven’t dealt with the hundreds of calls, we have just changed the object that receives them. That, too, we can look at later.

What we have now is a nice clean test-driven improvement to the system, with the Fleet handling matters more relating to its work, and the Group handling more details. Strategy upward, details downward. This is the way.

Very pleasant. Now to rest up in preparation for turkey. See you next time!