Better Attract Mode?

There are issues. A long, slightly fruitful refactoring. A much more deadly attract mode that plays better than I do.

Issues include:

I don’t know if you noticed in yesterday’s video, but the Driver doesn’t always shoot directly at the center of the Invader, it seems sometimes to be a bit off to one side or the other. I think I know why: we get the invader x positions one cycle before we do our update to move and fire. I think that means that our move target can be off by one invader step. But it depends a bit on whether we are shooting at a high up or low down invader, and on how fast the invaders are moving.
The invaders are not firing shots yet. They only fire when there is a player on the screen and they do not know that the Driver is a player.
Game Over comes up too soon after the last player is hit, so that we don’t even get to see his explosion. We should fix that, and perhaps even lengthen the delay further. I think our decision on that will depend a bit on what the code wants.
We have no tests for the Driver’s update logic.

Let’s begin with the one I least want to do, the test for update:

class Driver(Spritely, InvadersFlyer):

    def update(self, delta_time, fleets):
        target = self.nearest(self.invader_x_values)
        self.step_toward(target)
        self.count = (self.count + 1) % 60
        if not self.count:
            fleets.append(PlayerShot(self._sprite.center))

Oh this also raises an issue: we currently fire once per second. I would like to change that, so that it fires as often as it can (and perhaps only when in the open).

Let’s do another extract here, and show a bit more detail, to get some meaningful names for what’s going on. This will help us think about what we might want to test here.

    def update(self, delta_time, fleets):
        target = self.nearest(self.invader_x_values)
        self.step_toward(target)
        self.fire_when_ready(fleets)

    def fire_when_ready(self, fleets):
        self.count = (self.count + 1) % 60
        if not self.count:
            fleets.append(PlayerShot(self._sprite.center))

    def step_toward(self, target):
        step = 0
        if self._sprite.centerx < target:
            step = self.step
        elif self._sprite.centerx > target:
            step = -self.step
        centerx = self._sprite.centerx + step
        self.position = Vector2(centerx, self.position.y)

PyCharm puts a little usage indicator above each def, telling me how many usages each method has. The two last ones here have only one, which tells me they have no tests. Let’s do yet another extract.

I actually did two and some renaming:

    def update(self, delta_time, fleets):
        target_x = self.nearest_invader_x(self.invader_x_values)
        self.take_one_step_toward_target(target_x)
        self.fire_when_ready(fleets)

    def take_one_step_toward_target(self, target_x):
        step_x = self.one_step_toward_target(target_x)
        self.take_one_step(step_x)

    def take_one_step(self, step_x):
        centerx = self._sprite.centerx + step_x
        self.position = Vector2(centerx, self.position.y)

    def one_step_toward_target(self, target_x):
        step_x = 0
        if self._sprite.centerx < target_x:
            step_x = self.step
        elif self._sprite.centerx > target_x:
            step_x = -self.step
        return step_x

I thought it best to make it more clear that we are just dealing in the x coordinate. But my real point was that now there are a lot of little methods that we can test. We could have built it this way, at least in principle, and test-driven it, but I was not able to see this granularity from there. I can see it now and I want it tested. And, you know what, I still don’t entirely like this code.

Let me see if I can explain why. The way it works now, we call down and down and we access _sprite.centerx three times. This is not a matter of efficiency, it is a matter of meaning. We have a particular point in mind, centerx, and we want to get the nearest invader x to that point, and we want a step toward that point. And all three of those references need to be to the same value, or this doesn’t make sense.

Let’s try something weird. Let’s first commit this good code: refactoring for better names.

Now let’s inline that code all the way back and see if we can refactor it differently.

    def nearest_invader_x(self, x_values):
        possibles = self.select_values_in_open(x_values)
        if not possibles:
            return self.position.x
        best_x = possibles[0]
        for x in possibles:
            if abs(self.position.x - x) < abs(self.position.x - best_x):
                best_x = x
        return best_x

    def update(self, delta_time, fleets):
        target_x = self.nearest_invader_x(self.invader_x_values)
        x = 0
        if self._sprite.centerx < target_x:
            x = self.step
        elif self._sprite.centerx > target_x:
            x = -self.step
        step_x = x
        centerx = self._sprite.centerx + step_x
        self.position = Vector2(centerx, self.position.y)
        self.fire_when_ready(fleets)

I note that nearest... uses self.position, not self._sprite.centerx. The former is better.

Let’s get that position in update and use it. Now that I see this, I think we might have been able to do this without the inlining but let’s see where this goes. We have a save point to roll back to if we need it.

    def nearest_invader_x(self, starting_x, x_values):
        possibles = self.select_values_in_open(x_values)
        if not possibles:
            return starting_x
        best_x = possibles[0]
        for x in possibles:
            if abs(starting_x - x) < abs(starting_x - best_x):
                best_x = x
        return best_x

    def update(self, delta_time, fleets):
        starting_x = self.position.x
        target_x = self.nearest_invader_x(starting_x, self.invader_x_values)
        x = 0
        if starting_x < target_x:
            x = self.step
        elif starting_x > target_x:
            x = -self.step
        step_x = x
        centerx = starting_x + step_x
        self.position = Vector2(centerx, self.position.y)
        self.fire_when_ready(fleets)

Now we have a more abstract notion, the starting x value, which is only tied to the actual position of the Driver in one reference rather than a larger number some of which could be wrong. Now let’s extract again. I’ll try for a better name or two.

    def update(self, delta_time, fleets):
        starting_x = self.position.x
        target_x = self.nearest_invader_x(starting_x, self.invader_x_values)
        step_x = self.one_step_toward_target(starting_x, target_x)
        centerx = starting_x + step_x
        self.position = Vector2(centerx, self.position.y)
        self.fire_when_ready(fleets)

    def one_step_toward_target(self, starting_x, target_x):
        x = 0
        if starting_x < target_x:
            x = self.step
        elif starting_x > target_x:
            x = -self.step
        step_x = x
        return step_x

When are we going to commit this? In favor of now: we are green and moving where we want to move. In favor of waiting, we’re not sure this is better, and we’re not done. In favor of now: some of these changes might be worth keeping and others might not.

I’ll wait, but I’m probably wrong to do so.

I think we can improve the one_step_toward_target method.

First remove the silly final assignment, no doubt left over from some other time.

    def one_step_toward_target(self, starting_x, target_x):
        x = 0
        if starting_x < target_x:
            x = self.step
        elif starting_x > target_x:
            x = -self.step
        return x

I want two changes here. I want to return the values directly, and I want if-elif-else form.

    def one_step_toward_target(self, starting_x, target_x):
        if starting_x < target_x:
            x = self.step
        elif starting_x > target_x:
            x = -self.step
        else:
            x = 0
        return x

And then …

    def one_step_toward_target(self, starting_x, target_x):
        if starting_x < target_x:
            return self.step
        elif starting_x > target_x:
            return -self.step
        else:
            return 0

I am tempted, so tempted, to do a nested ternary here. I will spare us all the worry.

Turning our attention back to update:

    def update(self, delta_time, fleets):
        starting_x = self.position.x
        target_x = self.nearest_invader_x(starting_x, self.invader_x_values)
        step_x = self.one_step_toward_target(starting_x, target_x)
        centerx = starting_x + step_x
        self.position = Vector2(centerx, self.position.y)
        self.fire_when_ready(fleets)

Let’s extract two more lines …

    def update(self, delta_time, fleets):
        starting_x = self.position.x
        target_x = self.nearest_invader_x(starting_x, self.invader_x_values)
        step_x = self.one_step_toward_target(starting_x, target_x)
        self.move_along_x(starting_x, step_x)
        self.fire_when_ready(fleets)

    def move_along_x(self, starting_x, step_x):
        centerx = starting_x + step_x
        self.position = Vector2(centerx, self.position.y)

And, I think one more:

    def update(self, delta_time, fleets):
        self.move_toward_target()
        self.fire_when_ready(fleets)

    def move_toward_target(self):
        starting_x = self.position.x
        target_x = self.nearest_invader_x(starting_x, self.invader_x_values)
        step_x = self.one_step_toward_target(starting_x, target_x)
        self.move_along_x(starting_x, step_x)

That’s good enough to commit: possibly better refactoring.

No, one more extract. Is this getting silly? Yes, but:

    def move_toward_target(self):
        starting_x = self.position.x
        step_x = self.get_step_toward_target(starting_x)
        self.move_along_x(starting_x, step_x)

    def get_step_toward_target(self, starting_x):
        target_x = self.nearest_invader_x(starting_x, self.invader_x_values)
        step_x = self.one_step_toward_target(starting_x, target_x)
        return step_x

Let me do some tests and get out of this loop.

    def test_one_step_calculation(self):
        driver = Driver()
        step = driver.step
        assert driver.one_step_toward_target(500, 400) == -step
        assert driver.one_step_toward_target(500, 600) == step
        assert driver.one_step_toward_target(500, 500) == 0

We have tests already for `nearest_invader_x:

    def test_find_nearest_open_x(self):
        driver = Driver()
        driver.position = (500, u.INVADER_PLAYER_Y)
        x_in = (190, 290, 490, 690, 890)
        x_out = (250, 390, 590, 790)
        x_values = x_in + x_out
        nearest_invader_x = driver.nearest_invader_x(driver.position.x, x_values)
        assert nearest_invader_x == 490

    def test_no_open(self):
        driver = Driver()
        driver.position = (500, u.INVADER_PLAYER_Y)
        x_out = (250, 390, 590, 790)
        nearest_invader_x = driver.nearest_invader_x(driver.position.x, x_out)
        assert nearest_invader_x == driver.position.x

    def test_no_invaders(self):
        driver = Driver()
        driver.position = (500, u.INVADER_PLAYER_Y)
        no_invaders = []
        nearest_invader_x = driver.nearest_invader_x(driver.position.x, no_invaders)
        assert nearest_invader_x == driver.position.x

Do we want to test move_along_x? We certainly can.

    def test_move_along_x(self):
        driver = Driver()
        driver.position = (500, u.INVADER_PLAYER_Y)
        driver.move_along_x(driver.position.x, 4)
        assert driver.position.x == 504

Honestly, I am tired of this. Let’s do fire_when_ready and get out of here.

What do we want? Let’s start by firing whenever there is no player shot on the screen.

We’ll need some flagging:

    def begin_interactions(self, fleets):
        self.invader_x_values = []
        self._can_shoot = True

    def interact_with_playershot(self, shot, fleets):
        self._can_shoot = False

    def fire_when_ready(self, fleets):
        if self._can_shoot:
            fleets.append(PlayerShot(self._sprite.center))

This is untested. Let’s test it:

    def test_firing(self):
        fleets = Fleets()
        fi = FI(fleets)
        driver = Driver()
        driver.begin_interactions(fleets)
        driver.interact_with_playershot(None, fleets)
        driver.fire_when_ready(fleets)
        assert not fi.player_shots
        driver.begin_interactions(fleets)
        driver.fire_when_ready(fleets)
        assert fi.player_shots

The driver is far more deadly now, but still makes the mistake of not clearing the left hand column, which is finally fatal. We can also see the issue with missing faster invaders:

Summary

I think I wasted a bit of time with all that refactoring to try to make Driver more testable. In retrospect, I might have saved a bit of time by just testing it in place, with somewhat larger tests. I do think I like the code better … but there is something telling me that some lower-level helper objects might be called for.

Since this is all a one-off, special purpose piece of the program, it’s probably more than good enough. If I had made it more nearly perfect, I’d be more proud. As it is, I do think it’s better code and better tested.

Driver is also a terrifyingly good player given that all it does is try to move under the closest open invader and keep firing as fast as it can. To fancy up the attract mode, there are things we could do, such as:

Randomize the start a bit so that the Driver behavior looks different each time;
Randomize the driver a bit, perhaps just delaying shots sometimes;
Give driver the ability to be concerned about invaders getting too low, shifting priority from nearest to lowest.
Maybe make it always shoot the lowest open invader? Might be interesting.

For now we have a decent attract mode and our other issues remain:

No invader shots;
Game Over comes up too fast after last Player is hit.

Could we have done more this session? I think yes. The steps we took were wandering, especially inlining all the code and breaking it out a bit differently. Sometimes that happens. I think it averages out.

Send me your thoughts if you wish.

See you next time!