Python Asteroids on GitHub

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
else:
target = target_position + ship_or_none.velocity * aim_time
saucer_position = saucer.position
``````

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)
target = target_position + ship_or_none.velocity * aim_time
saucer_position = saucer.position

if not aim_time:
else:
distance_to_target = aim_time * u.MISSILE_SPEED
adjusted_distance = distance_to_target - 2 * self._radius
``````

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:
else:

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

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:
else:
``````

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)
target = target_position + ship_or_none.velocity * aim_time
saucer_position = saucer.position
``````

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)
target = target_position + ship.velocity * aim_time
saucer_position = saucer.position
``````

### 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)
target = target_position + ship.velocity * aim_time
saucer_position = saucer.position
``````
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

def optimal_shot(self, delta_position, ship):
aim_time = self.time_to_target(delta_position, ship.velocity)
``````

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
``````

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
``````

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

def optimal_shot(self, delta_position, ship):
aim_time = self.time_to_target(delta_position, ship.velocity)
``````

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

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

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

vector_to_target = target_position - saucer_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(saucer_position + offset, adjusted_velocity)
fleets.append(missile)
``````

I wonder if we could make that initial `if`batch 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