Python 153 - Further Refactoring

We continue a long refactoring session. It goes so smoothly that it all happens in one stress-free morning.

Continuing our long refactoring session, note how everything is done in small steps, mostly aimed at making things better.

Let’s look at some code that seems overly complicated by a wide margin.

    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)

What if we extract the small adjustment ratio calculation into its own method?

    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)
        adjustment_ratio = self.compute_adjustment_ratio(aim_time)
        target = target_position + ship_or_none.velocity * aim_time
        saucer_position = saucer.position
        self.create_adjusted_missile(adjustment_ratio, target, saucer_position, fleets)

    def compute_adjustment_ratio(self, aim_time):
        if not aim_time:
            adjustment_ratio = 1
        else:
            distance_to_target = aim_time * u.MISSILE_SPEED
            adjusted_distance = distance_to_target - 2 * self._radius
            adjustment_ratio = adjusted_distance / distance_to_target
        return adjustment_ratio

I inverted the if just to see if I like it. I generally prefer the short branch first. But let’s extract the long one for symmetry and because that’s what you’re supposed to do.

Now extract again:

    def compute_adjustment_ratio(self, aim_time):
        if not aim_time:
            adjustment_ratio = 1
        else:
            adjustment_ratio = self.compute_ratio(aim_time)
        return adjustment_ratio

    def compute_ratio(self, 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
        return adjustment_ratio

Inline temp variable adjustment_ratio to return result directly:

    def compute_ratio(self, aim_time):
        distance_to_target = aim_time * u.MISSILE_SPEED
        adjusted_distance = distance_to_target - 2 * self._radius
        return adjusted_distance / distance_to_target

Simplify the caller, which looks like this:

    def compute_adjustment_ratio(self, aim_time):
        if not aim_time:
            adjustment_ratio = 1
        else:
            adjustment_ratio = self.compute_ratio(aim_time)
        return adjustment_ratio

That becomes this:

    def compute_adjustment_ratio(self, aim_time):
        return 1 if not aim_time else self.compute_ratio(aim_time)

Note: I grant that some might not prefer this little conversion. I myself like it.

Invert that if, putting the complex bit better in view:

    def compute_adjustment_ratio(self, aim_time):
        return self.compute_ratio(aim_time) if aim_time else 1

I notice that while I prefer the shorter branch of an if to be first, I prefer the more complex part first here. Of course we also got rid of a not.

Nice. Commit: tidying.

Note: Nice improvements, still not remotely tired.

Now back to the big method again, after that little simplification:

    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)
        adjustment_ratio = self.compute_adjustment_ratio(aim_time)
        target = target_position + ship_or_none.velocity * aim_time
        saucer_position = saucer.position
        self.create_adjusted_missile(adjustment_ratio, target, saucer_position, fleets)

Rename ship_or_none, we know we have a ship here, can’t be None.

    def create_optimal_missile(self, fleets, saucer, ship):
        target_position = self.closest_aiming_point(saucer.position, ship.position, u.SCREEN_SIZE)
        delta_position = target_position - saucer.position
        aim_time = self.time_to_target(delta_position, ship.velocity)
        adjustment_ratio = self.compute_adjustment_ratio(aim_time)
        target = target_position + ship.velocity * aim_time
        saucer_position = saucer.position
        self.create_adjusted_missile(adjustment_ratio, target, saucer_position, fleets)

Reflection

Look back at these past couple/three changes. We’re making the code better in very small steps, each one just a bit of an improvement taking almost no time. Almost every change has been a machine refactoring and the remainder have been very small and simple. Test remain green throughout.

This is how we get to better code: bit by bit.

Moving right along, we are working to improve this:

    def create_optimal_missile(self, fleets, saucer, ship):
        target_position = self.closest_aiming_point(saucer.position, ship.position, u.SCREEN_SIZE)
        delta_position = target_position - saucer.position
        aim_time = self.time_to_target(delta_position, ship.velocity)
        adjustment_ratio = self.compute_adjustment_ratio(aim_time)
        target = target_position + ship.velocity * aim_time
        saucer_position = saucer.position
        self.create_adjusted_missile(adjustment_ratio, target, saucer_position, fleets)

Note: This next one is, as the text says, a bit odd. See what you think.

I think I’d like to try something a bit odd. Can I extract those two lines in the middle, that get the time and the ratio?

    def create_optimal_missile(self, fleets, saucer, ship):
        target_position = self.closest_aiming_point(saucer.position, ship.position, u.SCREEN_SIZE)
        delta_position = target_position - saucer.position
        adjustment_ratio, aim_time = self.optimal_shot(delta_position, ship)
        target = target_position + ship.velocity * aim_time
        saucer_position = saucer.position
        self.create_adjusted_missile(adjustment_ratio, target, saucer_position, fleets)

    def optimal_shot(self, delta_position, ship):
        aim_time = self.time_to_target(delta_position, ship.velocity)
        adjustment_ratio = self.compute_adjustment_ratio(aim_time)
        return adjustment_ratio, aim_time

I think I may like that. Some may disagree about returning two things from one function. This does suggest that there might be a tiny object here with two computed members we could access. This will do for now.

Reorder method lines to create saucer_position sooner and use it:

    def create_optimal_missile(self, fleets, saucer, ship):
        saucer_position = saucer.position
        target_position = self.closest_aiming_point(saucer_position, ship.position, u.SCREEN_SIZE)
        delta_position = target_position - saucer_position
        adjustment_ratio, aim_time = self.optimal_shot(delta_position, ship)
        target = target_position + ship.velocity * aim_time
        self.create_adjusted_missile(adjustment_ratio, target, saucer_position, fleets)

Rename a couple of things:

    def create_optimal_missile(self, fleets, saucer, ship):
        saucer_position = saucer.position
        closest_shot_position = self.closest_aiming_point(saucer_position, ship.position, u.SCREEN_SIZE)
        delta_position = closest_shot_position - saucer_position
        adjustment_ratio, aim_time = self.optimal_shot(delta_position, ship)
        target_position = closest_shot_position + ship.velocity * aim_time
        self.create_adjusted_missile(adjustment_ratio, target_position, saucer_position, fleets)

I like this better. Commit: tidying.

I’d like that double-return function’s results better in the other order. I do this change by hand, as no PyCharm refactoring is offered, but I do edit the two pairs with multi-cursor to keep them correct.

    def create_optimal_missile(self, fleets, saucer, ship):
        saucer_position = saucer.position
        closest_shot_position = self.closest_aiming_point(saucer_position, ship.position, u.SCREEN_SIZE)
        delta_position = closest_shot_position - saucer_position
        # change in next line
        aim_time, adjustment_ratio = self.optimal_shot(delta_position, ship)
        target_position = closest_shot_position + ship.velocity * aim_time
        self.create_adjusted_missile(adjustment_ratio, target_position, saucer_position, fleets)

    def optimal_shot(self, delta_position, ship):
        aim_time = self.time_to_target(delta_position, ship.velocity)
        adjustment_ratio = self.compute_adjustment_ratio(aim_time)
        return aim_time, adjustment_ratio

Commit: reverse return order.

I feel the need to play the game. Must think why. Things look good. If I can understand why I wanted to try it, I might be able to think of a test that’s needed. I’m not sure. Just a bit nervous, I guess.

Note: Still not tired, diving back in. This is going very well. Please notice, not so much the details, but the tiny steps taken. Each of these could be committed, but at least we are up to date as of now.

Let’s review the whole picture.

    def fire_available_missile(self, chance, fleets, saucer, ship_or_none):
        if not ship_or_none:
            target = self.random_position()
            self.create_unoptimized_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_unoptimized_missile(saucer.position, target, fleets)

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

    def create_optimal_missile(self, fleets, saucer, ship):
        saucer_position = saucer.position
        closest_shot_position = self.closest_aiming_point(saucer_position, ship.position, u.SCREEN_SIZE)
        delta_position = closest_shot_position - saucer_position
        aim_time, adjustment_ratio = self.optimal_shot(delta_position, ship)
        target_position = closest_shot_position + ship.velocity * aim_time
        self.create_adjusted_missile(adjustment_ratio, target_position, saucer_position, fleets)

    def optimal_shot(self, delta_position, ship):
        aim_time = self.time_to_target(delta_position, ship.velocity)
        adjustment_ratio = self.compute_adjustment_ratio(aim_time)
        return aim_time, adjustment_ratio

    def compute_adjustment_ratio(self, aim_time):
        return self.compute_ratio(aim_time) if aim_time else 1

    def compute_ratio(self, aim_time):
        distance_to_target = aim_time * u.MISSILE_SPEED
        adjusted_distance = distance_to_target - 2 * self._radius
        return adjusted_distance / distance_to_target

    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)

I wonder if we could make that initial ifbatch a bit more intelligible.

    def fire_available_missile(self, chance, fleets, saucer, ship_or_none):
        if not ship_or_none:
            target = self.random_position()
            self.create_unoptimized_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_unoptimized_missile(saucer.position, target, fleets)

We could move the random position code inside the create unoptimized. That might look better. Do it with Extract Method:

    def fire_available_missile(self, chance, fleets, saucer, ship_or_none):
        if not ship_or_none:
            self.create_random_missile(fleets, saucer)
        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:
            self.create_random_missile(fleets, saucer)

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

I did it with a new method because there are tests that pass values to create_unoptimized_missile. I wanted them to have the ability to set desired values. It would be good to change this but we are on a roll. This change may have been better even without the test concern: I’m not sure.

Can we simplify the if nest somehow? Combine two cases with or:

    def fire_available_missile(self, chance, fleets, saucer, ship_or_none):
        if not ship_or_none:
            self.create_random_missile(fleets, saucer)
        ## change in next line
        elif saucer.always_target or chance < u.SAUCER_TARGETING_FRACTION:
            self.create_optimal_missile(fleets, saucer, ship_or_none)
        else:
            self.create_random_missile(fleets, saucer)

Then extract a named condition:

    def fire_available_missile(self, chance, fleets, saucer, ship_or_none):
        if not ship_or_none:
            self.create_random_missile(fleets, saucer)
        # extract should_target below
        elif 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

We can invert those last two branches in the if:

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

Now we have two visible adjacent random ones. We can combine them:

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

Now we can invert the if. PyCharm will do it. It knows logic.

    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)

Note: Isn’t that nice? Four odd cases collapse to two. I think that’s quite sweet.

That seems to me to be a clear improvement. Commit: tidying.

We’ll review the code again now. But first a general comment:

General Comment: This session has gone on so long that I’ve broken it into four articles of which this is the third. What is significant, I think, is that this is all one session, and I am hardly even tired, because the changes have almost all been machine refactorings, and the few other changes have been very simple and have not required a lot of thinking or wondering.; We’ll come back to this in the final summary, I expect. We’ll break here.

Next in series