Python Asteroids on GitHub

I’m going to take another try at improving the ShotOptimizer code. Not because we really need to, but because we want to learn how to make code better. That’s my story and I’m sticking to it.

It’s 0645, which I consider a civilized hour to get up, and I have a plan for improving the ShotOptimizer. But first …

Demonstrating Aliasing

To confirm yesterday’s discovery that Vector2 += aliases, I want to write a test for aliasing:

    def test_show_vector_aliasing(self):
        original = Vector2(100, 100)
        copied = original
        copied += Vector2(10, 20)
        assert copied == Vector2(110, 120)
        assert original == copied  # !!!! aliasing.

This test passes! We modify copied and original is also modified. Why? Well, because it is. And because if one were to naively implement vector a+=b it would probably look like this:

a.x += b.x
a.y += b.y

That code would change vector a in place, mutating its internals, unlike a regular add a+b, which might look like this, creating a new vector to be subsequently saved:

return Vector(a.x+b.x, a.y+b.y)

I don’t know what pygame Vector2 really does internally. Perhaps it is open-source and we could look. But the test above proves that it will alias if we don’t copy first.

So that nails down yesterday’s discovery of the aliasing issue. Let’s get to our real purpose this morning. Now then …

Improving ShotOptimizer

I want to make the ShotOptimizer code more clear, if I can. The relevant code bits look like this:

class ShotOptimizer:
    @property
    def targeted_solution(self):
        if not self.ship:
            return self.random_solution
        shooter_position = self.saucer.position
        best_target_position = self.closest_aiming_point(shooter_position, self.ship.position, u.SCREEN_SIZE)
        safe_distance = self.saucer.missile_head_start
        target_position = self.lead_the_target(best_target_position, safe_distance, shooter_position)
        return FiringSolution(target_position, shooter_position, safe_distance, 1)

    def lead_the_target(self, best_target_position, safe_distance, shooter_position):
        target_position = best_target_position
        for _ in range(3):
            target_position = self.improved_aiming_point(
                target_position,
                self.ship.velocity,
                best_target_position,
                shooter_position,
                u.MISSILE_SPEED,
                safe_distance)
        return target_position

    @staticmethod
    def improved_aiming_point(
            initial_aiming_point,
            ship_velocity,
            ship_position,
            saucer_position,
            missile_speed,
            missile_offset):
        missile_travel_distance = saucer_position.distance_to(initial_aiming_point) - missile_offset
        missile_travel_time = missile_travel_distance / missile_speed
        ship_motion = missile_travel_time * ship_velocity
        anticipated_ship_position = ship_position + ship_motion
        return anticipated_ship_position

With luck, we’ll come back to that 1 parameter in FiringSolution, which I think is useless, or perhaps used in testing. For now, our interest is in lead_the_target and improved_aiming_point.

N.B.
No such luck. Maybe another time.

One big issue with the above code is that the six-element calling sequence to improved_aiming_point makes both the call and the implementation so hard to read that we are likely to miss the essential notion of working out an iterative solution.

I propose to improve that by extracting another class.

Extracting a Class

I propose to create a new class, called, I don’t know, AimAdjuster, which receives all the parameters we need, and implements a simpler adjustment method. I believe this will make things better, but I’m not certain. The title of this article, “One More Time” may be optimistic. That’ OK: we’re here to find better ways of expressing ourselves in code.

I would like to just extract the thing without TDDing a new class. Let’s see what we might do.

How about creating a raft of new members right in ShotOptimizer? Let’s try this:

    # noinspection PyAttributeOutsideInit
    def lead_the_target(self, best_target_position, safe_distance, shooter_position):
        self.original_target_position = best_target_position
        self.target_velocity = self.ship.velocity
        self.shooter_position = shooter_position
        self.safe_distance = safe_distance
        self.missile_speed = u.MISSILE_SPEED
        target_position = copy(best_target_position)
        for _ in range(3):
            target_position = self.improved_aiming_point(target_position)
        return target_position

    def improved_aiming_point(self, initial_aiming_point):
        missile_travel_distance = self.shooter_position.distance_to(initial_aiming_point) - self.safe_distance
        missile_travel_time = missile_travel_distance / self.missile_speed
        ship_motion = missile_travel_time * self.target_velocity
        anticipated_ship_position = self.original_target_position + ship_motion
        return anticipated_ship_position

That’s still a bit messy, let’s extract all that member creation:

    def lead_the_target(self, best_target_position, safe_distance, shooter_position):
        self.set_up_aim_calculation(best_target_position, safe_distance, shooter_position)
        target_position = copy(best_target_position)
        for _ in range(3):
            target_position = self.improved_aiming_point(target_position)
        return target_position

    def improved_aiming_point(self, initial_aiming_point):
        missile_travel_distance = self.shooter_position.distance_to(initial_aiming_point) - self.safe_distance
        missile_travel_time = missile_travel_distance / self.missile_speed
        ship_motion = missile_travel_time * self.target_velocity
        anticipated_ship_position = self.original_target_position + ship_motion
        return anticipated_ship_position

    # noinspection PyAttributeOutsideInit
    def set_up_aim_calculation(self, best_target_position, safe_distance, shooter_position):
        self.original_target_position = best_target_position
        self.target_velocity = self.ship.velocity
        self.shooter_position = shooter_position
        self.safe_distance = safe_distance
        self.missile_speed = u.MISSILE_SPEED

That makes the main line more quiet.

I have broken tests, and I am not surprised. Let’s look at one: I’m pretty sure they’ll all have the same issue:

    def test_aiming_point(self):
        ship_position = Vector2(100, 100)
        ship_velocity = Vector2(10, 10)
        saucer_position = Vector2(0, 0)
        missile_speed = 100
        starting_distance = 141.42  # trig
        flight_time = starting_distance/100
        ship_move = ship_velocity*flight_time
        new_ship_position = ship_position + ship_move
        new_target = ShotOptimizer.improved_aiming_point(ship_position, ship_velocity, ship_position, saucer_position, missile_speed, 0)
        dist = new_target.distance_to(new_ship_position)
        assert dist < 0.001

Right. The calling sequence has changed. I did the code above by hand, not with Change Signature. Even if I had used Change Signature, these tests would fail because they’re not setting up the magic variables.

This is evil, but what if I were to call the set up function from this test?

No. The real issue is that this isn’t a test of ShotOptimizer but of what happened to be a static method. It’s not even a static method any more. Let’s recast this test correctly, if we can.

No. It’s too far to reach if I try to set up a ship and saucer situation. I can’t just transform these tests, I’ll need to rewrite them.

Let’s push forward. We might come out OK, and if we don’t, we’ll know more about how to do this. I can always change the title.

Let’s have our set up method return a class: the ShotOptimizer for now:

    def lead_the_target(self, best_target_position, safe_distance, shooter_position):
        aim_improver = self.set_up_aim_calculation(best_target_position, safe_distance, shooter_position)
        target_position = copy(best_target_position)
        for _ in range(3):
            target_position = aim_improver.improved_aiming_point(target_position)
        return target_position

    # noinspection PyAttributeOutsideInit
    def set_up_aim_calculation(self, best_target_position, safe_distance, shooter_position):
        self.original_target_position = best_target_position
        self.target_velocity = self.ship.velocity
        self.shooter_position = shooter_position
        self.safe_distance = safe_distance
        self.missile_speed = u.MISSILE_SPEED
        return self

Now we have an imaginary class AimImprover, currently played by ShotOptimizer. But we can readily create and use the class:

    # noinspection PyAttributeOutsideInit
    def set_up_aim_calculation(self, best_target_position, safe_distance, shooter_position):
        self.original_target_position = best_target_position
        self.target_velocity = self.ship.velocity
        self.shooter_position = shooter_position
        self.safe_distance = safe_distance
        self.missile_speed = u.MISSILE_SPEED
        return AimImprover(self.original_target_position, self.target_velocity, self.shooter_position, self.missile_speed, self.safe_distance)

There is no such class … yet. Inline all the self things.

    def set_up_aim_calculation(self, best_target_position, safe_distance, shooter_position):
        return AimImprover(best_target_position, self.ship.velocity, shooter_position, u.MISSILE_SPEED, safe_distance)

Now to create AimImprover:

class AimImprover:
    def __init__(self, best_target_position, target_velocity, shooter_position, missile_speed, safe_distance):
        self.original_target_position = best_target_position
        self.target_velocity = target_velocity
        self.shooter_position = shooter_position
        self.missile_speed = missile_speed
        self.safe_distance = safe_distance

    def improved_aiming_point(self, initial_aiming_point):
        missile_travel_distance = self.shooter_position.distance_to(initial_aiming_point) - self.safe_distance
        missile_travel_time = missile_travel_distance / self.missile_speed
        ship_motion = missile_travel_time * self.target_velocity
        anticipated_ship_position = self.original_target_position + ship_motion
        return anticipated_ship_position

I just sort of copied and pasted this in. Same three tests are failing. I think this works and I test in the game to find out. It does seem to work.

Now I should be able to change my ShotOptimizer tests to AimImprover tests, I think.

Here’s the first one fixed up:

    def test_aiming_point(self):
        ship_position = Vector2(100, 100)
        ship_velocity = Vector2(10, 10)
        saucer_position = Vector2(0, 0)
        missile_speed = 100
        starting_distance = 141.42  # trig
        flight_time = starting_distance/100
        ship_move = ship_velocity*flight_time
        new_ship_position = ship_position + ship_move
        improver = AimImprover(ship_position, ship_velocity, saucer_position, missile_speed, 0)
        new_target = improver.improved_aiming_point(ship_position)
        dist = new_target.distance_to(new_ship_position)
        assert dist < 0.001

With similar changes to the other two tests, we are green!

After inlining the temporary set_up method, ShotOptimizer looks like this:

    @property
    def targeted_solution(self):
        if not self.ship:
            return self.random_solution
        shooter_position = self.saucer.position
        best_target_position = self.closest_aiming_point(shooter_position, self.ship.position, u.SCREEN_SIZE)
        safe_distance = self.saucer.missile_head_start
        target_position = self.lead_the_target(best_target_position, safe_distance, shooter_position)
        return FiringSolution(target_position, shooter_position, safe_distance, 1)

    def lead_the_target(self, best_target_position, safe_distance, shooter_position):
        aim_improver = AimImprover(best_target_position, self.ship.velocity, shooter_position, u.MISSILE_SPEED,
                                   safe_distance)
        target_position = copy(best_target_position)
        for _ in range(3):
            target_position = aim_improver.improved_aiming_point(target_position)
        return target_position

And AimImprover is as it was up above. We are green and good to go. Commit: Added AimImprover class in attempt to make shot optimization more clear.

Let’s reflect and sum up.

Reflection

Today went well, in part due to a few earlier attempts to improve things. (See articles #181 and #182 for gory details.) The process was interesting, and if it is a known pattern for extracting a class, it was nonetheless new to me.

We began by creating a setup method that stored all the parameters to our method with the long calling sequence, and then changing the method to accept only what changed as part of the calling sequence, and using member variables instead.

That is generally not considered to be a good practice, because it is thought that all the members of a class should change at about the same frequency. So saving things just to shorten calling sequences is thought to be a bit naff.

But then we “cleverly” returned self as if we had created a class, and sent our improved_aiming_point method to that. And then, we actually created the new class, for which we had all the parameters right there in hand, and returned it instead. We moved the improved_aiming_point method over with essentially no change.

And there it was. We rewired the tests to point to the thew object and everything was good.

It really went quite smoothly. Not bad at all for, what, about 15 attempts at various ideas?

Summary

Yes, it really did take a lot of tries to make this code better (by my lights), and I freely grant that it doesn’t improve Asteroids much if at all.

What it does improve is my mind, my ability to improve code, and my general sense of being in a good place in this program, and in my head. And that is why, quite often, when we can afford them, we do these things: they make our lives a bit better.

I feel better about the code in lead_the_target, because it better reflects that we are improving the aiming point in each iteration. The code in AimImprover itself is able to be directly tested, although the tests, which I’ll post below, are not exactly marvels of obvious clarity.

The improved_aiming_point code is no less clear than it was, and now it is the only operational method in its own class rather than a static method in ShotOptimizer. We have come to suspect static methods of wanting to be on some other class, and that seems to have been the case here.

We didn’t return to the 1 parameter in FiringSolution. Perhaps another time. I would prefer to see the method be more like adjustment+=improver.adjust(), and perhaps we’ll try that another time as well. Nonetheless …

I think this is perfect … and it could still use a little improvement. (Pace Suzuki Roshi.) We’ll save any further improvement for another time.

I hope you’ll join me then, and comment or question on mastodon.social, where I am ronjeffries.

See you soon!


    def test_aiming_point(self):
        ship_position = Vector2(100, 100)
        ship_velocity = Vector2(10, 10)
        saucer_position = Vector2(0, 0)
        missile_speed = 100
        starting_distance = 141.42  # trig
        flight_time = starting_distance/100
        ship_move = ship_velocity*flight_time
        new_ship_position = ship_position + ship_move
        improver = AimImprover(ship_position, ship_velocity, saucer_position, missile_speed, 0)
        new_target = improver.improved_aiming_point(ship_position)
        dist = new_target.distance_to(new_ship_position)
        assert dist < 0.001

    def test_iterated_aiming_point(self):
        ship_position = Vector2(100, 100)
        ship_velocity = Vector2(10, 10)
        saucer_position = Vector2(0, 0)
        missile_speed = 100
        new_target = ship_position
        improver = AimImprover(ship_position, ship_velocity, saucer_position, missile_speed, 0)
        for _ in range(3):
            new_target = improver.improved_aiming_point(new_target)
        ship_speed = ship_velocity.length()
        ship_move_distance = ship_position.distance_to(new_target)
        ship_time = ship_move_distance / ship_speed
        missile_move_distance = saucer_position.distance_to(new_target)
        missile_time = missile_move_distance / missile_speed
        assert ship_time == pytest.approx(missile_time, 0.01)

    def test_iterated_offset_aiming_point(self):
        ship_position = Vector2(100, 100)
        ship_velocity = Vector2(10, 10)
        saucer_position = Vector2(0, 0)
        missile_speed = 100
        missile_offset = 20
        new_target = ship_position
        improver = AimImprover(ship_position, ship_velocity, saucer_position, missile_speed, missile_offset)
        for _ in range(3):
            new_target = improver.improved_aiming_point(new_target)
        ship_speed = ship_velocity.length()
        ship_move_distance = ship_position.distance_to(new_target)
        ship_time = ship_move_distance / ship_speed
        missile_move_distance = saucer_position.distance_to(new_target) - missile_offset
        missile_time = missile_move_distance / missile_speed
        assert ship_time == pytest.approx(missile_time, 0.01)