Python 155 - Some small improvements
This afternoon, I notice something that could be better. Let’s see if we can find a bit of improvement.
I happened to glance at PyCharm and saw this:
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.velocity_adjustment(aim_time)
return aim_time, adjustment_ratio
At first, I was just thinking that maybe closest_shot_position should be done first, and that maybe it needed a better name. But then I noticed that optimal_shot has an odd choice of parameters. One is a point and the other is a game object. Inside, from the game object, the method extracts velocity. Let’s correct the imbalance:
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
delta_velocity = ship.velocity # we treat saucer as not moving
aim_time, adjustment_ratio = self.optimal_shot(delta_position, delta_velocity)
target_position = closest_shot_position + delta_velocity * aim_time
self.create_adjusted_missile(adjustment_ratio, target_position, saucer_position, fleets)
def optimal_shot(self, delta_position, delta_velocity):
aim_time = self.time_to_target(delta_position, delta_velocity)
adjustment_ratio = self.velocity_adjustment(aim_time)
return aim_time, adjustment_ratio
Better. The name of the method isn’t great. But let’s look more deeply:
def optimal_shot(self, delta_position, delta_velocity):
aim_time = self.time_to_target(delta_position, delta_velocity)
adjustment_ratio = self.velocity_adjustment(aim_time)
return aim_time, adjustment_ratio
def velocity_adjustment(self, aim_time):
return self.compensate_for_offset(aim_time) if aim_time else 1
def compensate_for_offset(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
It seems to me that once we are down here near the geometry, we shouldn’t be reaching back up to get self._radius. Let’s pass that in as another parameter:
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
delta_velocity = ship.velocity # we treat saucer as not moving
aim_time, adjustment_ratio = self.optimal_shot(delta_position, delta_velocity, 2*self._radius)
target_position = closest_shot_position + delta_velocity * aim_time
self.create_adjusted_missile(adjustment_ratio, target_position, saucer_position, fleets)
def optimal_shot(self, delta_position, delta_velocity, initial_offset):
aim_time = self.time_to_target(delta_position, delta_velocity)
adjustment_ratio = self.velocity_adjustment(aim_time, initial_offset)
return aim_time, adjustment_ratio
def velocity_adjustment(self, aim_time, initial_offset):
return self.compensate_for_offset(aim_time, initial_offset) if aim_time else 1
def compensate_for_offset(self, aim_time, initial_offset):
distance_to_target = aim_time * u.MISSILE_SPEED
adjusted_distance = distance_to_target - initial_offset
return adjusted_distance / distance_to_target
PyCharm points out that compensate can be static. Now none of optimal_shot, velocity_adjustment, nor compensate_for_offset are really referring to the Gunner or Saucer at all.
This could be a new object, I think. I feel that one has been trying to hatch for quite a while.
Let’s commit what we have, as it is working well and a bit better.
Commit: refactoring toward separate shot optimizer?
Extraction
Now how can we extract a class from here? In particular, I’d like to have some tests, but let’s see if we can just do what’s needed.
Let’s create a tiny class and see if we can do a strangler conversion. As you’ll see, by that I mean that we’ll create a small separate class, pass it the caller at first, and call back to get the work done. Then we’ll move the actual code over, bit by bit, keeping the thing running all the time.
The “strangler” idea comes from Michael Feathers, I believe, in his amazing book Working Effectively with Legacy Code.
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
delta_velocity = ship.velocity # we treat saucer as not moving
initial_offset = 2*self._radius
optimizer = ShotOptimizer(self, delta_position, delta_velocity, initial_offset)
aim_time, adjustment_ratio = optimizer.optimize()
target_position = closest_shot_position + delta_velocity * aim_time
self.create_adjusted_missile(adjustment_ratio, target_position, saucer_position, fleets)
And the new class so far:
class ShotOptimizer:
def __init__(self, gunner, delta_position, delta_velocity, initial_offset):
self._gunner = gunner
self._delta_position = delta_position
self._delta_velocity = delta_velocity
self._initial_offset = initial_offset
def optimize(self):
return self._gunner.optimal_shot(self._delta_position, self._delta_velocity, self._initial_offset)
As you see, we’ve passed in the gunner as a temporary measure, and we’re just calling back to gunner to do the work for us. Tests are green.
Now we can move just one method and repeat the trick.
def optimize(self):
return self.optimal_shot(self._delta_position, self._delta_velocity, self._initial_offset)
def optimal_shot(self, delta_position, delta_velocity, initial_offset):
aim_time = self._gunner.time_to_target(delta_position, delta_velocity)
adjustment_ratio = self._gunner.velocity_adjustment(aim_time, initial_offset)
return aim_time, adjustment_ratio
We could be committing each of these, at least to my local repo. Let’s not, while that would be a better habit for me, I don’t want to change habit in the middle of a strangler.
We can remove the optimal_shot method from Gunner, and we do. Let’s move time_to_target. It has lots of tests, so we’ll copy it over and not remove the old version from Gunner yet.
def optimal_shot(self, delta_position, delta_velocity, initial_offset):
aim_time = self.time_to_target(delta_position, delta_velocity)
adjustment_ratio = self._gunner.velocity_adjustment(aim_time, initial_offset)
return aim_time, adjustment_ratio
@staticmethod
def time_to_target(delta_position, relative_velocity):
# from https://www.gamedeveloper.com/programming/shooting-a-moving-target#close-modal
# return time for hit or -1
# quadratic
a = relative_velocity.dot(relative_velocity) - u.MISSILE_SPEED*u.MISSILE_SPEED
b = 2 * relative_velocity.dot(delta_position)
c = delta_position.dot(delta_position)
disc = b*b - 4*a*c
if disc < 0:
return 0
else:
divisor = (math.sqrt(disc) - b)
if divisor:
return 2*c / divisor
else:
return 0
Green again. We have to import math and u, of course. Scared me for a moment when some tests failed.
What’s next? velocity_adjustment. Let’s go nuts and move two at once.
class ShotOptimizer:
def __init__(self, gunner, delta_position, delta_velocity, initial_offset):
self._gunner = gunner
self._delta_position = delta_position
self._delta_velocity = delta_velocity
self._initial_offset = initial_offset
def optimize(self):
return self.optimal_shot(self._delta_position, self._delta_velocity, self._initial_offset)
def optimal_shot(self, delta_position, delta_velocity, initial_offset):
aim_time = self.time_to_target(delta_position, delta_velocity)
adjustment_ratio = self.velocity_adjustment(aim_time, initial_offset)
return aim_time, adjustment_ratio
def velocity_adjustment(self, aim_time, initial_offset):
return self.compensate_for_offset(aim_time, initial_offset) if aim_time else 1
@staticmethod
def compensate_for_offset(aim_time, initial_offset):
distance_to_target = aim_time * u.MISSILE_SPEED
adjusted_distance = distance_to_target - initial_offset
return adjusted_distance / distance_to_target
@staticmethod
def time_to_target(delta_position, relative_velocity):
# from https://www.gamedeveloper.com/programming/shooting-a-moving-target#close-modal
# return time for hit or -1
# quadratic
a = relative_velocity.dot(relative_velocity) - u.MISSILE_SPEED*u.MISSILE_SPEED
b = 2 * relative_velocity.dot(delta_position)
c = delta_position.dot(delta_position)
disc = b*b - 4*a*c
if disc < 0:
return 0
else:
divisor = (math.sqrt(disc) - b)
if divisor:
return 2*c / divisor
else:
return 0
Now ShotOptimizer isn’t using the gunner. Remove it from the init and its sole use.
class Gunner:
...
optimizer = ShotOptimizer(delta_position, delta_velocity, initial_offset)
...
class ShotOptimizer:
def __init__(self, delta_position, delta_velocity, initial_offset):
self._delta_position = delta_position
self._delta_velocity = delta_velocity
self._initial_offset = initial_offset
So that’s mostly done. However, now we can move away from that slightly funky two-element return. It’s considered Pythonic but here I think we can do better.
We’ll just code what we want to say (design by intention) and then make it work.
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
delta_velocity = ship.velocity # we treat saucer as not moving
initial_offset = 2*self._radius
optimizer = ShotOptimizer(delta_position, delta_velocity, initial_offset)
aim_time = optimizer.aim_time
adjustment_ratio = optimizer.adjustment_ratio
target_position = closest_shot_position + delta_velocity * aim_time
self.create_adjusted_missile(adjustment_ratio, target_position, saucer_position, fleets)
Now to make that work, let’s do this:
class ShotOptimizer:
def __init__(self, delta_position, delta_velocity, initial_offset):
self.aim_time, self.adjustment_ratio = self.optimal_shot(delta_position, delta_velocity, initial_offset)
def optimal_shot(self, delta_position, delta_velocity, initial_offset):
aim_time = self.time_to_target(delta_position, delta_velocity)
adjustment_ratio = self.velocity_adjustment(aim_time, initial_offset)
return aim_time, adjustment_ratio
Right in __init__, we call our optimal_shot method and save the members for our user to fetch them. The old optimize method just goes away, we do all the work in __init__.Nice.
Commit: Gunner now uses ShotOptimizer to, well, optimize a shot.
Works a treat, it does. Now let’s clean up the leftovers in Gunner. We need to redirect some tests.
The tests work fine if I just plug in ShotOptimizer. Perfect. Nothing to see there.
Commit: remove unused optimizer methods from Gunner.
So this is definitely good. Gunner is simpler and ShotOptimizer is pretty nice as well.
Now let’s look at this, in Gunner:
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
delta_velocity = ship.velocity # we treat saucer as not moving
initial_offset = 2*self._radius
optimizer = ShotOptimizer(delta_position, delta_velocity, initial_offset)
aim_time = optimizer.aim_time
adjustment_ratio = optimizer.adjustment_ratio
target_position = closest_shot_position + delta_velocity * aim_time
self.create_adjusted_missile(adjustment_ratio, target_position, saucer_position, fleets)
I think we can improve this with a bit of renaming, and maybe even some inlining. Let’s try it.
def create_optimal_missile(self, fleets, saucer, ship):
saucer_position = saucer.position
best_target_position = self.closest_aiming_point(saucer_position, ship.position, u.SCREEN_SIZE)
delta_position = best_target_position - saucer_position
delta_velocity = ship.velocity # we treat saucer as not moving
missile_head_start = 2*self._radius
optimizer = ShotOptimizer(delta_position, delta_velocity, missile_head_start)
future_target_position = best_target_position + delta_velocity * optimizer.aim_time
self.create_adjusted_missile(optimizer.adjustment_ratio, future_target_position, saucer_position, fleets)
I think I prefer that, though I do believe it could be better. We’ll let it sit and think about it again.
Commit: tidying.
If we look at these two methods together we see an anomaly: we are computing 2*self._radius twice:
def create_optimal_missile(self, fleets, saucer, ship):
saucer_position = saucer.position
best_target_position = self.closest_aiming_point(saucer_position, ship.position, u.SCREEN_SIZE)
delta_position = best_target_position - saucer_position
delta_velocity = ship.velocity # we treat saucer as not moving
missile_head_start = 2*self._radius
optimizer = ShotOptimizer(delta_position, delta_velocity, missile_head_start)
future_target_position = best_target_position + delta_velocity * optimizer.aim_time
self.create_adjusted_missile(optimizer.adjustment_ratio, future_target_position, saucer_position, fleets)
def create_adjusted_missile(self, velocity_adjustment, 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 * velocity_adjustment
offset = 2 * self._radius * direction_to_target
missile = Missile.from_saucer(saucer_position + offset, adjusted_velocity)
fleets.append(missile)
Better to pass the offset in, since we have defined it in the top method.
A first attempt at that breaks things. Not sure what I did wrong, roll back. Try again more slowly.
def create_optimal_missile(self, fleets, saucer, ship):
saucer_position = saucer.position
best_target_position = self.closest_aiming_point(saucer_position, ship.position, u.SCREEN_SIZE)
delta_position = best_target_position - saucer_position
delta_velocity = ship.velocity # we treat saucer as not moving
missile_head_start = 2*self._radius
optimizer = ShotOptimizer(delta_position, delta_velocity, missile_head_start)
future_target_position = best_target_position + delta_velocity * optimizer.aim_time
self.create_adjusted_missile(
missile_head_start,
optimizer.adjustment_ratio,
future_target_position,
saucer_position,
fleets)
@staticmethod
def create_adjusted_missile(missile_head_start, velocity_adjustment, 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 * velocity_adjustment
offset = missile_head_start * 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):
self.create_adjusted_missile(
2*self._radius,
1,
to_position,
from_position,
fleets)
We’re green as Kermit. Commit: tidying, remove reference to radius.
I think those two methods are still too large at 8 and 7 lines long (wow, huge!), but I don’t quite see what to do with them just now. We’ll let them steep a bit and see what we come up with another time.
Summary
A second object has been hinting at being born for a long time, inside Gunner. Today it came out of the egg as ShotOptimizer, which removes all the basic arithmetic from Gunner, leaving Gunner to manage the objects and ShotOptimizer the math. It seems like a rather decent split of responsibility.
Could we have done this split sooner, and if so, would it have helped? Don’t know, don’t care. I doubt that one path would have been much shorter than the other, and even so, this is the path we found and we like where we are.
Small steps that (seem to) move us toward better. And sure enough, our code gets better.
Let me emphasize once again that these changes may not be ones we would trouble ourselves to make if all we were doing here was making Asteroids work. It has been working all along. We are exploring ways of making any existing code better, using small steps, built-in refactoring tools, decent tests, and such brain power as we can muster.
And, as so often happens, when we slowly improve the code, the code slowly improves. Small, safe steps, no risky big moves. And we like that!
See you next time!