Python 159 - Messing About

I’m just going to look at random bits of code and see what they suggest to me. Watch me if you care to.

In ShotOptimizer, I find this:

class ShotOptimizer:
    def __init__(self, saucer, ship):
        shooter_position = saucer.position
        best_target_position = self.closest_aiming_point(shooter_position, ship.position, u.SCREEN_SIZE)
        vector_to_target = best_target_position - shooter_position
        safe_distance = saucer.missile_head_start
        aim_time, speed_adjustment = self.optimal_shot(vector_to_target, ship.velocity, safe_distance)
        target_position = best_target_position + ship.velocity * aim_time

        solution = FiringSolution(target_position, shooter_position, safe_distance, speed_adjustment)

        self.velocity = solution.velocity
        self.start = solution.start

This makes me think at least two things:

First, this method is too complicated. Second, I think we should be using the solution, not the velocity and start members in our actual code. Here’s what I mean:

class Gunner:
    @staticmethod
    def create_optimal_missile(fleets, saucer, ship):
        target_solution = ShotOptimizer(saucer, ship)
        fleets.append(Missile.from_saucer(target_solution.start, target_solution.velocity))

    def create_unoptimized_missile(self, shooter_position, target_position, fleets):
        safe_distance = self._missile_head_start
        speed_adjustment = 1
        solution = FiringSolution(target_position, shooter_position, safe_distance, speed_adjustment)
        missile = Missile.from_saucer(solution.start, solution.velocity)
        fleets.append(missile)

The unoptimized case just creates a FiringSolution and uses it. Since FiringSolution is a simpler object, I think we should use it above, in the optimal case. Like this:

class Gunner:
    @staticmethod
    def create_optimal_missile(fleets, saucer, ship):
        solution = ShotOptimizer(saucer, ship).firing_solution
        fleets.append(Missile.from_saucer(solution.start, solution.velocity))

    def create_unoptimized_missile(self, shooter_position, target_position, fleets):
        safe_distance = self._missile_head_start
        speed_adjustment = 1
        solution = FiringSolution(target_position, shooter_position, safe_distance, speed_adjustment)
        missile = Missile.from_saucer(solution.start, solution.velocity)
        fleets.append(missile)

class ShotOptimizer:
    def __init__(self, saucer, ship):
        shooter_position = saucer.position
        best_target_position = self.closest_aiming_point(shooter_position, ship.position, u.SCREEN_SIZE)
        vector_to_target = best_target_position - shooter_position
        safe_distance = saucer.missile_head_start
        aim_time, speed_adjustment = self.optimal_shot(vector_to_target, ship.velocity, safe_distance)
        target_position = best_target_position + ship.velocity * aim_time
        self.firing_solution = FiringSolution(target_position, shooter_position, safe_distance, speed_adjustment)

We can commit that. Commit: refactor to use FiringSolution for both aimed and random missiles.

Now I think we can improve the Gunner a bit. First, extract variable to make the code more visibly alike:

    @staticmethod
    def create_optimal_missile(fleets, saucer, ship):
        solution = ShotOptimizer(saucer, ship).firing_solution
        missile = Missile.from_saucer(solution.start, solution.velocity)
        fleets.append(missile)

    def create_unoptimized_missile(self, shooter_position, target_position, fleets):
        safe_distance = self._missile_head_start
        speed_adjustment = 1
        solution = FiringSolution(target_position, shooter_position, safe_distance, speed_adjustment)
        missile = Missile.from_saucer(solution.start, solution.velocity)
        fleets.append(missile)

Extract method to eliminate duplication.

    @staticmethod
    def create_optimal_missile(fleets, saucer, ship):
        solution = ShotOptimizer(saucer, ship).firing_solution
        Gunner.fire_using_solution(solution, fleets)

    def create_unoptimized_missile(self, shooter_position, target_position, fleets):
        safe_distance = self._missile_head_start
        speed_adjustment = 1
        solution = FiringSolution(target_position, shooter_position, safe_distance, speed_adjustment)
        Gunner.fire_using_solution(solution, fleets)

    @staticmethod
    def fire_using_solution(solution, fleets):
        missile = Missile.from_saucer(solution.start, solution.velocity)
        fleets.append(missile)

The fact that that is a static method makes us wonder where it would better reside.

What about FiringSolution? What if FiringSolution would return the missile, all nicely programmed?

class FiringSolution:
    def __init__(self, target_position, shooter_position, safe_distance, speed_adjustment):
        direction_to_target = (target_position - shooter_position).normalize()
        safety_offset = direction_to_target * safe_distance
        self.velocity = direction_to_target * u.MISSILE_SPEED * speed_adjustment
        self.start = shooter_position + safety_offset

    def saucer_missile(self):
        return Missile.from_saucer(self.start, self.velocity)

I should have committed a time or two already. Too late now, but we’re nearly to another good place.

class Gunner:
    @staticmethod
    def fire_using_solution(solution, fleets):
        missile = solution.saucer_missile()
        fleets.append(missile)

Inline the temp.

class Gunner:
    @staticmethod
    def create_optimal_missile(fleets, saucer, ship):
        solution = ShotOptimizer(saucer, ship).firing_solution
        Gunner.fire_using_solution(solution, fleets)

    def create_unoptimized_missile(self, shooter_position, target_position, fleets):
        safe_distance = self._missile_head_start
        speed_adjustment = 1
        solution = FiringSolution(target_position, shooter_position, safe_distance, speed_adjustment)
        Gunner.fire_using_solution(solution, fleets)

    @staticmethod
    def fire_using_solution(solution, fleets):
        fleets.append(solution.saucer_missile())

No, undo that. I don’t like the static thing there.

I go clear back to here:

    @staticmethod
    def create_optimal_missile(fleets, saucer, ship):
        solution = ShotOptimizer(saucer, ship).firing_solution
        missile = Missile.from_saucer(solution.start, solution.velocity)
        fleets.append(missile)

    def create_unoptimized_missile(self, shooter_position, target_position, fleets):
        safe_distance = self._missile_head_start
        speed_adjustment = 1
        solution = FiringSolution(target_position, shooter_position, safe_distance, speed_adjustment)
        missile = Missile.from_saucer(solution.start, solution.velocity)
        fleets.append(missile)

We need to make optimal not be static.

    def create_optimal_missile(self, fleets, saucer, ship):
        solution = ShotOptimizer(saucer, ship).firing_solution
        missile = Missile.from_saucer(solution.start, solution.velocity)
        fleets.append(missile)

    def create_unoptimized_missile(self, shooter_position, target_position, fleets):
        safe_distance = self._missile_head_start
        speed_adjustment = 1
        solution = FiringSolution(target_position, shooter_position, safe_distance, speed_adjustment)
        missile = Missile.from_saucer(solution.start, solution.velocity)
        fleets.append(missile)

I am not inclined to commit, because I am not sure I’m going to like this, having not liked what I did a moment ago. This time let’s get the missile from the solution.

    def create_optimal_missile(self, fleets, saucer, ship):
        solution = ShotOptimizer(saucer, ship).firing_solution
        missile = solution.saucer_missile()
        fleets.append(missile)

    def create_unoptimized_missile(self, shooter_position, target_position, fleets):
        safe_distance = self._missile_head_start
        speed_adjustment = 1
        solution = FiringSolution(target_position, shooter_position, safe_distance, speed_adjustment)
        missile = solution.saucer_missile()
        fleets.append(missile)

Now inline missile.

    def create_optimal_missile(self, fleets, saucer, ship):
        solution = ShotOptimizer(saucer, ship).firing_solution
        fleets.append(solution.saucer_missile())

    def create_unoptimized_missile(self, shooter_position, target_position, fleets):
        safe_distance = self._missile_head_start
        speed_adjustment = 1
        solution = FiringSolution(target_position, shooter_position, safe_distance, speed_adjustment)
        fleets.append(solution.saucer_missile())

I like this well enough to commit: refactoring. Maybe needed a better comment.

I don’t like the fact that create_optimal_missile could be static. It doesn’t care about anything in the Gunner. With our newly increased sensitivity to such things, we take it as a sign that maybe responsibilities aren’t quite right.

Let’s back away and look at a slightly bigger picture:

    def fire_available_missile(self, chance, fleets, saucer, ship_or_none):
        if ship_or_none and self.should_target(chance, saucer):
            self.create_optimal_missile(fleets, saucer, ship_or_none)
        else:
            self.create_random_missile(fleets, saucer)

    @staticmethod
    def should_target(chance, saucer):
        return saucer.always_target or chance < u.SAUCER_TARGETING_FRACTION

    def create_random_missile(self, fleets, saucer):
        target = self.random_position()
        self.create_unoptimized_missile(saucer.position, target, fleets)

    def create_optimal_missile(self, fleets, saucer, ship):
        solution = ShotOptimizer(saucer, ship).firing_solution
        fleets.append(solution.saucer_missile())

    def create_unoptimized_missile(self, shooter_position, target_position, fleets):
        safe_distance = self._missile_head_start
        speed_adjustment = 1
        solution = FiringSolution(target_position, shooter_position, safe_distance, speed_adjustment)
        fleets.append(solution.saucer_missile())

Now, it seems to me that part of the issue is that optimized missiles, which maybe should be called targeted as opposed to random, need info about the ship, but random missiles do not.

What if ShotOptimizer, perhaps renamed, had two methods, targeted_solution and random_solution?

Then both methods could call ShotOptimizer, which would be more balanced. Let’s see if we can do that.

Let’s first work toward a new method, targeted_solution, in property form.

class ShotOptimizer:
    def __init__(self, saucer, ship):
        self.saucer = saucer
        self.ship = ship
        self.create_targeted_solution(saucer, ship)

    def create_targeted_solution(self, saucer, ship):
        shooter_position = saucer.position
        best_target_position = self.closest_aiming_point(shooter_position, ship.position, u.SCREEN_SIZE)
        vector_to_target = best_target_position - shooter_position
        safe_distance = saucer.missile_head_start
        aim_time, speed_adjustment = self.optimal_shot(vector_to_target, ship.velocity, safe_distance)
        target_position = best_target_position + ship.velocity * aim_time
        return FiringSolution(target_position, shooter_position, safe_distance, speed_adjustment)
    @property
    def targeted_solution(self):
        return self.create_targeted_solution(self.saucer, self.ship)

This breaks many tests, but the fix should be just this:

    def create_optimal_missile(self, fleets, saucer, ship):
        solution = ShotOptimizer(saucer, ship).targeted_solution
        fleets.append(solution.saucer_missile())

We are green. Now let’s do the random one.

    @property
    def random_solution(self):
        return FiringSolution(self.random_position(), self.saucer.position, self.saucer.missile_head_start, 1)

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

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

Now I can use that in Gunner:

    def fire_available_missile(self, chance, fleets, saucer, ship_or_none):
        if ship_or_none and self.should_target(chance, saucer):
            self.create_optimal_missile(fleets, saucer, ship_or_none)
        else:
            self.create_random_missile(fleets, saucer, ship_or_none)

    def create_random_missile(self, fleets, saucer, ship):
        solution = ShotOptimizer(saucer, ship).random_solution
        fleets.append(solution.saucer_missile())

    def create_optimal_missile(self, fleets, saucer, ship):
        solution = ShotOptimizer(saucer, ship).targeted_solution
        fleets.append(solution.saucer_missile())

This breaks some tests that are using the create_unoptimized_missile method.

Lost the thread. Step was too big for some reason.

Back to here:

    def create_optimal_missile(self, fleets, saucer, ship):
        solution = ShotOptimizer(saucer, ship).targeted_solution
        fleets.append(solution.saucer_missile())

    def create_unoptimized_missile(self, shooter_position, target_position, fleets):
        safe_distance = self._missile_head_start
        speed_adjustment = 1
        solution = FiringSolution(target_position, shooter_position, safe_distance, speed_adjustment)
        fleets.append(solution.saucer_missile())

In ShotOptimizer, with a little refactoring, we’re here:

    def __init__(self, saucer, ship):
        self.saucer = saucer
        self.ship = ship

    @property
    def targeted_solution(self):
        shooter_position = self.saucer.position
        best_target_position = self.closest_aiming_point(shooter_position, self.ship.position, u.SCREEN_SIZE)
        vector_to_target = best_target_position - shooter_position
        safe_distance = self.saucer.missile_head_start
        aim_time, speed_adjustment = self.optimal_shot(vector_to_target, self.ship.velocity, safe_distance)
        target_position = best_target_position + self.ship.velocity * aim_time
        return FiringSolution(target_position, shooter_position, safe_distance, speed_adjustment)

    @property
    def random_solution(self):
        return FiringSolution(self.random_position(), self.saucer.position, self.saucer.missile_head_start, 1)

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

The random_solution is unused but I am confident in it.

We need to get it in use, but without breaking any tests. Here we go:

class Gunner:
    def fire_available_missile(self, chance, fleets, saucer, ship_or_none):
        if ship_or_none and self.should_target(chance, saucer):
            self.create_optimal_missile(fleets, saucer, ship_or_none)
        else:
            self.create_random_missile(fleets, saucer, ship_or_none)

class ShotOptimizer:
    def __init__(self, saucer, ship):
        self.saucer = saucer
        self.ship = ship

    @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)
        vector_to_target = best_target_position - shooter_position
        safe_distance = self.saucer.missile_head_start
        aim_time, speed_adjustment = self.optimal_shot(vector_to_target, self.ship.velocity, safe_distance)
        target_position = best_target_position + self.ship.velocity * aim_time
        return FiringSolution(target_position, shooter_position, safe_distance, speed_adjustment)

    @property
    def random_solution(self):
        return FiringSolution(self.random_position(), self.saucer.position, self.saucer.missile_head_start, 1)

This is passing all the tests and working in the game. The check for ship is due to the fact that we have two paths to random missile, and in one of them there is no ship.

We could, and probably should avoid that None, providing a dummy ship or some such thing.

I suspect we’re not quite where things are in balance yet.

There are methods in Gunner that can be removed, and I think some have tests.

I have two tests on the old unoptimized one.

Curiously, I find that the tests are rather suspicious but I fix them up. We’re green and I am of a mind to commit: Refactored to Gunner -> ShotOptimizer -> FiringSolution -> Missile.

Let’s review some of the good bits. Now Gunner is down to this:

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

    def fire(self, delta_time, saucer, ship_or_none: Ship | None, fleets):
        self._timer.tick(delta_time, self.fire_if_missile_available, saucer, ship_or_none, fleets)

    def fire_if_missile_available(self, saucer, ship_or_none, fleets):
        if did_we_fire := saucer.missile_tally < u.SAUCER_MISSILE_LIMIT:
            chance = random.random()
            self.fire_available_missile(chance, fleets, saucer, ship_or_none)
        return did_we_fire

    def fire_available_missile(self, chance, fleets, saucer, ship_or_none):
        if ship_or_none and self.should_target(chance, saucer):
            self.create_optimal_missile(fleets, saucer, ship_or_none)
        else:
            self.create_random_missile(fleets, saucer, ship_or_none)

    @staticmethod
    def should_target(chance, saucer):
        return saucer.always_target or chance < u.SAUCER_TARGETING_FRACTION

    @staticmethod
    def create_random_missile(fleets, saucer, ship_or_none):
        solution = ShotOptimizer(saucer, ship_or_none).random_solution
        fleets.append(solution.saucer_missile())

    @staticmethod
    def create_optimal_missile(fleets, saucer, ship):
        solution = ShotOptimizer(saucer, ship).targeted_solution
        fleets.append(solution.saucer_missile())

This is much simpler than it was. I think it can be better. Let’s move the random number down in:

        @staticmethod
        def should_target(chance, saucer):
            return saucer.always_target or random.random() < u.SAUCER_TARGETING_FRACTION

Now we can change signature and remove the other roll:

    def fire_if_missile_available(self, saucer, ship_or_none, fleets):
        if did_we_fire := saucer.missile_tally < u.SAUCER_MISSILE_LIMIT:
            self.fire_available_missile(fleets, saucer, ship_or_none)
        return did_we_fire

    def fire_available_missile(self, fleets, saucer, ship_or_none):
        if ship_or_none and self.should_target(saucer):
            self.create_optimal_missile(fleets, saucer, ship_or_none)
        else:
            self.create_random_missile(fleets, saucer, ship_or_none)

    @staticmethod
    def should_target(saucer):
        return saucer.always_target or random.random() < u.SAUCER_TARGETING_FRACTION

Commit that: push random number call down.

Let’s inline the two create methods. We either have to make them non-static, or do it by hand. By hand is OK this time, it’s copy-paste.

    def fire_available_missile(self, fleets, saucer, ship_or_none):
        if ship_or_none and self.should_target(saucer):
            solution = ShotOptimizer(saucer, ship_or_none).targeted_solution
            fleets.append(solution.saucer_missile())
        else:
            solution = ShotOptimizer(saucer, ship_or_none).random_solution
            fleets.append(solution.saucer_missile())

And pull out the duplication:

    def fire_available_missile(self, fleets, saucer, ship_or_none):
        if ship_or_none and self.should_target(saucer):
            solution = ShotOptimizer(saucer, ship_or_none).targeted_solution
        else:
            solution = ShotOptimizer(saucer, ship_or_none).random_solution
        fleets.append(solution.saucer_missile())

Unfortunately removing those methods broke a couple of tests. They are easily set right and we can commit: refactor And here’s Gunner now:

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

    def fire(self, delta_time, saucer, ship_or_none: Ship | None, fleets):
        self._timer.tick(delta_time, self.fire_if_missile_available, saucer, ship_or_none, fleets)

    def fire_if_missile_available(self, saucer, ship_or_none, fleets):
        if did_we_fire := saucer.missile_tally < u.SAUCER_MISSILE_LIMIT:
            self.fire_available_missile(fleets, saucer, ship_or_none)
        return did_we_fire

    def fire_available_missile(self, fleets, saucer, ship_or_none):
        if ship_or_none and self.should_target(saucer):
            solution = ShotOptimizer(saucer, ship_or_none).targeted_solution
        else:
            solution = ShotOptimizer(saucer, ship_or_none).random_solution
        fleets.append(solution.saucer_missile())

    @staticmethod
    def should_target(saucer):
        return saucer.always_target or random.random() < u.SAUCER_TARGETING_FRACTION

Much shorter and still making perfect sense. I think we’ll break here, review the other classes next time.

Summary

Of course we are gilding the asteroid with all these improvements, but what we’re doing is discovering how to make complex code less complex, confusing code more clear. Our changes this afternoon were simple enough, but were not all done with machine refactorings. Such is the case when we move capability between classes: PyCharm offers little or no support for that. Maybe next year.

A few tests broke, but only because either we had moved a capability to another class, or because we had changed a calling sequence, so the changes were straightforward enough that I didn’t even show them.

The flow of missile creation is now similar in both cases, random or targeted. Which reminds me, we nicely got rid of the awkward naming create_optimal_missile vs create_unoptimized_missile or whatever those methods were. Now we have a targeted_solution and random_solution, which seems to me to make more sense.

The Gunner just decides that it can fire, and chooses random or targeted. It asks the ShotOptimizer to return whichever kind of missile it wants. ShotOptimizer always returns a FiringSolution, whether it runs the complex quadratic code or not. And FiringSolution creates a missile without regard to details, and returns it.

This is the kind of object-oriented solution that I like. Ask an object for something, it asks another, which asks another, and after a while the answer returns.

I think this is nearly good, but we’ll review the classes used by Gunner and see how much better we can make them … in future articles.

See you then!