Python 152 - Refactoring

I begin a long refactoring session. I’ll split the article another time or two. I am a merciful person at heart.

Second in Series

Here, without committing the solution from the preceding article (I should have, but I didn’t) I begin refactoring the solution to avoid the temporal coupling and to generally improve the implementation. It’s long, smooth, and all goes well.

Lots of nice small steps in here. Browse a bit, you might find it interesting.

Moving Along

So let’s make more of a mess. My tentative plan, so far, is to keep inlining methods until I get everything I need in one place, and then to rearrange things in a more felicitous, nicer, and generally better fashion.

This time, with no real plan, I’m think I’m going to do that inlining fairly blindly, expecting that a plan will emerge. At that point, we might roll back and do it over, roll back and try something new, or continue, improving the code until we actually like it. That last case would be my favorite, but I grant that it’s not the mostly likely. More likely is that we’ll see some smaller changes, and do it again. I hope the least likely is that I’ll throw up my hands and scream “Nooooo!””

But … maybe we can do better than blind inlining. I think I see a tempting opportunity.

One thing that strikes me as odd about all this is that we call create_targeted_missile in two separate states. In the one case, we have actually chosen a random target and have set the adjustment ratio to one, using an un-adjusted velocity. In the other, we have a real target and want the adjusted value. But the code wanders down to the same place.

That starts happening here:

    def fire_available_missile(self, chance, fleets, saucer, ship_or_none):
        self._adjustment_ratio = 1
        ship_position = self.select_aiming_point(chance, saucer, ship_or_none)
        self.create_targeted_missile(saucer.position, ship_position, fleets)

    def select_aiming_point(self, chance, saucer, ship_or_none):
        if not ship_or_none:
            return self.random_position()
        elif saucer.always_target:
            return self.optimal_aiming_point(saucer, ship_or_none)
        elif chance < u.SAUCER_TARGETING_FRACTION:
            return self.optimal_aiming_point(saucer, ship_or_none)
        else:
            return self.random_position()

Could we have two methods, perhaps create_optimized_missile and create_unoptimized_missile, and use whichever one we mean?

Note: Nice refactoring sequence starting here.

Let’s inline the thing above and see if we can get there.

    def fire_available_missile(self, chance, fleets, saucer, ship_or_none):
        self._adjustment_ratio = 1
        if not ship_or_none:
            result = self.random_position()
        elif saucer.always_target:
            result = self.optimal_aiming_point(saucer, ship_or_none)
        elif chance < u.SAUCER_TARGETING_FRACTION:
            result = self.optimal_aiming_point(saucer, ship_or_none)
        else:
            result = self.random_position()
        ship_position = result
        self.create_targeted_missile(saucer.position, ship_position, fleets)

Duplicate the create call up into each branch. No first, inline ship_position in the final call.

    def fire_available_missile(self, chance, fleets, saucer, ship_or_none):
        self._adjustment_ratio = 1
        if not ship_or_none:
            result = self.random_position()
        elif saucer.always_target:
            result = self.optimal_aiming_point(saucer, ship_or_none)
        elif chance < u.SAUCER_TARGETING_FRACTION:
            result = self.optimal_aiming_point(saucer, ship_or_none)
        else:
            result = self.random_position()
        self.create_targeted_missile(saucer.position, result, fleets)

Now duplicate the bottom line and move up and into the ifs.

    def fire_available_missile(self, chance, fleets, saucer, ship_or_none):
        self._adjustment_ratio = 1
        if not ship_or_none:
            result = self.random_position()
            self.create_targeted_missile(saucer.position, result, fleets)
        elif saucer.always_target:
            result = self.optimal_aiming_point(saucer, ship_or_none)
            self.create_targeted_missile(saucer.position, result, fleets)
        elif chance < u.SAUCER_TARGETING_FRACTION:
            result = self.optimal_aiming_point(saucer, ship_or_none)
            self.create_targeted_missile(saucer.position, result, fleets)
        else:
            result = self.random_position()
            self.create_targeted_missile(saucer.position, result, fleets)

Let’s rename those result guys to target in the interest of clarity.

    def fire_available_missile(self, chance, fleets, saucer, ship_or_none):
        self._adjustment_ratio = 1
        if not ship_or_none:
            target = self.random_position()
            self.create_targeted_missile(saucer.position, target, fleets)
        elif saucer.always_target:
            target = self.optimal_aiming_point(saucer, ship_or_none)
            self.create_targeted_missile(saucer.position, target, fleets)
        elif chance < u.SAUCER_TARGETING_FRACTION:
            target = self.optimal_aiming_point(saucer, ship_or_none)
            self.create_targeted_missile(saucer.position, target, fleets)
        else:
            target = self.random_position()
            self.create_targeted_missile(saucer.position, target, fleets)

Yes. As an aside, I think I’d prefer that the target be first in those create calls, but we’ll let that ride for now.

Can we simplify the if nest while we’re here? Or should we just create our two methods and call them?

As I look at this code and imagine what it might do, I think I might do well to create a new method to use in the two actual targeting calls. Something like create_optimal_missile. I’m thinking that the new method would call the optimal_aiming_point method itself.

What happens if I extract those two lines to a new method?

    def fire_available_missile(self, chance, fleets, saucer, ship_or_none):
        self._adjustment_ratio = 1
        if not ship_or_none:
            target = self.random_position()
            self.create_targeted_missile(saucer.position, target, fleets)
        elif saucer.always_target:
            self.create_optimal_missile(fleets, saucer, ship_or_none)
        elif chance < u.SAUCER_TARGETING_FRACTION:
            self.create_optimal_missile(fleets, saucer, ship_or_none)
        else:
            target = self.random_position()
            self.create_targeted_missile(saucer.position, target, fleets)

    def create_optimal_missile(self, fleets, saucer, ship_or_none):
        target = self.optimal_aiming_point(saucer, ship_or_none)
        self.create_targeted_missile(saucer.position, target, fleets)

Now let’s inline the code from create_targeted_missile:

    def create_optimal_missile(self, fleets, saucer, ship_or_none):
        target = self.optimal_aiming_point(saucer, ship_or_none)
        vector_to_target = target - saucer.position
        direction_to_target = vector_to_target.normalize()
        missile_velocity = u.MISSILE_SPEED * direction_to_target
        adjusted_velocity = missile_velocity * self._adjustment_ratio
        offset = 2 * self._radius * direction_to_target
        missile = Missile.from_saucer(saucer.position + offset, adjusted_velocity)
        fleets.append(missile)

Note: The previous changes created an opportunity. I was tempted to wait to reap the benefit but decided to go ahead:

Now … I’m not going to do this, but now, create_targeted_missile can be renamed and simplified. No, in fact let’s do it now.

    def create_unoptimized_missile(self, from_position, to_position, fleets):
        vector_to_target = to_position - from_position
        direction_to_target = vector_to_target.normalize()
        missile_velocity = u.MISSILE_SPEED * direction_to_target
        offset = 2 * self._radius * direction_to_target
        missile = Missile.from_saucer(from_position + offset, missile_velocity)
        fleets.append(missile)

All our tests are green through all this, by the way. And I am confident that the game is good as well.

Now back to the optimal guy:

    def create_optimal_missile(self, fleets, saucer, ship_or_none):
        target = self.optimal_aiming_point(saucer, ship_or_none)
        vector_to_target = target - saucer.position
        direction_to_target = vector_to_target.normalize()
        missile_velocity = u.MISSILE_SPEED * direction_to_target
        adjusted_velocity = missile_velocity * self._adjustment_ratio
        offset = 2 * self._radius * direction_to_target
        missile = Missile.from_saucer(saucer.position + offset, adjusted_velocity)
        fleets.append(missile)

Note: Still doing small refactoring steps.

Let’s inline the optimal_aiming_point code.

    def create_optimal_missile(self, fleets, saucer, ship_or_none):
        target_position = self.closest_aiming_point(saucer.position, ship_or_none.position, u.SCREEN_SIZE)
        delta_position = target_position - saucer.position
        aim_time = self.time_to_target(delta_position, ship_or_none.velocity)
        # egregious hack
        self.set_adjustment_ratio(aim_time)
        target = target_position + ship_or_none.velocity * aim_time
        vector_to_target = target - saucer.position
        direction_to_target = vector_to_target.normalize()
        missile_velocity = u.MISSILE_SPEED * direction_to_target
        adjusted_velocity = missile_velocity * self._adjustment_ratio
        offset = 2 * self._radius * direction_to_target
        missile = Missile.from_saucer(saucer.position + offset, adjusted_velocity)
        fleets.append(missile)

We’re doing this with machine refactoring, so we don’t really even try to understand much … yet. We’re just trying to get a level playing field. Let’s inline the set_adjustment_ratio.

PyCharm declines to do this for me. Let me hand-refactor the little method.

Note: I could have done this by hand, but getting PyCharm to do it is safer. So I make a small change by hand and then let PyCharm go at it.

After refactoring by hand:

    def set_adjustment_ratio(self, aim_time):
        if aim_time:
            distance_to_target = aim_time*u.MISSILE_SPEED
            adjusted_distance = distance_to_target - 2*self._radius
            self._adjustment_ratio = adjusted_distance / distance_to_target

Now will PyCharm inline it for me? It will and we get this:

    def create_optimal_missile(self, fleets, saucer, ship_or_none):
        target_position = self.closest_aiming_point(saucer.position, ship_or_none.position, u.SCREEN_SIZE)
        delta_position = target_position - saucer.position
        aim_time = self.time_to_target(delta_position, ship_or_none.velocity)
        # egregious hack
        if aim_time:
            distance_to_target = aim_time * u.MISSILE_SPEED
            adjusted_distance = distance_to_target - 2 * self._radius
            self._adjustment_ratio = adjusted_distance / distance_to_target
        target = target_position + ship_or_none.velocity * aim_time
        vector_to_target = target - saucer.position
        direction_to_target = vector_to_target.normalize()
        missile_velocity = u.MISSILE_SPEED * direction_to_target
        adjusted_velocity = missile_velocity * self._adjustment_ratio
        offset = 2 * self._radius * direction_to_target
        missile = Missile.from_saucer(saucer.position + offset, adjusted_velocity)
        fleets.append(missile)

That is quite nasty. It is also green and I think it’s good.

Now some judicious non-machine editing on that if.

    def create_optimal_missile(self, fleets, saucer, ship_or_none):
        target_position = self.closest_aiming_point(saucer.position, ship_or_none.position, u.SCREEN_SIZE)
        delta_position = target_position - saucer.position
        aim_time = self.time_to_target(delta_position, ship_or_none.velocity)
        if aim_time:
            distance_to_target = aim_time * u.MISSILE_SPEED
            adjusted_distance = distance_to_target - 2 * self._radius
            adjustment_ratio = adjusted_distance / distance_to_target
        else:
            adjustment_ratio = 1
        target = target_position + ship_or_none.velocity * aim_time
        vector_to_target = target - saucer.position
        direction_to_target = vector_to_target.normalize()
        missile_velocity = u.MISSILE_SPEED * direction_to_target
        adjusted_velocity = missile_velocity * adjustment_ratio
        offset = 2 * self._radius * direction_to_target
        missile = Missile.from_saucer(saucer.position + offset, adjusted_velocity)
        fleets.append(missile)

I have now removed all uses of the spooky _adjustment_ratio member variable!! Remove any vestiges. Two setters setting it to 1, no uses in live code.

Note: I could have been, and probably should have been, committing these changes. I didn’t because I was making the code generally worse and was expecting that it would get better. In general, I would not like to leave the GitHub code “worse”. And, after all, this sequence might not have worked. I was confident but not deranged.

Now I want to compare my two creation methods.

    def create_optimal_missile(self, fleets, saucer, ship_or_none):
        target_position = self.closest_aiming_point(saucer.position, ship_or_none.position, u.SCREEN_SIZE)
        delta_position = target_position - saucer.position
        aim_time = self.time_to_target(delta_position, ship_or_none.velocity)
        if aim_time:
            distance_to_target = aim_time * u.MISSILE_SPEED
            adjusted_distance = distance_to_target - 2 * self._radius
            adjustment_ratio = adjusted_distance / distance_to_target
        else:
            adjustment_ratio = 1
        target = target_position + ship_or_none.velocity * aim_time

        vector_to_target = target - saucer.position
        direction_to_target = vector_to_target.normalize()
        missile_velocity = u.MISSILE_SPEED * direction_to_target
        adjusted_velocity = missile_velocity * adjustment_ratio
        offset = 2 * self._radius * direction_to_target
        missile = Missile.from_saucer(saucer.position + offset, adjusted_velocity)
        fleets.append(missile)

    def create_unoptimized_missile(self, from_position, to_position, fleets):
        vector_to_target = to_position - from_position
        direction_to_target = vector_to_target.normalize()
        missile_velocity = u.MISSILE_SPEED * direction_to_target
        offset = 2 * self._radius * direction_to_target
        missile = Missile.from_saucer(from_position + offset, missile_velocity)
        fleets.append(missile)

I added a blank line there to show the almost common code. If we were to add a parameter to create_unoptimized_missile, the adjustment factor, we could call it from our create optimal code. That would simplify create_optimal_missile a bit.

Note: My first thought was to rewrite the unoptimized method to take the parameter. It came to me that I could extract the parameterized one from existing code and then see whether I could better combine the two mostly-duplicated methods. That worked out perfectly.; First time through, I didn’t make quite the right extraction.

Let’s do this: we’ll extract the more powerful case from create_optimal_missile, then use it from the other.

    def create_optimal_missile(self, fleets, saucer, ship_or_none):
        target_position = self.closest_aiming_point(saucer.position, ship_or_none.position, u.SCREEN_SIZE)
        delta_position = target_position - saucer.position
        aim_time = self.time_to_target(delta_position, ship_or_none.velocity)
        if aim_time:
            distance_to_target = aim_time * u.MISSILE_SPEED
            adjusted_distance = distance_to_target - 2 * self._radius
            adjustment_ratio = adjusted_distance / distance_to_target
        else:
            adjustment_ratio = 1
        target = target_position + ship_or_none.velocity * aim_time
        self.create_adjusted_missile(adjustment_ratio, target, saucer, fleets)

    def create_adjusted_missile(self, adjustment_ratio, target, saucer, fleets):
        vector_to_target = target - saucer.position
        direction_to_target = vector_to_target.normalize()
        missile_velocity = u.MISSILE_SPEED * direction_to_target
        adjusted_velocity = missile_velocity * adjustment_ratio
        offset = 2 * self._radius * direction_to_target
        missile = Missile.from_saucer(saucer.position + offset, adjusted_velocity)
        fleets.append(missile)

No. That won’t quite do. Our unoptimized method doesn’t have the saucer. Undo the extract and do again. We’re here:

    def create_optimal_missile(self, fleets, saucer, ship_or_none):
        target_position = self.closest_aiming_point(saucer.position, ship_or_none.position, u.SCREEN_SIZE)
        delta_position = target_position - saucer.position
        aim_time = self.time_to_target(delta_position, ship_or_none.velocity)
        if aim_time:
            distance_to_target = aim_time * u.MISSILE_SPEED
            adjusted_distance = distance_to_target - 2 * self._radius
            adjustment_ratio = adjusted_distance / distance_to_target
        else:
            adjustment_ratio = 1
        target = target_position + ship_or_none.velocity * aim_time

        vector_to_target = target - saucer.position
        direction_to_target = vector_to_target.normalize()
        missile_velocity = u.MISSILE_SPEED * direction_to_target
        adjusted_velocity = missile_velocity * adjustment_ratio
        offset = 2 * self._radius * direction_to_target
        missile = Missile.from_saucer(saucer.position + offset, adjusted_velocity)
        fleets.append(missile)

Note: Having done the extract, we take the opportunity to improve things a bit.

Extract variable:

    def create_optimal_missile(self, fleets, saucer, ship_or_none):
        target_position = self.closest_aiming_point(saucer.position, ship_or_none.position, u.SCREEN_SIZE)
        delta_position = target_position - saucer.position
        aim_time = self.time_to_target(delta_position, ship_or_none.velocity)
        if aim_time:
            distance_to_target = aim_time * u.MISSILE_SPEED
            adjusted_distance = distance_to_target - 2 * self._radius
            adjustment_ratio = adjusted_distance / distance_to_target
        else:
            adjustment_ratio = 1
        target = target_position + ship_or_none.velocity * aim_time

        saucer_position = saucer.position
        vector_to_target = target - saucer_position
        direction_to_target = vector_to_target.normalize()
        missile_velocity = u.MISSILE_SPEED * direction_to_target
        adjusted_velocity = missile_velocity * adjustment_ratio
        offset = 2 * self._radius * direction_to_target
        missile = Missile.from_saucer(saucer_position + offset, adjusted_velocity)
        fleets.append(missile)

Extract method again.

    def create_optimal_missile(self, fleets, saucer, ship_or_none):
        target_position = self.closest_aiming_point(saucer.position, ship_or_none.position, u.SCREEN_SIZE)
        delta_position = target_position - saucer.position
        aim_time = self.time_to_target(delta_position, ship_or_none.velocity)
        if aim_time:
            distance_to_target = aim_time * u.MISSILE_SPEED
            adjusted_distance = distance_to_target - 2 * self._radius
            adjustment_ratio = adjusted_distance / distance_to_target
        else:
            adjustment_ratio = 1
        target = target_position + ship_or_none.velocity * aim_time

        saucer_position = saucer.position
        self.create_adjusted_missile(adjustment_ratio, target, saucer_position, fleets)

    def create_adjusted_missile(self, adjustment_ratio, target_position, saucer_position, fleets):
        vector_to_target = target_position - saucer_position
        direction_to_target = vector_to_target.normalize()
        missile_velocity = u.MISSILE_SPEED * direction_to_target
        adjusted_velocity = missile_velocity * adjustment_ratio
        offset = 2 * self._radius * direction_to_target
        missile = Missile.from_saucer(saucer_position + offset, adjusted_velocity)
        fleets.append(missile)

And now I think I can do this:

    def create_unoptimized_missile(self, from_position, to_position, fleets):
        self.create_adjusted_missile(1, to_position, from_position, fleets)

Note: Extracting the more powerful method worked out perfectly: it allowed us to replace all the code of the simplified one with a call to the more powerful one. I think that worked out very nicely.

I think this code is improved enough to commit. Tests are green and I think it’s better if not great.

Commit: refactoring Gunner. Flaky adjustment member removed.

Note: This has been so stress-free that I am not even slightly tired at this point. So I decide to continue refactoring for improved clarity.; We’ll split the article here, since there’s an actual commit. More refactoring in the next article, all from the same morning session.

Next in series