Python 050 - Group Help
After Hill’s good advice yesterday, the Zoom group last night came up with something even better.
Today is Wednesday. At last night’s Friday Geeks’ Night Out Zoom Ensemble, I hadn’t intended to show the code that Hill suggested, but the topic came up and we looked at it again.
The code that I had wound up with after Hill’s advice was this:
def create_missile(self, ships):
should_target = random.random()
random_angle = random.random()
degrees, velocity_adjustment = self.missile_spec(should_target, random_angle, ships)
return self.missile_at_angle(degrees, velocity_adjustment)
def missile_spec(self, should_target, random_angle, ships):
if ships and should_target <= u.SAUCER_TARGETING_FRACTION:
velocity_adjustment = Vector2(0, 0)
ship = ships[0]
degrees = self.angle_to(ship.position)
else:
velocity_adjustment = self.velocity
degrees = random_angle * 360.0
return degrees, velocity_adjustment
def missile_at_angle(self, degrees, velocity_adjustment):
missile_velocity = Vector2(u.MISSILE_SPEED, 0).rotate(degrees) + velocity_adjustment
offset = Vector2(2 * self.radius, 0).rotate(degrees)
return Missile.from_saucer(self.position + offset, missile_velocity)
def angle_to(self, position):
aiming_point = nearest_point(self.position, position, u.SCREEN_SIZE)
angle_point = aiming_point - self.position
return degrees(atan2(angle_point.y, angle_point.x))
Hill (he’s everywhere) noted the function returning two results:
degrees, velocity_adjustment = self.missile_spec(should_target, random_angle, ships)
He was making the point that the language we program in modifies how we might express things, and was saying that as a dedicated devotee of compile-time typed languages like Kotlin, he would likely make that pair at least a data class. His point, and it’s a good one, is that returning those two things together suggests that they are a thing, and the method that creates them suggests what that thing is, possibly a “missile spec”.
I put in a simple nested class to show what it would look like, and our thinking really underlined Hill’s point, because the group sort of split on whether that was worth doing, with Hill saying he absolutely would, down to Bryan and me, who almost certainly wouldn’t, since it amounts to a single-use class.
But then, I’m pretty sure it was Bill who suggested a different idea. Instead of computing a spec, why not have a method that takes the random input and returns the missile? I refactored to that, calling the new method suitable_missile, and after a bit more refactoring, the code now looks like this:
def create_missile(self, ships):
should_target = random.random()
random_angle = random.random()
return self.suitable_missile(should_target, random_angle, ships)
def suitable_missile(self, should_target, random_angle, ships):
if self.targeting_ship(ships, should_target):
targeting_angle = self.angle_to(ships[0])
zero_velocity = Vector2(0, 0)
return self.missile_at_angle(targeting_angle, zero_velocity)
return self.missile_at_angle(random_angle * 360.0, self.velocity)
def targeting_ship(self, ships, should_target):
return ships and should_target <= u.SAUCER_TARGETING_FRACTION
def missile_at_angle(self, degrees, velocity_adjustment):
missile_velocity = Vector2(u.MISSILE_SPEED, 0).rotate(degrees) + velocity_adjustment
offset = Vector2(2 * self.radius, 0).rotate(degrees)
return Missile.from_saucer(self.position + offset, missile_velocity)
def angle_to(self, ship):
aiming_point = nearest_point(self.position, ship.position, u.SCREEN_SIZE)
angle_point = aiming_point - self.position
return degrees(atan2(angle_point.y, angle_point.x))
Here, to create a missile, we roll two “dice”. We pass the rolls to suitable_missile. The point of this step is to make suitable_missile testable, similarly to what we did yesterday afternoon, according to Hill’s hint.
To get a suitable missile, if we’re targeting the ship, we return a missile at a carefully crafted angle, and if not, at a random angle. The velocity thing is a bit of a hack, used to ensure that a targeted missile doesn’t accumulate the ship’s velocity and go off target.
Everything else is the same. But the code no longer has the need to return the two values, because it creates the values and then uses them immediately. It seemed to all of us to make more sense that way and the bump that caused the difference between us is now gone. No one felt the need to create an object for the angle and velocity in this scheme.
There’s one thing that I don’t like in this code:
def suitable_missile(self, should_target, random_angle, ships):
if self.targeting_ship(ships, should_target):
targeting_angle = self.angle_to(ships[0])
zero_velocity = Vector2(0, 0)
return self.missile_at_angle(targeting_angle, zero_velocity)
return self.missile_at_angle(random_angle * 360.0, self.velocity)
Bryan wanted to use the Guard Clause style above. I think it’s better with an else. Let’s refactor to an earlier form so that you can better see why. Here’s what it looked like before I inlined the second set of parameters:
def suitable_missile(self, should_target, random_angle, ships):
if self.targeting_ship(ships, should_target):
targeting_angle = self.angle_to(ships[0])
velocity_adjustment = Vector2(0, 0)
return self.missile_at_angle(targeting_angle, velocity_adjustment)
targeting_angle = random_angle * 360.0
velocity_adjustment = self.velocity
return self.missile_at_angle(targeting_angle, velocity_adjustment)
Same thing, different form. In either form, I think a multi-line “guard clause” is strange, and I prefer this:
def suitable_missile(self, should_target, random_angle, ships):
if self.targeting_ship(ships, should_target):
targeting_angle = self.angle_to(ships[0])
velocity_adjustment = Vector2(0, 0)
return self.missile_at_angle(targeting_angle, velocity_adjustment)
else:
targeting_angle = random_angle * 360.0
velocity_adjustment = self.velocity
return self.missile_at_angle(targeting_angle, velocity_adjustment)
This is the point from which we actually did the Guard Clause refactoring. I’m happy with inlining the bottom two parameters:
def suitable_missile(self, should_target, random_angle, ships):
if self.targeting_ship(ships, should_target):
targeting_angle = self.angle_to(ships[0])
velocity_adjustment = Vector2(0, 0)
return self.missile_at_angle(targeting_angle, velocity_adjustment)
else:
return self.missile_at_angle(random_angle * 360.0, self.velocity)
Inlining the other two parameters makes the first call to missile_at_angle look too messy with all the nested parentheses. So I wouldn’t do that.
Now when we look at this, we see the real structure. We also see that the simpler part of the branching is second. First might be better. Can we invert the if? PyCharm is happy to do that:
def suitable_missile(self, should_target, random_angle, ships):
if not self.targeting_ship(ships, should_target):
return self.missile_at_angle(random_angle * 360.0, self.velocity)
else:
targeting_angle = self.angle_to(ships[0])
velocity_adjustment = Vector2(0, 0)
return self.missile_at_angle(targeting_angle, velocity_adjustment)
Now here, with a one-liner, I am more willing to use Guard Clause. Let’s do that:
def suitable_missile(self, should_target, random_angle, ships):
if not self.targeting_ship(ships, should_target):
return self.missile_at_angle(random_angle * 360.0, self.velocity)
targeting_angle = self.angle_to(ships[0])
velocity_adjustment = Vector2(0, 0)
return self.missile_at_angle(targeting_angle, velocity_adjustment)
Of course now we don’t like that not. Let’s extract a method, call it cannot_target_ship. For some reason, PyCharm refuses, but I know how to do this.
def suitable_missile(self, should_target, random_angle, ships):
if self.cannot_target_ship(ships, should_target):
return self.missile_at_angle(random_angle * 360.0, self.velocity)
targeting_angle = self.angle_to(ships[0])
velocity_adjustment = Vector2(0, 0)
return self.missile_at_angle(targeting_angle, velocity_adjustment)
def cannot_target_ship(self, ships, should_target):
return not self.targeting_ship(ships, should_target)
def targeting_ship(self, ships, should_target):
return ships and should_target <= u.SAUCER_TARGETING_FRACTION
And now we can inline:
def cannot_target_ship(self, ships, should_target):
return not (ships and should_target <= u.SAUCER_TARGETING_FRACTION)
And now we can apply some boolean logic to that not
def cannot_target_ship(self, ships, should_target):
return not ships or should_target > u.SAUCER_TARGETING_FRACTION
And we wind up with this:
def create_missile(self, ships):
should_target = random.random()
random_angle = random.random()
return self.suitable_missile(should_target, random_angle, ships)
def suitable_missile(self, should_target, random_angle, ships):
if self.cannot_target_ship(ships, should_target):
return self.missile_at_angle(random_angle * 360.0, self.velocity)
targeting_angle = self.angle_to(ships[0])
velocity_adjustment = Vector2(0, 0)
return self.missile_at_angle(targeting_angle, velocity_adjustment)
def cannot_target_ship(self, ships, should_target):
return not ships or should_target > u.SAUCER_TARGETING_FRACTION
def missile_at_angle(self, degrees, velocity_adjustment):
missile_velocity = Vector2(u.MISSILE_SPEED, 0).rotate(degrees) + velocity_adjustment
offset = Vector2(2 * self.radius, 0).rotate(degrees)
return Missile.from_saucer(self.position + offset, missile_velocity)
def angle_to(self, ship):
aiming_point = nearest_point(self.position, ship.position, u.SCREEN_SIZE)
angle_point = aiming_point - self.position
return degrees(atan2(angle_point.y, angle_point.x))
Is this better? I think so. Is it perfect? I don’t think so. Should we try to improve it further? Perhaps, but let’s reflect here, starting with “Would you really do this?”
Reflection
In production code, would I really do this? Perhaps not, for at least two reasons, one of them bad and the other … not so good.
First, in a production environment, I’d be feeling pressure to ship these targeted missiles, and as soon as I had decent code and decent tests, I’d probably stop refactoring. I do not expect to revisit this code over the lifetime of Asteroids … except that I know already of a way to make it a bit better: aim ahead of the ship’s path, leading the target.
Even so, I’d be thinking, well, this is going to change, and it’s clear enough now, and I can clean it more when and if I come back.
This is good reasoning if the code is clear enough that when we come back to it, or a different programmer comes back to it, it’s easy to understand and change. All true, but we’re under pressure to deliver and as such, our decisions are likely to be a bit short-sighted.
I believe that developers should build up a solid sense of “good enough” code and practice getting there without regard to pressure, so that they’ll at least know when they may have left the code worse than it needs to be. And as a manager, I’d try to instill a sense of going as fast as we safely can, but not to instill pressure that would be harmful.
The second reason for not going this far in this program is that the program is small and the firing solution is small, and so if it’s a bit procedural, a bit less factored than ideal, it’s probably OK. And, honestly, it probably is.
I don’t like either of those reasons. Your decisions are yours, and you get to decide how much sanding to do, but I want the code to be as smooth as possible, so that when I return to it, I’ll be able to figure it out quickly and accurately.
That said, once I had those tests in place, as of yesterday afternoon, I felt at that time that the code was good enough. And yet, in a short time last night, and a short time this morning, I’ve found ways to make it even better. In a more real world situation, I might not have had the opportunity, but without it, when I inevitably returned to yesterday’s code, it would slow me down compared to today’s.
Investing in refactoring is speculative: it will not pay off unless someone has to understand the code later. But in incremental development, most code will be visited again and again, and the cost of leaving something less clear because “we’ll never be back here” is pretty risky, because when we do come back, it’s going to slow us down.
Again, YMMV, but I think refactoring is generally worth the time.
Is There More Meat Here?
Let’s look at the code one more time.
def create_missile(self, ships):
should_target = random.random()
random_angle = random.random()
return self.suitable_missile(should_target, random_angle, ships)
def suitable_missile(self, should_target, random_angle, ships):
if self.cannot_target_ship(ships, should_target):
return self.missile_at_angle(random_angle * 360.0, self.velocity)
targeting_angle = self.angle_to(ships[0])
velocity_adjustment = Vector2(0, 0)
return self.missile_at_angle(targeting_angle, velocity_adjustment)
def cannot_target_ship(self, ships, should_target):
return not ships or should_target > u.SAUCER_TARGETING_FRACTION
def missile_at_angle(self, degrees, velocity_adjustment):
missile_velocity = Vector2(u.MISSILE_SPEED, 0).rotate(degrees) + velocity_adjustment
offset = Vector2(2 * self.radius, 0).rotate(degrees)
return Missile.from_saucer(self.position + offset, missile_velocity)
def angle_to(self, ship):
aiming_point = nearest_point(self.position, ship.position, u.SCREEN_SIZE)
angle_point = aiming_point - self.position
return degrees(atan2(angle_point.y, angle_point.x))
One thing that I don’t like is that hack about the velocity. Let me try to explain.
Normally when we fire a missile, it’s given the standard missile velocity in the aiming direction, and we add to that velocity the velocity of the vehicle, to get the missile’s true velocity. If the vehicle is firing forward, the missile goes faster than if it’s firing backward.
The Missile creation logic expects to be given the starting position of the missile and its true velocity. So the normal calculation looks like this:
self.missile_at_angle(random_angle * 360.0, self.velocity)
def missile_at_angle(self, degrees, velocity_adjustment):
missile_velocity = Vector2(u.MISSILE_SPEED, 0).rotate(degrees) + velocity_adjustment
offset = Vector2(2 * self.radius, 0).rotate(degrees)
return Missile.from_saucer(self.position + offset, missile_velocity)
The input velocity_adjustment, in the normal case, is the velocity of the saucer. Perhaps a better name would be good here, but I don’t know of one.
In order to hit the aiming point, we need the angle the missile travels at to be the angle between the vehicle position and the target. That’s the atan2 we see in the code. But for the missile to fly at that angle, we cannot add the vehicle’s velocity to it. Thus the “velocity adjustment”, which is either the saucer’s velocity, or zero, depending on whether we’re firing randomly or at the target.
This could be done differently, but it would require more thinking and would, I think, be a bit more complicated. We’d have to solve for the angle to fire, given the impact of the saucer’s velocity. More algebra, and the effect looks just fine as it stands.
So I live with the hack, which essentially decides whether or not to add in the saucer’s velocity.
Probably we could do that some other way, such as with a flag sent down to miszile_at_angle, or a separate method for each case.
So far, I’ve not spotted a refactoring that I prefer. Perhaps later.
But Why Do All This?
Why am I willing to spend a couple of hours and upwards of 300 lines of article on about a dozen lines of code? It’s simple:
It’s always like this!
Well, almost always. The bulk of our code improvement generally comes down to a few lines that can be made more clear, plus, depending on our tool set, some changes elsewhere to plug i the changes. If the changes elsewhere are extensive, that is a sign that our code is not well factored, and so we should improve the situation, and if they are not all that extensive, our our tool set helps us, then the improvements are also worthy.
It seems to me that almost all the refactoring I need is mostly local. So practicing om local refactoring in these easy cases pays off in the ability to do refactoring when it isn’t quite so easy. I’m more practiced and therefore better at it, and I develop confidence that empowers me to go after issues when I see them.
I do this because my craft is important to me, and in the hope that readers will find ideas that help them be more effective, more comfortable, happier, in their own work. Or, maybe I’m just good to be pointed out as a tiny fool. That’s OK too.
Better code today, and every day. See you next time!