Python 128 - Code Review
I had an idea last night as I was dozing off. If I can remember it, I’d like to try it out. This turns into a long series of revisions of about ten lines of code. Fun for a Saturday.
One of the nastiest tricks in the Saucer’s book is that when it targets the ship, it is smart enough to fire over the border if that is a shorter shot. For example, suppose the Saucer is close to the left side of the screen and the ship is close to the right. Then it makes sense for the saucer to fire left, back over the edge, so that the missile comes at the ship from the right.
I wrote up how that works back in Python 048 - Targeting, which includes this picture of a possible scenario:
If we consider the game screen to be a view into a larger space, we can see that there are secondary occurrences, or echoes of the ship. If the screen size is s, and the ship is at (x,y), it is also seen at (x+s,y), (x-s,y+s), and so on. (The process continues for 2s, 3s, but those copies are guaranteed to be further away.)
So in the picture above, our best shot is the red line, firing at (x+s,y+s) rather than at (x,y).
The code that figures that out works on the coordinates independently, and it looks like this:
def nearest(shooter, target, wrap_size):
direct_distance = abs(target - shooter)
target_wrap_left = target - wrap_size
wrap_left_distance = abs(target_wrap_left - shooter)
target_wrap_right = target + wrap_size
wrap_right_distance = abs(target_wrap_right - shooter)
if wrap_left_distance < direct_distance:
return target_wrap_left
elif wrap_right_distance < direct_distance:
return target_wrap_right
else:
return target
I see that better names would help here. We’re going to replace this so let me just explain.
The shooter and target are numbers, the x or y coordinate of shooter and target respectively. So direct_distance, the absolute value of the difference of those, is the distance between the shooter and target on that axis.
Then target_wrap_left is the target’s coordinate minus the screen size, and the wrap_left_distance is the distance to that coordinate. And so on.
The code calculates the two alternate points and distances, and picks the coordinate whose distance is less than direct, if there is one, otherwise returning the direct one.
Last night, as I was dozing off, I had an idea for a better way. I made a point of thinking about it long enough so that I might not forget, and Mirabile Dictu! I didn’t forget.
We’ll just consider one axis: it works the same for either.
Take the direct distance to the target. If it is less than or equal to half the screen size, it’s the shortest possible shot. And if it is greater than half the screen size, one of the other two shots is better.
Which one? Whichever side the shooter is closer to! If shooter’s coordinate is greater than half the screen size, shoot at target+screen_size, if not, shoot at target-screen_size.
So this is much simpler. It may also be more obscure but I’ll want to see it in code before I decide that. The nearest function above is no paragon of obviousness.
We have sufficient tests for this feature:
def test_can_choose_nearest_scalar_target(self):
target = 100
screen_size = 500
shooter = 200
assert nearest(shooter, target, screen_size) == 100
shooter = 400
assert nearest(shooter, target, screen_size) == 100 + 500
target = 400
shooter = 100
assert nearest(shooter, target, screen_size) == 400 - 500
def test_can_choose_nearest_point(self):
target = Vector2(100, 400)
screen_size = 500
shooter = Vector2(150, 150)
assert nearest_point(shooter, target, screen_size) == Vector2(100, 400)
shooter = Vector2(150, 50)
assert nearest_point(shooter, target, screen_size) == Vector2(100, -100)
I think I’ll just do nearest with the new algorithm and see what we get.
def nearest(shooter_coord, target_coord, wrap_size):
half = wrap_size / 2
direct_distance = abs(target_coord - shooter_coord)
if direct_distance <= half:
return target_coord
if shooter_coord >= half:
return target_coord + wrap_size
else:
return target_coord - wrap_size
The tests are green. In the game, I cruised over to the right, the saucer emerged on the left and fired one right up my tailpipe.
I think an else-if goes better here.
def nearest(shooter_coord, target_coord, wrap_size):
half = wrap_size / 2
direct_distance = abs(target_coord - shooter_coord)
if direct_distance <= half:
return target_coord
elif shooter_coord >= half:
return target_coord + wrap_size
else:
return target_coord - wrap_size
Could this be more clear? Let’s try something.
def nearest(shooter_coord, target_coord, wrap_size):
middle = wrap_size / 2
direct_distance = abs(target_coord - shooter_coord)
direct_is_best = direct_distance <= middle
wrap_high_is_best = shooter_coord >= middle
if direct_is_best:
return target_coord
elif wrap_high_is_best:
return target_coord + wrap_size
else:
return target_coord - wrap_size
Better? I’m not sure. The big block of text at the top bothers me. We could do this:
def nearest(shooter_coord, target_coord, wrap_size):
middle = wrap_size / 2
direct_distance = abs(target_coord - shooter_coord)
direct_is_best = direct_distance <= middle
if direct_is_best:
return target_coord
wrap_high_is_best = shooter_coord >= middle
if wrap_high_is_best:
return target_coord + wrap_size
else:
return target_coord - wrap_size
Or this:
def nearest(shooter_coord, target_coord, wrap_size):
middle = wrap_size / 2
direct_distance = abs(target_coord - shooter_coord)
direct_is_best = direct_distance <= middle
wrap_high_is_best = shooter_coord >= middle
return target_coord if direct_is_best \
else (target_coord + wrap_size if wrap_high_is_best
else target_coord - wrap_size)
That one has the “advantage” of a single return statement, which one might prefer over three. It has the disadvantage of being the first nested if-else expression I’ve ever seen.
We could avoid the returns by saving a result and returning it:
def nearest(shooter_coord, target_coord, wrap_size):
middle = wrap_size / 2
direct_distance = abs(target_coord - shooter_coord)
direct_is_best = direct_distance <= middle
wrap_high_is_best = shooter_coord >= middle
if direct_is_best:
best_shot = target_coord
elif wrap_high_is_best:
best_shot = target_coord + wrap_size
else:
best_shot = target_coord - wrap_size
return best_shot
We may not have the best names. For example:
def nearest(shooter_coord, target_coord, wrap_size):
middle = wrap_size / 2
direct_distance = abs(target_coord - shooter_coord)
direct_is_best = direct_distance <= middle
prefer_to_shoot_right = shooter_coord >= middle
if direct_is_best:
best_shot = target_coord
elif prefer_to_shoot_right:
best_shot = target_coord + wrap_size
else:
best_shot = target_coord - wrap_size
return best_shot
We could select the adjustment and then add it.
def nearest(shooter_coord, target_coord, wrap_size):
middle = wrap_size / 2
direct_distance = abs(target_coord - shooter_coord)
direct_is_best = direct_distance <= middle
prefer_to_shoot_right = shooter_coord >= middle
if direct_is_best:
adjustment = 0
elif prefer_to_shoot_right:
adjustment = wrap_size
else:
adjustment = -wrap_size
return target_coord + adjustment
All of these are passing the tests, by the way.
We could compute the adjustment first. Wasteful but would it be more clear?
def nearest(shooter_coord, target_coord, wrap_size):
middle = wrap_size / 2
direct_distance = abs(target_coord - shooter_coord)
direct_is_best = direct_distance <= middle
if direct_is_best:
return target_coord
prefer_to_shoot_right = shooter_coord >= middle
adjustment = wrap_size if prefer_to_shoot_right else -wrap_size
return target_coord + adjustment
That wasn’t quite what I thought I’d do but it seemed better than whatever I was thinking.
This next will be obscure but interesting.
def nearest(shooter_coord, target_coord, wrap_size):
middle = wrap_size / 2
direct_distance = abs(target_coord - shooter_coord)
direct_is_best = direct_distance <= middle
if direct_is_best:
return target_coord
adjustment = math.copysign(wrap_size, shooter_coord - middle)
return target_coord + adjustment
If we’re on the high side, shooter_coord - middle will be positive and we add wrap_size, otherwise we subtract it.
I actually like this form quite a bit but after all this, maybe the second thing I tried is best:
def nearest(shooter_coord, target_coord, wrap_size):
half = wrap_size / 2
direct_distance = abs(target_coord - shooter_coord)
if direct_distance <= half:
return target_coord
elif shooter_coord >= half:
return target_coord + wrap_size
else:
return target_coord - wrap_size
I’m going to commit this: Revise nearest to a simpler algorithm.
Reflection
So this is interesting. I’ve just spent about an hour trying various ways of writing a function of only nine or ten lines. This on top of the fact that I was replacing a perfectly good algorithm with a slightly simpler one, in code that worked just fine and was maybe three lines longer.
Well, in my defense, it’s Saturday, so I’m allowed to have fun. And I’ve learned a few things, including how to write a nested if-expression in case I want to sin again, and I learned about copysign, which is actually a rather good Python idea.
Almost invariably, after we call sign, we multiply the result times some value to set it negative if needed. And we often have to do something special about the zero case. Python avoids all that by just setting the sign of the left value to the sign of the right value. Nice idea.
There is, however, a larger question here. The nearest function, and nearest_point, which calls nearest, are top-level functions in the Gunner module. As a old, and I really meant it, Smalltalk programmer, there is no place in my lexicon for top-level functions. Everything belongs in some object in my model. Now since I also have decades of experience in C-like languages, and many others, I do know that these things exist, but in this object-oriented design that we have here, I’d like these things to be inside some kind of object.
We could make a Targeter object with essentially these two functions as methods.
We could make them methods of Gunner. They might be static methods, but that could be OK.
class Gunner:
def create_targeted_missile(self, saucer_position, ship_position, fleets):
aiming_point = self.nearest_point(saucer_position, ship_position, u.SCREEN_SIZE)
angle = Vector2(0, 0).angle_to(aiming_point - saucer_position)
missile = self.missile_at_angle(saucer_position, angle, Vector2(0, 0))
fleets.add_flyer(missile)
def nearest_point(self, shooter, target, wrap_size):
nearest_x = self.nearest(shooter.x, target.x, wrap_size)
nearest_y = self.nearest(shooter.y, target.y, wrap_size)
return Vector2(nearest_x, nearest_y)
@staticmethod
def nearest(shooter_coord, target_coord, wrap_size):
half = wrap_size / 2
direct_distance = abs(target_coord - shooter_coord)
if direct_distance <= half:
return target_coord
elif shooter_coord >= half:
return target_coord + wrap_size
else:
return target_coord - wrap_size
This is fine, of course. The tests, however, are still calling the top-level functions. Readily fixed:
def test_can_choose_nearest_scalar_target(self):
target = 100
screen_size = 500
shooter = 200
gunner = Gunner()
assert gunner.nearest(shooter, target, screen_size) == 100
shooter = 400
assert gunner.nearest(shooter, target, screen_size) == 100 + 500
target = 400
shooter = 100
assert gunner.nearest(shooter, target, screen_size) == 400 - 500
def test_can_choose_nearest_point(self):
target = Vector2(100, 400)
screen_size = 500
shooter = Vector2(150, 150)
gunner = Gunner()
assert gunner.nearest_point(shooter, target, screen_size) == Vector2(100, 400)
shooter = Vector2(150, 50)
assert gunner.nearest_point(shooter, target, screen_size) == Vector2(100, -100)
Now I should be able to remove the top-level ones. Done and green. Commit: move nearest_point and nearest into Gunner class.
Reflection
Well, my plan this morning was to quickly deal with this new simpler notion of “nearest”, and then review Gunner and Saucer. Instead, I had lots of fun just pushing a few lines of code around, finding arrangements that might be better. Kind of like spending the morning doing one’s piano etudes, or sharpening the kitchen knives. Quiet, calm, kind of meditative. A bit of learning, and a bit of just building intuition about ways of doing things.
And the fingers are more limber and the knives are sharper.
And the code is, in my opinion, just a bit better.
- Wondering …
-
Suppose it were the case that we were on a screen, 1024 wide, with only integer coordinates. Like in the original asteroids. Coordinates would be ten-bit numbers. I wonder if we could do all this target selection with just a few bit-wise operations. But I digress.
I’m still thinking about how to make this code crystal clear. I think it’s clear enough: but practicing making code better has value to me, because it helps me write better code the first (or second or third time), and it makes it easier for me to see how to make something more clear with a small investment, rather than a whole morning, like today. (In fairness, it’s only 0920 right now, so you can’t say I’ve wasted the morning.)
What if we changed this:
def create_targeted_missile(self, saucer_position, ship_position, fleets):
aiming_point = self.nearest_point(saucer_position, ship_position, u.SCREEN_SIZE)
angle = Vector2(0, 0).angle_to(aiming_point - saucer_position)
missile = self.missile_at_angle(saucer_position, angle, Vector2(0, 0))
fleets.add_flyer(missile)
To this:
def create_targeted_missile(self, saucer_position, ship_position, fleets):
best_aiming_point = self.best_aiming_point(saucer_position, ship_position, u.SCREEN_SIZE)
angle = Vector2(0, 0).angle_to(best_aiming_point - saucer_position)
missile = self.missile_at_angle(saucer_position, angle, Vector2(0, 0))
fleets.add_flyer(missile)
Better? What about that weird angle stuff?
def create_targeted_missile(self, saucer_position, ship_position, fleets):
best_aiming_point = self.best_aiming_point(saucer_position, ship_position, u.SCREEN_SIZE)
angle = self.angle_to_hit(best_aiming_point, saucer_position)
missile = self.missile_at_angle(saucer_position, angle, Vector2(0, 0))
fleets.add_flyer(missile)
@staticmethod
def angle_to_hit(best_aiming_point, saucer_position):
return Vector2(0, 0).angle_to(best_aiming_point - saucer_position)
Better? What about this:
def create_targeted_missile(self, shooter_position, target_position, fleets):
best_aiming_point = self.best_aiming_point(shooter_position, target_position, u.SCREEN_SIZE)
angle = self.angle_to_hit(best_aiming_point, shooter_position)
missile = self.missile_at_angle(shooter_position, angle, Vector2(0, 0))
fleets.add_flyer(missile)
Or, what about this:
def create_targeted_missile(self, from_position, to_position, fleets):
best_aiming_point = self.best_aiming_point(from_position, to_position, u.SCREEN_SIZE)
angle = self.angle_to_hit(best_aiming_point, from_position)
missile = self.missile_at_angle(from_position, angle, Vector2(0, 0))
fleets.add_flyer(missile)
Too fine? Too detailed? Too nuanced? Not important enough?
I don’t know. When next I have to visit this code, I’ll want all the help I can get. Slightly better names might be just the hint I need to move a little faster and avoid an important mistake.
Summary
Does it pay off to spend a couple of hours practicing? Who knows? For me, it’s fun and its own reward. If the knives are a bit sharper, that’s great too!
Do you see a better way to have done what I’ve done here? Let me know!
See you next time!
Post-credits Scene: Inline everything, rename.
@staticmethod
def nearest(shooter_coord, target_coord, screen_size):
if abs(target_coord - shooter_coord) <= screen_size / 2:
return target_coord
elif shooter_coord >= screen_size / 2:
return target_coord + screen_size
else:
return target_coord - screen_size
Personally, I don’t like this one. I prefer the explaining variable names. I’m reverting, but I do prefer screen_size over wrap_size.
@staticmethod
def nearest(shooter_coord, target_coord, screen_size):
half = screen_size / 2
direct_distance = abs(target_coord - shooter_coord)
if direct_distance <= half:
return target_coord
elif shooter_coord >= half:
return target_coord + screen_size
else:
return target_coord - screen_size
But you know what? What if we had a tiny object and the shooter and target were its member variables …
Enough already. See you next time!