Python Asteroids on GitHub

OK, lily-gilding aside, what are we really doing to do with nearest?

Presently, this is nearest:

    @staticmethod
    def nearest(shooter_coord, target_coord, screen_size):
        half = screen_size / 2
        direct_distance = abs(target_coord - shooter_coord)
        if direct_distance <= half:
            return target_coord
        elif shooter_coord >= half:
            return target_coord + screen_size
        else:
            return target_coord - screen_size

I spent most of Saturday playing with various refactorings of this code, even creating a tiny object to do the job. That was fun, and good coding practice. But what, if anything, should this code really be?

Here’s what I really think:

    @staticmethod
    def nearest(shooter_coord, target_coord, screen_size):
        direct_distance = abs(target_coord - shooter_coord)
        if direct_distance <= screen_size / 2:
            return target_coord
        elif shooter_coord > target_coord:
            return target_coord + screen_size
        else:
            return target_coord - screen_size

I do break out direct_distance because it adds a bit of meaning, and because the first if would be long and hard to read without it. I removed half because I only need it once. And I observe that if the direct distance isn’t half the screen or less, then if we are to the right of the target we shoot to the right, otherwise to the left.

Tests are green. The Saucer still shoots me down more than I deserve. Commit: improve nearest.

What about Targeter and its tests? They’re in the repo for historical reasons, no reason to keep them in the build. I’ll remove both files. Commit: remove targeter experiment and tests.

Let’s look at my sticky Jira notes. Regarding Gunner things, I find:

  • Didn’t check ship pos
  • tgt missile tally
  • test select missile
  • gunner magic number radius
  • short circuit ship pos None
  • change early test to use create_ with proper params

The first one refers to this code:

class Gunner:
    def create_targeted_missile(self, from_position, to_position, fleets):
        best_aiming_point = self.best_aiming_point(from_position, to_position, u.SCREEN_SIZE)
        angle = self.angle_to_hit(best_aiming_point, from_position)
        missile = self.missile_at_angle(from_position, angle, Vector2(0, 0))
        fleets.add_flyer(missile)

The rules of Gunner are that we are passed the saucer position and ship position, and if there is no ship, we pass None. That’s checked higher up in the code:

    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)

So we “know” that we’ll never call create_targeted_missile with to_position equal to None. For very large values of “know”, at least right now.

I don’t like this. Let’s look all the way back at where we fire the Gunner in Saucer.

    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)

This is weird. We’re using a thing that should be a position (Vector2) as a flag. And we have to create the parameter manually.

What would be some better possibilities?

  1. Pass Ship instead of its position, deal with it inside. It would still possibly be None but a more legitimate one.
  2. Generate a random position in Saucer if there is no ship.
  3. Generate a random position in Gunner if the parameter is None.
  4. #1 plus #3.

Right, #4 it is. This may change quite a bit, though. Tests abound for this thing. We’ll see: can’t be too hard.

I think this time it will be easier to change the code, let the tests break, and then let that guide us to fix them up.

    def fire(self, delta_time, missile_tally, saucer_position, saucer_velocity, ship_or_none, fleets):
        if ship_or_none:
            ship_position = ship_or_none.position
        else:
            ship_position = self.random_position()
        self._timer.tick(delta_time, self.fire_missile, missile_tally, saucer_position, saucer_velocity, ship_position, fleets)

    def random_position(self):
        return Vector2(self.random_coordinate(), self.random_coordinate())

    @staticmethod
    def random_coordinate():
        return random.randrange(0, u.SCREEN_SIZE)

If we get a ship, we use its position. If we don’t, we roll up a random position. Thereafter, the code is safe and does not care. We can change it further down:

    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()
        self.select_missile(fleets, saucer_position, saucer_velocity, ship_position, should_target)

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

We have two tests failing:

    def test_no_fire_on_short_time(self):
        delta_time = 0.1
        saucer_position = Vector2(0, 0 )
        ship_position = Vector2(1, 1)
        fleets = Fleets()
        Gunner().fire(delta_time, 0, saucer_position, Vector2(0, 0), ship_position, fleets)
        assert not FI(fleets).saucer_missiles

    def test_fire_on_time(self):
        delta_time = u.SAUCER_MISSILE_DELAY
        saucer_position = Vector2(0, 0 )
        ship_position = Vector2(1, 1)
        fleets = Fleets()
        Gunner().fire(delta_time, 0, saucer_position, Vector2(0, 0), ship_position, fleets)

These need to pass in a Ship. Easily done.

    def test_no_fire_on_short_time(self):
        delta_time = 0.1
        saucer_position = Vector2(0, 0 )
        ship_position = Vector2(1, 1)
        ship = Ship(ship_position)
        fleets = Fleets()
        Gunner().fire(delta_time, 0, saucer_position, Vector2(0, 0), ship, fleets)
        assert not FI(fleets).saucer_missiles

    def test_fire_on_time(self):
        delta_time = u.SAUCER_MISSILE_DELAY
        saucer_position = Vector2(0, 0 )
        ship_position = Vector2(1, 1)
        ship = Ship(ship_position)
        fleets = Fleets()
        Gunner().fire(delta_time, 0, saucer_position, Vector2(0, 0), ship, fleets)
        assert FI(fleets).saucer_missiles

We could in-line those positions, but this is fine.

I want some help from PyCharm, so let’s do this:

    def fire(
    	self, 
    	delta_time, 
    	missile_tally, 
    	saucer_position, 
    	saucer_velocity, 
    	ship_or_none: Ship | None, 
    	fleets):

I’ve added a type hint for ship_or_none, hoping that PyCharm will show me where I’ve got it wrong. I can also just look for senders of the method.

class Saucer(Flyer):
    def fire_if_possible(self, delta_time, fleets):
        self._gunner.fire(delta_time, self._missile_tally, self.position, self._velocity, self._ship, fleets)

That’s all there is. I’ve fixed the two tests that call here, and the other tests call the inner methods, so they need no changes … except that a review finds these two tests:

    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

    def test_no_ship_overrides_targeting(self):
        pass

The first one is passing a None to fire_missile. This is no longer allowed.

That test runs by accident, because of this:

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

This code is still checking for None, though it is now guaranteed not to be, because we roll the random number above. This test needs a change.

    def test_handle_ship_none(self):
        delta_time = 1.0
        tally = 0
        fleets = Fleets()
        fi = FI(fleets)
        saucer_position = Vector2(500, 500)
        saucer_velocity = Vector2(0, 0)
        ship = None
        Gunner().fire(delta_time, tally, saucer_position, saucer_velocity, ship, fleets)
        assert fi.saucer_missiles

This runs. The other test was a placeholder and the one above handles the case, so I remove:

    def test_no_ship_overrides_targeting(self):
        pass

We are green. Game works. Commit: Change Gunner to accept Ship as parameter, possibly None rather than position.

Nooo … I’ve broken the tests and then committed.

Ah. I typed a “r” into the test file trying to start the game. Easily fixed.

I thought that PyCharm had the ability to require green tests before committing, but if it does have that feature, I haven’t found it. I’ll just have to be more careful. Like that’ll even work.

Where are we?

Reflection

We’ve improved nearest by simplifying it. I am not convinced at all that the code is obvious, but it is tested and makes sense, although it requires a bit of thinking to understand it. I feel that I’ve spent more than enough time on it, so I’ll let it rest at least until the next time I ever look at it, which is probably never.

Then I felt a bit of concern about passing Gunner either a position or a None, and changed it to pass a Ship or None. There’s a difference in the caller, in that the Saucer has a variable, _ship, which is either a Ship or None, so it doesn’t have to do anything special to use its Gunner. And the Gunner just rolls up a random location if it is passed a null Ship, and uses that value instead of the ship’s position. So it’s all good.

This allows me to strike out and remove a sticky:

  • Didn't check ship pos

Rather than just check it, we made it not require checking. A noticeable improvement.

I think this one is no longer relevant, because a review of the tests satisfies me.

  • change early test to use create_ with proper params

These one have test so we are OK cancelling them:

  • tgt missile tally
  • test select missile

This one no longer applies:

  • short circuit ship pos None

We’re left with this:

  • Didn't check ship pos
  • tgt missile tally
  • test select missile
  • gunner magic number radius
  • short circuit ship pos None
  • change early test to use create_ with proper params

The remaining ticket refers to this:

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

To fix this, what we really need is to pass in the Saucer so that we can fetch its radius. The Saucer, when complete, comes in two sizes, large and small. We’ll let this sticky note ride.

Summary

We’ve managed to clear quite a few tickets this morning. I call it a couple of hours well spent. The Gunner is better, nearest is as lean as I can make it, albeit not as clear as one could possibly make it. For now, we’ll leave it be:

    @staticmethod
    def nearest(shooter_coord, target_coord, screen_size):
        direct_distance = abs(target_coord - shooter_coord)
        if direct_distance <= screen_size / 2:
            return target_coord
        elif shooter_coord > target_coord:
            return target_coord + screen_size
        else:
            return target_coord - screen_size

Just for fun, I added this handy diagram:

    def nearest(shooter_coord, target_coord, screen_size):
        """     Handy Diagram       """
        """  ______|______|______   """
        """   T      T---S++T       """
        """ Central T too far away. """
        """ We are to his right so  """
        """ we shoot toward right!  """
        direct_distance = abs(target_coord - shooter_coord)
        if direct_distance <= screen_size / 2:
            return target_coord
        elif shooter_coord > target_coord:
            return target_coord + screen_size
        else:
            return target_coord - screen_size

Anyway, enough for this morning. See you next time!