Python 166
Zoom report. I notice a thing. What is to be done about it? Something nice. Repeating the Faulkner quote from yesterday.
Last night’s Zoom was nigh on to desultory, but we did discuss some interesting notions. Central to them was the observation, initiated by GeePaw, whom I generally call “Hill” for historical reasons. He observed that the things we talk about are generally actually important, and that we all tend to agree on the issue, even if we do not agree on exactly how to handle it. There’s a lot of “I do this when that happens, you do that when it happens”, but very little “no, that’s not an issue”. When someone points out Feature Envy, no one says “no it isn’t”. When someone points to a method that isn’t cohesive, no one argues that it’s just fine. When someone points to a long method, no one argues that it should stay that long.
We do not always agree on whether to make an improvement, and we don’t always agree on the improvement to make. Quite often, we’ll try a few different ways to see what feels best. Often, we settle on one. Sometimes, rarely, we do not.
We are, to put it kindly, “mature”. In less “mature” teams we often see arguments about whether 25 lines is the limit to method size, or whether it should be 50, or whether this particular 300 line function is just fine as it stands. We literally never do that.
We believe, of course, that it’s a good thing when a group has a sufficient understanding of programming, and sufficient trust in each other, that when someone says “that looks odd”, we don’t defend it, we gather round to see what would make it better.
At a more detailed level, we looked at my Missile object, which had been accused in the past of trying to be two classes, SaucerMissile and ShipMissile, glommed into one. I actually tried it that way a while back, didn’t like the effect, and put it back.
In the prior single-class implementation, there was essentially a boolean in Missile saying whether it was, or was not, a missile from the ship. (See me trying to avoid saying “is a ship missile”? That would be a dead giveaway that it’s more than one kind of thing.)
In the current scheme, there are two new features on Missile. One is that it has a dynamic method. confirm_score. The method / function is defined at creation time:
class Missile(Flyer):
confirm_score: Callable[[int], int]
@classmethod
def from_saucer(cls, transponder_key, position, velocity):
return cls(transponder_key, position, velocity, [0, 0, 0],
confirmation=lambda score: 0)
@classmethod
def from_ship(cls, transponder_key, position, velocity):
return cls(transponder_key, position, velocity, u.ASTEROID_SCORE_LIST,
confirmation=lambda score: score)
def __init__(self, transponder_key, position, velocity, missile_score_list, confirmation):
self._transponder = Transponder(transponder_key)
self.confirm_score = confirmation
self.score_list = missile_score_list
self._timer = Timer(u.MISSILE_LIFETIME)
self._location = MovableLocation(position, velocity)
You can see that we paste in one of two small lambda functions, receiving score and either returning it, in the “ship” has, or returning zero in the “saucer” case. The upshot of that dynamic method is that missiles from ship score points and missiles from saucer do not. We do that like this:
class Saucer(Flyer):
def score_for_hitting(self, missile):
return missile.confirm_score(self._score)
Is that still a class waiting to be born? Possibly. And a dynamically-created method is rather deep in the bag of tricks. Legitimate but might be too clever.
The other new feature is the Transponder. When a missile is created, its creator provides a transponder_key, which is burned into the ROM of the missile’s transponder. When, later, we ping the missile, if the key we provide matches the transponder key, the missile executes the command we provided. It goes like this:
class Saucer(Flyer):
def interact_with_missile(self, missile, fleets):
missile.ping_transponder("saucer", self.increment_tally)
if missile.are_we_colliding(self.position, self._radius):
fleets.append(Score(self.score_for_hitting(missile)))
self.explode(fleets)
If the missile we ping is from the saucer, we increment our tally, otherwise we do not, because the ping will only have effect if the transponder’s key is “saucer”.
Are those two thinly-disguised checks signals that Missile might better be represented in two classes rather than one? It definitely whiffs of that code smell, doesn’t it?
Back to the Zoom Ensemble. We all sniff that scent of two classes in one. We may differ on what to do about it. Missile has about a dozen small methods and properties. Exactly one of those differs between a missile from the saucer and one from the ship: the confirm_score dynamic method.
What if there were zero differences? We can make that happen, I think. Remember that we have this:
class Saucer(Flyer):
def interact_with_missile(self, missile, fleets):
missile.ping_transponder("saucer", self.increment_tally)
if missile.are_we_colliding(self.position, self._radius):
fleets.append(Score(self.score_for_hitting(missile)))
self.explode(fleets)
def score_for_hitting(self, missile):
return missile.confirm_score(self._score)
Could we use the transponder to do this job for us? I think we could. Let’s try it.
Extract Method:
def interact_with_missile(self, missile, fleets):
missile.ping_transponder("saucer", self.increment_tally)
if missile.are_we_colliding(self.position, self._radius):
self.score_points(fleets, missile)
self.explode(fleets)
def score_points(self, fleets, missile):
fleets.append(Score(self.score_for_hitting(missile)))
Hm. Transponder doesn’t take args. I really want to say this:
def interact_with_missile(self, missile, fleets):
missile.ping_transponder("saucer", self.increment_tally)
if missile.are_we_colliding(self.position, self._radius):
missile.ping_transponder("ship", self.score_points, fleets)
self.explode(fleets)
def score_points(self, fleets):
fleets.append(Score(self._score))
Transponder needs to be smarter, as does Missile’s ping method:
class Missile(Flyer):
def ping_transponder(self, transponder_key, function, *args):
self._transponder.ping(transponder_key, function, *args)
class Transponder:
def ping(self, key, function, *args):
if key == self._key:
function(* args)
And my tests are green. I can’t remove that confirm_score code yet, because we use it elsewhere. Fix that up to use our new scheme. In Asteroid, we have:
class Asteroid(Flyer):
def score_and_split(self, missile, fleets):
fleets.append(Score(self.score_for_hitting(missile)))
self.split_or_die(fleets)
def score_for_hitting(self, missile):
return missile.confirm_score(self._score)
As before, extract:
def score_and_split(self, missile, fleets):
self.score_points(missile, fleets)
self.split_or_die(fleets)
def score_points(self, missile, fleets):
fleets.append(Score(self.score_for_hitting(missile)))
Plug in _score, because we’re going to conditionally apply the append operation:
def score_and_split(self, missile, fleets):
self.score_points(missile, fleets)
self.split_or_die(fleets)
def score_points(self, missile, fleets):
fleets.append(Score(self._score))
Change signature since missile is unused.
def score_points(self, fleets):
fleets.append(Score(self._score))
def split_or_die_on_collision(self, collider, fleets):
if collider.are_we_colliding(self.position, self.radius):
self.split_or_die(fleets)
Use transponder:
def score_and_split(self, missile: Missile, fleets):
missile.ping_transponder("ship", self.score_points, fleets)
self.split_or_die(fleets)
def score_points(self, fleets):
fleets.append(Score(self._score))
We have tests for the confirm_score, which we are removing. Remove those two little tests. Remove the confirmation from Missile creation:
class Missile(Flyer):
Saucer = None
radius = 2
@classmethod
def from_saucer(cls, transponder_key, position, velocity):
return cls(transponder_key, position, velocity, [0, 0, 0])
@classmethod
def from_ship(cls, transponder_key, position, velocity):
return cls(transponder_key, position, velocity, u.ASTEROID_SCORE_LIST)
def __init__(self, transponder_key, position, velocity, missile_score_list):
self._transponder = Transponder(transponder_key)
self.score_list = missile_score_list
self._timer = Timer(u.MISSILE_LIFETIME)
self._location = MovableLocation(position, velocity)
We are green. Commit: use transponder to score missiles. Remove confirm_score.
Discussion
There is now only one difference between missiles from the Saucer and those from the Ship: their Transponder instances have different keys. There are no conditional checks anywhere for type, and there is exactly one check for key equality, in Transponder.
And we have removed the somewhat tricky callable member confirm_score, which amounted to a dynamic method, different for saucer and for ship.
The cost was that we added *args to the Transponder, which on another day with another first application, we might have done off the bat.
Wait!
Hm, what about that scoring parameter that I see up there in the class methods? I don’t think we actually use it any more.
There is a test that checks it. We don’t refer to it anywhere in the game code: asteroids know their own scores now.
Change Signature:
@classmethod
def from_saucer(cls, transponder_key, position, velocity):
return cls(transponder_key, position, velocity)
@classmethod
def from_ship(cls, transponder_key, position, velocity):
return cls(transponder_key, position, velocity)
def __init__(self, transponder_key, position, velocity):
self._transponder = Transponder(transponder_key)
self._timer = Timer(u.MISSILE_LIFETIME)
self._location = MovableLocation(position, velocity)
Now those class methods look mysteriously similar. We no longer need them. We can inline: will PyCharm “just do it”? No, can’t inline functions with decorators. Let me try removing the decorator. First commit: remove unused score list from constructors.
I can’t convince PyCharm to inline this so I’ll just do it the hard way.
class Missile(Flyer):
Saucer = None
radius = 2
def __init__(self, transponder_key, position, velocity):
self._transponder = Transponder(transponder_key)
self._timer = Timer(u.MISSILE_LIFETIME)
self._location = MovableLocation(position, velocity)
Commit: remove class method constructors, no longer needed.
As we were saying …
The Missile class is now much simpler and no longer needs type-suggesting constructors.
There is now no distinction at all between ship missiles and saucer missiles in the Missile class. The only distinction is that each missile carries a Transponder that responds to messages addressed to the key of that Transponder and we just happen to use “ship” on missiles fired by the ship and “saucer” on those fired by the saucer.
Ship and Saucer use their respective keys to count how many of their missiles are still flying. Asteroid and Saucer use the “ship” key to decide whether to give score points to the player.
Are we still distinguishing two kind of Missile? Absolutely. Is what we have enough of an indication that there should be two classes of missile? I would say that it is not. I do not know what my betters, peers, and colleagues would say. Since two discrete Missile classes would duplicate about a dozen methods, I think this is a fine solution.
Which brings me back …
to the discussion last night. Hill pointed out last night that the Transponder is a lot like a Strategy pattern, and it is. A difference is that we usually expect to see either a separate function (as with the confirm_score, which is also like Strategy), or an instance of separate classes with Strategy, and here we have an instance of the same class, differing only by data value.
Half of one, six a dozen of the other. It is like Strategy, and it is what it is.
And I approve of it. It has isolated the difference between kinds of missiles into a single tiny object and that’s good enough for me.
We generally agree on the observations about things … and we sometimes have different preferences for how we would resolve issues. One might split a class into two similar ones, like SaucerMissile and ShipMissile. Another might plug in some behavior that would handle variance, like confirm_score. Another might plug in a small object to deal with variance, like Transponder.
It’s all good. There are few absolute answers in our work. The thing is to always be thinking, and to be willing to push the code in a few different directions, perhaps over several sessions, until our discomfort is minimized. As we practice … and practice and practice … and as we work with our colleagues … we learn to create code that keeps us comfortable, and to change code in small steps toward more comfort.
It’s fun, it feels good, it keeps our brains alive and happy … and we get better and better code rather than deteriorating code. It’s a good thing.
I want to close with the William Faulkner quote I footnoted yesterday:
Get it down. Take chances. It may be bad, but it’s the only way you can do anything really good.
See you next time!