Python 163
Working with Saucer has me thinking that scoring is unnecessarily complicated. Let’s look into that. Isn’t it amazing how much a tiny program like this one can be improved? And I didn’t even try to do a poor job.
When the Saucer is hit, it does this:
def interact_with_missile(self, missile, fleets):
self.missile_tally += missile.saucer_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, attacker):
index = 0 if self.is_small_saucer else 1
return attacker.scores_for_hitting_saucer()[index]
It asks the attacker, which could be a missile, a ship, or an asteroid, what score it gets for hitting a saucer. Ship and Asteroid return a list of two zeros, because there is no score in that case:
@staticmethod
def scores_for_hitting_saucer():
return [0, 0]
And Missile might score:
class Missile(FLyer):
def scores_for_hitting_saucer(self):
return self._saucer_score_list
The score list depends on the missile source:
class Missile(Flyer):
@classmethod
def from_saucer(cls, position, velocity):
return cls(position, velocity, [0, 0, 0], [0, 0], False)
@classmethod
def from_ship(cls, position, velocity):
return cls(position, velocity, u.MISSILE_SCORE_LIST, u.SAUCER_SCORE_LIST, True)
def __init__(self, position, velocity, missile_score_list, saucer_score_list, from_ship):
if from_ship:
self.saucer_tally = 0
self.ship_tally = 1
else:
self.saucer_tally = 1
self.ship_tally = 0
self.score_list = missile_score_list
self._timer = Timer(u.MISSILE_LIFETIME)
self._saucer_score_list = saucer_score_list
self._location = MovableLocation(position, velocity)
As you can infer from the code above, the same kind of thing is done when a missile hits an asteroid.
There is a word for this bit of design, and I believe that it is probably “wrong” or “waytoocomplicated”, if that’s a word. The game rules are these:
- If an asteroid is destroyed by a missile from the ship, it will add to the score depending on asteroid size, 20 for large, 50 for medium and 100 for small.
- If the saucer is destroyed by a missile from the ship, it will score 200 for the large saucer and 1000 for the small one.
“So”, we ask ourselves, “so, why doesn’t the saucer (or asteroid) know its score values, and just score them?”
“Well”, we reply, “well, because only a missile from the ship actually scores, not other collisions. So we have to involve the attacker.”
“Yes”, we come back, “yes, but they have no reason to know how much to score, only whether or not to score.”
“You have a point there”, we admit, “You have a point there.”
Let us design a bit here. We’ll suppose that the saucer will know its score. Perhaps the large one just knows the 200 and the small one just knows the 1000, set up at the time of creation. Then when it is hit, we could ask the attacker whether it is a ship missile and either score or not.
Or, we could pass it the score and expect it to return the score back, or a zero, depending on what it knows about itself. That would be better than returning True / False, which would require an if and would be a bit obscure. There might still be an if in the missile, but I can think of at least one tricksy way to avoid that.
Let’s sketch what the saucer code might look like:
Wait! I just noticed that the saucer knows it has been hit by a missile and only even asks about the score in that case. No other class needs even to implement the scores_for_hitting_saucer method. The other classes will never see the call. This is even more way too complicated than I realized.
I try removing one, the one in saucer, which seems quite unlikely to be needed, since there’s only ever one saucer at a time, and a test fails. At least I tested this non-feature.
Right, the test is wrong as well:
def test_everyone_supports_saucer_score_lists(self):
asteroid = Asteroid()
missile = Missile.from_ship(Vector2(100, 100), Vector2(100, 100))
saucer = Saucer.large()
ship = Ship(Vector2(200, 200))
assert asteroid.scores_for_hitting_saucer()
assert missile.scores_for_hitting_saucer()
assert saucer.scores_for_hitting_saucer()
assert ship.scores_for_hitting_saucer()
There’s a similar one for asteroid scores. Also not so good.
I think we’d best test the new idea, whatever it is, into being, and then use it.
On the saucer side, what if it goes like this:
- The saucer knows its basic score, 200 or 1000, based on its factory method.
- Upon hitting a missile, the saucer will unconditionally create a Score object, which may contain zero and will therefore have no benefit when the ScoreKeeper consumes it.
- The saucer will send a message to the missile, saying, oh,
authorize_score(score: int)and the missile will return the provided value or zero depending on whether it is a ship missile or not.
Let’s start with the Missile test.
def test_ship_missile_authorizes_saucer_score(self):
missile = Missile.from_ship(Vector2(0, 0), Vector2(0, 0))
assert missile.authorize_score(1000) == 1000
The test fails, no surprise there. Fake it till you make it:
class Missile(Flyer):
def authorize_score(self, score: int):
return score
Test passes. New test:
def test_saucer_missile_does_not_authorize_saucer_score(self):
missile = Missile.from_saucer(Vector2(0, 0), Vector2(0, 0))
assert missile.authorize_score(1000) == 0
This one fails. Now how can we help Missile sort this out?
Missile’s init looks like this:
def __init__(self, position, velocity, missile_score_list, saucer_score_list, from_ship):
if from_ship:
self.saucer_tally = 0
self.ship_tally = 1
else:
self.saucer_tally = 1
self.ship_tally = 0
self.score_list = missile_score_list
self._timer = Timer(u.MISSILE_LIFETIME)
self._saucer_score_list = saucer_score_list
self._location = MovableLocation(position, velocity)
Let’s do this:
def __init__(self, position, velocity, missile_score_list, saucer_score_list, from_ship):
if from_ship:
self.saucer_tally = 0
self.ship_tally = 1
self._authorization_factor = 1
else:
self.saucer_tally = 1
self.ship_tally = 0
self._authorization_factor = 0
self.score_list = missile_score_list
self._timer = Timer(u.MISSILE_LIFETIME)
self._saucer_score_list = saucer_score_list
self._location = MovableLocation(position, velocity)
def authorize_score(self, score: int):
return self._authorization_factor * score
Test is green. Is this too obscure, too tricksy, multiplying the incoming score by zero or one? One might object a bit but the only alternatives I see are less desirable:
A straightforward thing would be a boolean in the constructor and an if statement in authorize_score. I’d rather avoid the if.
A more clever thing would be to put one of two functions into a member, rather than the multiplier. Let me commit this code and show you. Commit: missile knows authorize_score.
We could do this:
def __init__(self, position, velocity, missile_score_list, saucer_score_list, from_ship):
if from_ship:
self.saucer_tally = 0
self.ship_tally = 1
self._authorize = lambda score: score
else:
self.saucer_tally = 1
self.ship_tally = 0
self._authorize = lambda score: 0
self.score_list = missile_score_list
self._timer = Timer(u.MISSILE_LIFETIME)
self._saucer_score_list = saucer_score_list
self._location = MovableLocation(position, velocity)
def authorize_score(self, score: int):
return self._authorize(score)
I actually think I like that and will let it stand. Let me point out that we could do something even more weird and store the lambda directly into self.authorize_score and it would work just fine. But PyCharm would know that there was no method by that name at compile time and it would warn us. And rightly so.
Even this is of course a bit risky: what if _authorize isn’t initialized? Then scoring will blow up. But it would blow up if the integer wasn’t initialized as well.
And traditional if statement would not blow up, since in Python everything is sufficiently boolean, but it would still be wrong if a boolean were not initialized.
I’m going to allow this. If you object, let’s discuss it on Mastodon. If you have an even better idea, let’s hear it!
Commit: use lambdas in authorize_score.
Now we are ready to change Saucer. I have at least one existing test for saucers vs missiles, so let’s just go ahead and change Saucer and see if it breaks them.
def small(cls):
return cls(radius=10, score=1000, sound="saucer_small", is_small=True, always_target=True, scale=4)
@classmethod
def large(cls):
return cls(radius=20, score=200, sound="saucer_big", is_small=False, always_target=False, scale=8)
def __init__(self, radius, score, sound, is_small, always_target, scale):
Saucer.direction = -Saucer.direction
x = 0 if Saucer.direction > 0 else u.SCREEN_SIZE
position = Vector2(x, random.randrange(0, u.SCREEN_SIZE))
velocity = Saucer.direction * u.SAUCER_VELOCITY
self._directions = (velocity.rotate(45), velocity, velocity, velocity.rotate(-45))
self._gunner = Gunner(always_target)
self._location = MovableLocation(position, velocity)
self._radius = radius
self._score = score
self._ship = None
self._sound = sound
self._zig_timer = Timer(u.SAUCER_ZIG_TIME)
self.is_small_saucer = is_small
self.missile_tally = 0
self.missile_head_start = 2*self._radius
self.create_surface_class_members(scale)
Now the saucer knows its score. Let’s first implement the scoring to be wrong
def score_for_hitting(self, attacker):
return 2
That breaks three tests.
def score_for_hitting(self, missile):
return missile.authorize_score(self._score)
And we are green. I cannot resist trying to shoot a saucer in the actual game. Works as advertised. Commit: saucer now uses authorize logic for scoring.
Now we can remove a whole raft of code supporting the old way.
All the methods about scores_for_hitting_saucer can go. The values in u can go, and the references in Missile:
@classmethod
def from_saucer(cls, position, velocity):
return cls(position, velocity, [0, 0, 0], False)
@classmethod
def from_ship(cls, position, velocity):
return cls(position, velocity, u.MISSILE_SCORE_LIST, True)
def __init__(self, position, velocity, missile_score_list, from_ship):
if from_ship:
self.saucer_tally = 0
self.ship_tally = 1
self._authorize = lambda score: score
else:
self.saucer_tally = 1
self.ship_tally = 0
self._authorize = lambda score: 0
self.score_list = missile_score_list
self._timer = Timer(u.MISSILE_LIFETIME)
self._location = MovableLocation(position, velocity)
I think that’s got it all. Commit: remove unused old saucer score tables and references.
We can do the same with regard to asteroid scores, and we will, but let’s sum up for now.
Summary
I’m not sure what triggered me to recognize that scoring was more complicated than it needs to be, but I think it came up in my thinking about some things that Bill Wake said to me on Mastodon, about not passing booleans into constructors and something that Bruce Onder said:
Also, why not init the score based on the size? Then when a saucer is killed, it can copy its own score value into the score object and be done with it.
Certainly Bruce’s idea is at the core of what’s done here, and I know it was Bill who got me thinking about the smarter constructors. Thanks to them both!
Prior to this change, the Missile had a list of scores for hitting a saucer, containing two values based on the saucer size, which the missile had no business knowing about. In fact, it really had no business knowing scores at all, although it does need to somehow reflect whether it is a ship missile or a saucer missile.
Now, the Saucer knows its own score and does not even know the score of the other size of saucer. It asks the missile to “authorize” the score, which the missile does by either returning whatever value was passed to it, or zero. In either case, the saucer adds a Score instance of that value, and the score accrues as it should.
There’s still just one interaction with the missile relating to scoring; that interaction is simpler, returning a single value; and the missile no longer has a list of scores, for a small but notable simplification.
We did dig a bit deeply into the bag of tricks when we provided the missile with a member variable _authorize(score) which holds a lambda returning either score or zero.
We could have had two additional methods in Missile, perhaps authorize_zero and authorize_provided, and set _authorize to one of those, but I don’t see any reason to prefer that scheme over the lambda, which is a perfectly good Python construct.
I think this is just a bit better and overall a bit more clear, and I like it. What do you think? Engage me on Mastodon if you care to.
See you next time!