Python 131 - Grumble
I am slightly irritated by the fact that my lovely Gunner object would really benefit from an additional change.
Gunner is a bit odd. When we create one, we have the option of passing in the saucer’s radius:
class Gunner:
def __init__(self, saucer_radius=20):
self._timer = Timer(u.SAUCER_MISSILE_DELAY)
self._radius = saucer_radius
And when we ask it to fire a missile, we pass it mass quantities of information, including the saucer position and its velocity:
def fire(
self,
delta_time,
missile_tally,
saucer_position,
saucer_velocity,
ship_or_none: Ship | None,
fleets):
I was really planning to be done with this for a while, but last night I was reading about the smaller saucer in the original game, which is supposed to start appearing at score 3000. In addition to being smaller, it fires only targeted shots.
Anyway, as I thought about this code, I came to believe that instead of passing in three separate facts about the saucer, we should simply pass in the saucer. The Gunner is a friend, and the radius, position, and velocity are information that a saucer’s gunner would have access to.
So we need to change the calling sequence here.
I’ll begin by trying PyCharm’s Change Signature and see what it does for us. If it does reasonably well, we’ll go from there. If not … we’ll try something else.
The change breaks five tests, and some code inside the Gunner. Fix that first.
def fire(self, delta_time, missile_tally, saucer, ship_or_none: Ship | None, fleets):
if ship_or_none:
ship_position = ship_or_none.position
else:
ship_position = self.random_position()
self._timer.tick(delta_time, self.fire_missile, missile_tally, saucer.position, saucer.velocity, ship_position, fleets)
Now let’s look at the tests.
Some simply need a saucer to pass in. These three, for example:
def test_no_fire_on_short_time(self):
delta_time = 0.1
ship_position = Vector2(1, 1)
ship = Ship(ship_position)
fleets = Fleets()
Gunner().fire(delta_time, 0, Saucer(), ship, fleets)
assert not FI(fleets).saucer_missiles
def test_fire_on_time(self):
delta_time = u.SAUCER_MISSILE_DELAY
ship_position = Vector2(1, 1)
ship = Ship(ship_position)
fleets = Fleets()
Gunner().fire(delta_time, 0, Saucer(), ship, fleets)
assert FI(fleets).saucer_missiles
def test_handle_ship_none(self):
delta_time = 1.0
tally = 0
fleets = Fleets()
fi = FI(fleets)
ship = None
Gunner().fire(delta_time, tally, Saucer(), ship, fleets)
assert fi.saucer_missiles
I was prompted by PyCharm to notice that saucer had its velocity marked private, so I renamed that from _velocity to velocity.
Carrying on with failing tests, the remaining two are fixed by causing the Saucer to pass itself to fire:
class Saucer(Flyer):
def fire_if_possible(self, delta_time, fleets):
self._gunner.fire(delta_time, self._missile_tally, self, self._ship, fleets)
Looking at that, two things come to mind. First, if we chose to make _missile_tally public, we could avoid passing it. Second, if we are going to pass it, it makes more sense to pass it after the saucer parameter rather than before.
We need not pass it. We’ve made the commitment that the Gunner is part of the Saucer and can access its members.
We’ll make _missile_tally public. Curiously, doing so breaks a test involving ship, where PyCharm mistakenly renamed the member. Tests are now green, after I put that back the way it was.
How here, we can change the code to access the tally, in preparation for removing it from the fire sequence:
def fire(self, delta_time, missile_tally, saucer, ship_or_none: Ship | None, fleets):
if ship_or_none:
ship_position = ship_or_none.position
else:
ship_position = self.random_position()
self._timer.tick(delta_time, self.fire_missile, missile_tally, saucer.position, saucer.velocity, ship_position, fleets)
I see that we should push saucer down a bit further. We’ll wait until we have tally brought in. The following passes all the tests:
def fire(self, delta_time, missile_tally, saucer, ship_or_none: Ship | None, fleets):
if ship_or_none:
ship_position = ship_or_none.position
else:
ship_position = self.random_position()
self._timer.tick(delta_time, self.fire_missile, saucer.missile_tally, saucer.position, saucer.velocity, ship_position, fleets)
And let’s change signature again.
def fire(self, delta_time, saucer, ship_or_none: Ship | None, fleets):
if ship_or_none:
ship_position = ship_or_none.position
else:
ship_position = self.random_position()
self._timer.tick(delta_time, self.fire_missile, saucer.missile_tally, saucer.position, saucer.velocity, ship_position, fleets)
Now for the parameters to fire_missile, we need to change the list in the timer tick above, but also of course the fire_missile method. We’ll try Change Signature there.
def fire_missile(self, saucer, ship_position, fleets):
if missile_tally >= u.SAUCER_MISSILE_LIMIT:
return
should_target = random.random()
self.select_missile(fleets, saucer_position, saucer_velocity, ship_position, should_target)
This is now rife with squiggles, so we fix up the missing saucer.s:
def fire_missile(self, saucer, ship_position, fleets):
if saucer.missile_tally >= u.SAUCER_MISSILE_LIMIT:
return
should_target = random.random()
self.select_missile(fleets, saucer.position, saucer.velocity, ship_position, should_target)
This breaks tests. Oh, I didn’t fix up the call yet.
def fire(self, delta_time, saucer, ship_or_none: Ship | None, fleets):
if ship_or_none:
ship_position = ship_or_none.position
else:
ship_position = self.random_position()
self._timer.tick(delta_time, self.fire_missile, saucer, ship_position, fleets)
Still one test broken, probably someone calling fire_missile directly.
def test_can_only_fire_limited_number(self):
no_target = 0.5
angle = 0.0
fleets = Fleets()
fi = FI(fleets)
saucer_position = Vector2(500, 500)
velocity = Vector2(0, 0)
ship_position = Vector2(0, 0)
count = u.SAUCER_MISSILE_LIMIT
Gunner().fire_missile(None, ship_position, fleets)
assert not fi.saucer_missiles
Right. That’s readily fixed:
def test_can_only_fire_limited_number(self):
no_target = 0.5
angle = 0.0
fleets = Fleets()
fi = FI(fleets)
saucer_position = Vector2(500, 500)
velocity = Vector2(0, 0)
ship_position = Vector2(0, 0)
saucer = Saucer()
saucer._location.position = saucer_position
saucer._location.velocity = velocity
saucer.missile_tally = u.SAUCER_MISSILE_LIMIT
Gunner().fire_missile(saucer, ship_position, fleets)
assert not fi.saucer_missiles
And we are green. We should be committing this, shouldn’t we?
Commit: modifying Saucer-Gunner to pass saucer to gunner, not individual bits.
We can push saucer a bit deeper:
def fire_missile(self, saucer, ship_position, fleets):
if saucer.missile_tally >= u.SAUCER_MISSILE_LIMIT:
return
should_target = random.random()
self.select_missile(fleets, saucer.position, saucer.velocity, ship_position, should_target)
def select_missile(self, fleets, saucer_position, saucer_velocity, ship_position, should_target):
if ship_position and should_target <= u.SAUCER_TARGETING_FRACTION:
self.create_targeted_missile(saucer_position, ship_position, fleets)
else:
random_angle = random.random()
self.create_random_missile(random_angle, saucer_position, saucer_velocity, fleets)
def create_random_missile(self, random_angle, saucer_position, saucer_velocity, fleets):
missile = self.missile_at_angle(saucer_position, random_angle*360.0, saucer_velocity)
fleets.add_flyer(missile)
You could argue either way on whether to pass saucer or the individual members, but I think that now that we’ve committed to passing the saucer, we should stick to that notion.
So we do select_missile with another Change Signature.
def select_missile(self, fleets, saucer, ship_position, should_target):
if ship_position and should_target <= u.SAUCER_TARGETING_FRACTION:
self.create_targeted_missile(saucer.position, ship_position, fleets)
else:
random_angle = random.random()
self.create_random_missile(random_angle, saucer.position, saucer.velocity, fleets)
I’m electing to do this one method at a time. Also: commit: pushing saucer parameter down as far as it will go.
Next, what about create_targeted_missile. It is really a function that only wants to think about positions:
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)
I think we’ll let this one stay as it is, since it is really just about positions. What about create_random_missile?
def select_missile(self, fleets, saucer, ship_position, should_target):
if ship_position and should_target <= u.SAUCER_TARGETING_FRACTION:
self.create_targeted_missile(saucer.position, ship_position, fleets)
else:
random_angle = random.random()
self.create_random_missile(random_angle, saucer.position, saucer.velocity, fleets)
def create_random_missile(self, random_angle, saucer_position, saucer_velocity, fleets):
missile = self.missile_at_angle(saucer_position, random_angle*360.0, saucer_velocity)
fleets.add_flyer(missile)
I think we’ll draw the line where it is now. And there’s something to think about.
Reflection
Passing the saucer instead of three of its properties simplified our Gunner object quite a bit, and … oh, what about that radius that we pass in?
Push down a reflection level
class Gunner:
def __init__(self, saucer_radius=20):
self._timer = Timer(u.SAUCER_MISSILE_DELAY)
self._radius = saucer_radius
Couldn’t we drop that parameter and just access the saucer’s radius? Where do we use that? Oh, this is irritating:
def missile_at_angle(self, position, desired_angle, velocity_adjustment):
missile_velocity = Vector2(u.MISSILE_SPEED, 0).rotate(desired_angle) + velocity_adjustment
offset = Vector2(2 * self._radius, 0).rotate(desired_angle)
return Missile.from_saucer(position + offset, missile_velocity)
That’s way down below where we have access to the saucer. I am tempted to do this differently. I am tempted to pass the saucer on creation and just pass the ship and such during firing.
The most irritating thing about this idea is that I’ve just done all this nice refactoring, for almost an hour, and now I may have to do it again. But there is a decent argument against Gunner owning the Saucer: Saucer owns the Gunner.
As things stand Gunner is passed a pointer to the saucer during firing. It’s a parameter that it is allowed to make calls on. If we pass it as part of the init and save it, we have created a permanent two-way link between saucer and gunner and back to saucer.
We sort of have that link anyway, with the radius being passed in. But a two-way circular reference … I just have a tendency to avoid those. Too many odd things can happen.
Looking at the code, I think we’ll leave this alone. It’s not exactly right, but I don’t see a good way to make it more right, the way the calls to missile_at_angle go:
def select_missile(self, fleets, saucer, ship_position, should_target):
if ship_position and should_target <= u.SAUCER_TARGETING_FRACTION:
self.create_targeted_missile(saucer.position, ship_position, fleets)
else:
random_angle = random.random()
self.create_random_missile(random_angle, saucer.position, saucer.velocity, fleets)
def create_random_missile(self, random_angle, saucer_position, saucer_velocity, fleets):
missile = self.missile_at_angle(saucer_position, random_angle*360.0, saucer_velocity)
fleets.add_flyer(missile)
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)
We’re kind of sneaking the radius parameter into missile_at_angle by keeping it in a member. That’s not really good design but I don’t see an easy way to improve it.
Pop up a reflection level
But that brings me back to what I was saying … we have simplified Gunner quite a bit by passing in the saucer … but there is a fairly clear line inside Gunner where something happens. Up near the top, we pass the saucer around. Then we stop passing the saucer and from there on down, we are passing points, velocities, radii … relatively raw “numerical” values.
In the deeper levels, it is as if we have extracted the values we need and now are just running relatively low-level calculations on those numbers. We no longer care about saucer and ship, it’s all about positions and velocities.
If we think about responsibilities, the upper methods have responsibilities about the saucer, checking its missile tally, making strategic decisions about whether to target or fire randomly … and then the deeper methods are responsible for decisions about numbers.
This shift in responsibility is a signal. It is a sign that there may be two objects coming into being here, one for strategy and one for numerical work.
We’ll not do anything about that right now, but it is well worth considering. We might have some kind of subordinate targeting computer that returns the angle and velocity at which to fire the missile and leave the firing higher up. Right now, it’s at the very bottom, and that might not be right.
Connection to General Principles
Here in the Gunner, which isn’t all that large at 80-ish lines, it may not make much difference. But when we see an object that processes different kinds of information in different parts of its code, that’s often a sign that there is another object waiting to be born, or that this object should split like an amoeba into two or more objects.
The result of this kind of thinking, of course, is that we tend to wind up with lots of little objects. If a developer is used to highly procedural code and large objects, this kind of design can be confusing. It’s hard to see how things work, because it is hard to follow the procedural flow.
But there’s a value to this kind of design. When separation of concerns is done well, we generally do not need to follow the procedural flow.
Take the Gunner as an example. If we’re trying to figure out Saucer, a week or so ago we would have seen a half-dozen methods about firing, and we would have to at least consider them long enough to be sure they didn’t relate to whatever we were doing with Saucer. Now, all those methods are over in Gunner and if we don’t care about shooting, we don’t even look there, and if we do care about shooting, that’s the only place we have to look.
We call that “information hiding”, and “separation of concerns”, and done well, it simplifies our work by limiting how much code we have to consider when making the changes we’re trying to make.
With Gunner we have already made one large and complicated object into two smaller less complicated objects, and if we do find another object inside Gunner, we’ll take one slightly complicated object and break it into two even less complicated objects.
And that is generally a good thing, at least by my lights. Your lights may differ.
Summary
Users of Gunner—and Gunner—itself are now simpler, because we pass a Saucer to Gunner instead of several saucer components. I think that’s better.
What do you think? Let me know, if you care to. Or ask questions. I’m ronjeffries on mastodon.social.
See you next time!