Python Asteroids on GitHub

As a lark, I plugged in the Gunner. Doesn’t seem to work. Let’s see what’s up.

I changed this:

class Saucer(Flyer):
    def fire_if_possible(self, delta_time, fleets):
        self._missile_timer.tick(delta_time, self.fire_if_missile_available, fleets)

To this:

    def fire_if_possible(self, delta_time, fleets):
        ship_position = self._ship.position if self._ship else None
        Gunner().fire(delta_time, self._missile_tally, self.position, self._velocity, ship_position, fleets)

The Saucer does not fire any missiles. I am a fool! I can’t create a new one every time, it’ll restart the timer! With a Gunner() in __init__, and the change below, it works perfectly:

    def fire_if_possible(self, delta_time, fleets):
        ship_position = self._ship.position if self._ship else None
        self._gunner.fire(delta_time, self._missile_tally, self.position, self._velocity, ship_position, fleets)

Let’s clean up a bit before we commit. There are methods in Saucer that need to go away.

    def __init__(...)
        self._missile_timer = Timer(u.SAUCER_MISSILE_DELAY)  # deleted

    def fire_if_missile_available(self, fleets) -> bool:
        if self._missile_tally >= u.SAUCER_MISSILE_LIMIT:
            return False
        missile = self.create_missile()
        fleets.add_flyer(missile)
        return True


    def create_missile(self):
        should_target = random.random()
        random_angle = random.random()
        return self.suitable_missile(should_target, random_angle)

    def angle_to(self, ship):
        aiming_point = nearest_point(self.position, ship.position, u.SCREEN_SIZE)
        return Vector2(0, 0).angle_to(aiming_point - self.position)

    def suitable_missile(self, should_target, random_angle):
        if self.cannot_target_ship(should_target):
            return self.missile_at_angle(random_angle * 360.0, self._velocity)
        else:
            targeting_angle = self.angle_to(self._ship)
            velocity_adjustment = Vector2(0, 0)
            return self.missile_at_angle(targeting_angle, velocity_adjustment)

    def cannot_target_ship(self, should_target):
        return not self._ship or should_target > u.SAUCER_TARGETING_FRACTION

    def missile_at_angle(self, desired_angle, velocity_adjustment):
        missile_velocity = Vector2(u.MISSILE_SPEED, 0).rotate(desired_angle) + velocity_adjustment
        offset = Vector2(2 * self._radius, 0).rotate(desired_angle)
        return Missile.from_saucer(self.position + offset, missile_velocity)


def nearest_point(shooter, target, wrap_size):
    nearest_x = nearest(shooter.x, target.x, wrap_size)
    nearest_y = nearest(shooter.y, target.y, wrap_size)
    return Vector2(nearest_x, nearest_y)


def nearest(shooter, target, wrap_size):
    direct_distance = abs(target - shooter)
    target_wrap_left = target - wrap_size
    wrap_left_distance = abs(target_wrap_left - shooter)
    target_wrap_right = target + wrap_size
    wrap_right_distance = abs(target_wrap_right - shooter)
    if wrap_left_distance < direct_distance:
        return target_wrap_left
    elif wrap_right_distance < direct_distance:
        return target_wrap_right
    else:
        return target

Some tests are now failing, after all that deletion. Game continues OK. Check the tests, see if they can be made to work or should be removed.

Something has broken my imports. I wish I had committed. I back out some changes. Looks like removing the nearest functions has done me in.

I fix the test to import those from Gunner, and start fixing tests.


    def test_alternating_direction(self):
        Saucer.init_for_new_game()
        saucer = Saucer()
        assert saucer.position.x == 0
        assert saucer.velocity_testing_only == u.SAUCER_VELOCITY
        saucer = Saucer()
        assert saucer.position.x == u.SCREEN_SIZE
        assert saucer.velocity_testing_only == -u.SAUCER_VELOCITY
        assert saucer._zig_timer.delay == u.SAUCER_ZIG_TIME
        assert saucer._zig_timer.elapsed == 0

That one had some spurious checks on missile timer that I just removed.

I remove these tests as no longer needed:

    def test_random_missile_velocity_0(self):
    def test_random_missile_velocity_90(self):
    def test_random_missile_position_90(self):
    def test_missile_spec_targeted(self):
    def test_missile_spec_no_ship(self):
    def test_missile_spec_no_dice(self):

I feel that Gunner’s tests are solid, so I don’t feel badly about removing those. Time to commit to this. Commit: Saucer now uses Gunner for missile firing.

Let’s reflect, including a review of Gunner.

Reflection

The fact that Gunner was built with TDD gave me real confidence in it, to the point that when I installed it incorrectly I was quite surprised that it didn’t work. And when I did plug it in correctly, I was not surprised at all that it worked perfectly.

It’s interesting that when I built the capability at first, it didn’t occur to me, or at least didn’t stick with me, to build a separate targeting object like Gunner. The result was much more arms-length testing and, I think, weaker tests.

One of the nicest things I did for myself was the insertion of a method into which the random numbers are sent. That made it easy to test behavior, by sending in values that would drive the behavior I wanted to test.

Let’s review the code.

class Gunner:
    def __init__(self):
        self._timer = Timer(u.SAUCER_MISSILE_DELAY)
        self._radius = 20

What is this _radius? Well, it’s the saucer radius. That needs to be generalized, though right now we only have one size of saucer. Let’s provide for an alternative but not use it:

class Gunner:
    def __init__(self, saucer_radius=20):
        self._timer = Timer(u.SAUCER_MISSILE_DELAY)
        self._radius = saucer_radius

Commit: Gunner accepts saucer radius, defaults to magic number 20. I make a note about this.

    def fire(self, delta_time, missile_tally, saucer_position, saucer_velocity, ship_position, fleets):
        self._timer.tick(delta_time, self.fire_missile, missile_tally, saucer_position, saucer_velocity, ship_position, fleets)

    def fire_missile(self, missile_tally, saucer_position, saucer_velocity, ship_position, fleets):
        if missile_tally >= u.SAUCER_MISSILE_LIMIT:
            return
        should_target = random.random()
        random_angle = random.random()
        self.create_missile(should_target, random_angle, saucer_position, saucer_velocity, ship_position, fleets)

    def create_missile(self, should_target, random_angle, saucer_position, saucer_velocity, ship_position, fleets):
        if ship_position and should_target <= u.SAUCER_TARGETING_FRACTION:
            self.create_targeted_missile(saucer_position, ship_position, fleets)
        else:
            self.create_random_missile(random_angle, saucer_position, saucer_velocity, fleets)

One thing not to like about this is that ship_position can be None, and that does not become obvious until two calls down, in create_missile.

We could refactor a bit but it’ll break some tests. I think it’ll be worth it, though.

First some renaming:

    def fire(self, delta_time, missile_tally, saucer_position, saucer_velocity, ship_position_or_none, fleets):
        self._timer.tick(delta_time, self.fire_missile, missile_tally, saucer_position, saucer_velocity, ship_position_or_none, fleets)

    def fire_missile(self, missile_tally, saucer_position, saucer_velocity, ship_position_or_none, fleets):
        if missile_tally >= u.SAUCER_MISSILE_LIMIT:
            return
        should_target = random.random()
        random_angle = random.random()
        self.create_missile(should_target, random_angle, saucer_position, saucer_velocity, ship_position_or_none, fleets)

    def create_missile(self, should_target, random_angle, saucer_position, saucer_velocity, ship_position_or_none, fleets):
        if ship_position_or_none and should_target <= u.SAUCER_TARGETING_FRACTION:
            self.create_targeted_missile(saucer_position, ship_position_or_none, fleets)
        else:
            self.create_random_missile(random_angle, saucer_position, saucer_velocity, fleets)

PyCharm is whining because my lines are too long. I could say pos and vel, I suppose. We’ll let that ride for now.

My nice feature of having a place to inject the random numbers is getting in the way now. I’ll just do the right thing and sort the tests.

    def fire_missile(self, missile_tally, saucer_position, saucer_velocity, ship_position_or_none, fleets):
        if missile_tally >= u.SAUCER_MISSILE_LIMIT:
            return
        should_target = random.random()
        if ship_position_or_none and should_target <= u.SAUCER_TARGETING_FRACTION:
            self.create_targeted_missile(saucer_position, ship_position_or_none, fleets)
        else:
            random_angle = random.random()
            self.create_random_missile(random_angle, saucer_position, saucer_velocity, fleets)

As soon as I remove the create_missile method, tests will break.

Change this test thus:

    def test_random_missile(self):
        no_target = 0.5
        angle = 0.0
        fleets = Fleets()
        fi = FI(fleets)
        position = Vector2(500, 500)
        Gunner().create_random_missile(angle, position, Vector2(12, 34), fleets)
        assert fi.saucer_missiles
        missile = fi.saucer_missiles[0]
        assert missile.position == position + Vector2(40, 0)
        assert missile.velocity_testing_only == Vector2(u.MISSILE_SPEED + 12, 34)

That goes green. What we do not have now is a test of the branching logic in fire_missile. Another test needs work:

    def test_handle_ship_position_none(self):
        do_target = 0.1
        angle = 0.0
        fleets = Fleets()
        fi = FI(fleets)
        saucer_position = Vector2(500, 500)
        velocity = Vector2(0, 0)
        ship_position = Vector2(0, 0)
        count = u.SAUCER_MISSILE_LIMIT
        Gunner().fire_missile(0, saucer_position, velocity, None, fleets)
        assert fi.saucer_missiles

That one goes through the branching logic, checking the None path. Is there a way to check the dice roll?

We extract a method:

    def fire_missile(self, missile_tally, saucer_position, saucer_velocity, ship_position_or_none, fleets):
        if missile_tally >= u.SAUCER_MISSILE_LIMIT:
            return
        should_target = random.random()
        self.select_missile(fleets, saucer_position, saucer_velocity, ship_position_or_none, should_target)

    def select_missile(self, fleets, saucer_position, saucer_velocity, ship_position_or_none, should_target):
        if ship_position_or_none and should_target <= u.SAUCER_TARGETING_FRACTION:
            self.create_targeted_missile(saucer_position, ship_position_or_none, fleets)
        else:
            random_angle = random.random()
            self.create_random_missile(random_angle, saucer_position, saucer_velocity, fleets)

Now we can write a test against select_missile, providing a non-null ship position and a low or high roll to test the selection’s if. Forgive me, but I’m going to defer that, but I will make a note for it. The afternoon, and the article, has droned on too long.

It needs doing, mind you, and if I had a pair, I’d let them drive, and we’d get the new tests. But we have a lot of exercising going on already, and the code is clear, so I am only a little bit ashamed. Commit: refactor Gunner for better handling of None in ship_position. Needs tests.

We’ll continue our review next time, probably tomorrow morning.

Summary

Gunner is a bit better, and the changes to tests have been simple as we remove all the extra code from Saucer. We’ll review that class as well, but we removed at least as much code as we put into Gunner, and Gunner is a bit more clear, while Saucer is more cohesive than it was.

Good progress. See you next time!