Python 164
Let’s continue the scoring improvements, with a little help from our friends.
Tomas Aschan and I had a couple of exchanges over Mastodon and he suggested “confirm” as a better word than “authorize” for my function that, well, confirms the scoring of a missile against a Saucer. He also suggested a way to declare a method that will be overridden without upsetting PyCharm, so we’ll try both of those ideas now.
First, rename this:
def authorize_score(self, score: int):
return self._authorize(score)
To this:
def confirm_score(self, score: int):
return self._authorize(score)
Tomas’ suggestion for getting around the extra function was to declare the method as a callable field, and set it in __init__. We’ll try that.
class Missile(Flyer):
confirm_score: Callable[[int], int]
def __init__(self, position, velocity, missile_score_list, from_ship):
if from_ship:
self.saucer_tally = 0
self.ship_tally = 1
self.confirm_score = lambda score: score
else:
self.saucer_tally = 1
self.ship_tally = 0
self.confirm_score = lambda score: 0
self.score_list = missile_score_list
self._timer = Timer(u.MISSILE_LIFETIME)
self._location = MovableLocation(position, velocity)
That works fine. Better would be to pass in the confirmation in the factory methods:
@classmethod
def from_saucer(cls, position, velocity):
return cls(position, velocity, [0, 0, 0], lambda score: 0, False)
@classmethod
def from_ship(cls, position, velocity):
return cls(position, velocity, u.MISSILE_SCORE_LIST, lambda score: score, True)
def __init__(self, position, velocity, missile_score_list, confirmation, from_ship):
self.confirm_score = confirmation
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._location = MovableLocation(position, velocity)
One test fails. I suspect it is creating a missile directly. Yes. Not worth showing, I just changed the test to use from_ship instead of creating the Missile with its basic __init__.
Is there a way to outlaw a direct creation of a Python object? I haven’t found one that I actually like. We’ll not worry about it: if someone wants to create one the hard way that’ll be fine with me.
Let’s commit these small changes. Commit: rename authorize to confirm, define method directly using lambdas from constructor methods.
And now let’s do the same change for asteroids. Asteroids will have to know their score, as does the Saucer.
I think I know well enough how to do this that I’ll just change the main scoring line in Asteroid and work until the tests all run again.
class Asteroid(Flyer):
def score_for_hitting(self, attacker):
return attacker.scores_for_hitting_asteroid()[self.size]
That will want to become, temporarily:
def score_for_hitting(self, missile):
score = u.ASTEROID_SCORE_LIST[self.size]
return missile.confirm_score(score)
A test breaks. Good, though I was hoping for more than one.
Oh, the issue is that we do not distinguish type when we collide, we’re relying on all the objects implementing the old scores_for_... method:
def interact_with_asteroid(self, asteroid, fleets):
pass
def interact_with_missile(self, missile, fleets):
self.split_or_die_on_collision(fleets, missile)
def interact_with_saucer(self, saucer, fleets):
self.split_or_die_on_collision(fleets, saucer)
def interact_with_ship(self, ship, fleets):
self.split_or_die_on_collision(fleets, ship)
def split_or_die_on_collision(self, fleets, collider):
if collider.are_we_colliding(self.position, self.radius):
fleets.append(Score(self.score_for_hitting(collider)))
self.split_or_die(fleets)
In our new scheme we do not want to attempt scoring at all when an asteroid hits anything but a Missile. Therefore:
def interact_with_missile(self, missile, fleets):
self.score_missile_collision(fleets, missile)
def score_missile_collision(self, fleets, missile):
fleets.append(Score(self.score_for_hitting(missile)))
self.split_or_die_on_collision(fleets, missile)
def split_or_die_on_collision(self, fleets, collider):
if collider.are_we_colliding(self.position, self.radius):
self.split_or_die(fleets)
We are green. I didn’t see as many fails as I would like. I’ll run the game and then break things.
Oh how silly. I’m not checking for a collision which means that ship missiles score all the time while flying.
Shall I try to fix it, or should I roll back? I should roll back but let’s think first.
This gets me to green and should fix the problem, but I think I need some tests around this.
def interact_with_missile(self, missile, fleets):
if missile.are_we_colliding(self.position, self.radius):
self.score_missile_collision(fleets, missile)
That’s good but I do not trust the tests, since they didn’t find that rather odd defect. Let’s break scoring and see what breaks.
def score_for_hitting(self, missile):
# score = u.ASTEROID_SCORE_LIST[self.size]
score = 33
return missile.confirm_score(score)
This breaks a delightful 6 tests. A quick review finds this one, which seems quite a good check:
def test_missile_vs_asteroid_scoring(self):
fleets = Fleets()
fi = FI(fleets)
pos = Vector2(100, 100)
vel = Vector2(0, 0)
asteroid = Asteroid(2, pos)
missile = Missile.from_ship(pos, vel)
asteroid.interact_with_missile(missile, fleets)
scores = fi.scores
assert scores[0].score == u.ASTEROID_SCORE_LIST[asteroid.size]
asteroid.size = 1
asteroid.interact_with_missile(missile, fleets)
scores = fi.scores
assert scores[1].score == u.ASTEROID_SCORE_LIST[asteroid.size]
asteroid.size = 0
asteroid.interact_with_missile(missile, fleets)
scores = fi.scores
assert scores[2].score == u.ASTEROID_SCORE_LIST[asteroid.size]
Let’s set the Asteroid back as we had it and commit, then improve things:
def score_for_hitting(self, missile):
score = u.ASTEROID_SCORE_LIST[self.size]
return missile.confirm_score(score)
We could continue to look the score up here. Or we could set it in __init__. That won’t save us any time, in fact it’ll be slightly more costly since some asteroids never get shot by missiles, but it’ll be a bit more clear here and no less clear in __init__.
class Asteroid(Flyer):
def __init__(self, size=2, position=None):
self.size = size
if self.size not in [0, 1, 2]:
self.size = 2
self._score = u.ASTEROID_SCORE_LIST[self.size]
self.radius = [16, 32, 64][self.size]
position = position if position is not None else Vector2(0, random.randrange(0, u.SCREEN_SIZE))
angle_of_travel = random.randint(0, 360)
velocity = u.ASTEROID_SPEED.rotate(angle_of_travel)
self._location = MovableLocation(position, velocity)
self._offset = Vector2(self.radius, self.radius)
self._surface = SurfaceMaker.asteroid_surface(self.radius * 2)
def score_for_hitting(self, missile):
return missile.confirm_score(self._score)
This breaks the test shown above, because it is just setting size into an existing asteroid, not creating a new one. Fix that:
def test_missile_vs_asteroid_scoring(self):
fleets = Fleets()
fi = FI(fleets)
pos = Vector2(100, 100)
vel = Vector2(0, 0)
asteroid = Asteroid(2, pos)
missile = Missile.from_ship(pos, vel)
asteroid.interact_with_missile(missile, fleets)
scores = fi.scores
assert scores[0].score == u.ASTEROID_SCORE_LIST[asteroid.size]
asteroid = Asteroid(1, pos)
asteroid.interact_with_missile(missile, fleets)
scores = fi.scores
assert scores[1].score == u.ASTEROID_SCORE_LIST[asteroid.size]
asteroid = Asteroid(0, pos)
asteroid.interact_with_missile(missile, fleets)
scores = fi.scores
assert scores[2].score == u.ASTEROID_SCORE_LIST[asteroid.size]
Weak test to begin with, since it assumes too much about how the code works. Better now.
Let’s see about a test that assures us that missiles aren’t just flying around scoring.
def test_missile_only_scores_on_hit(self):
# yes there was such a defect for a moment
fleets = Fleets()
fi = FI(fleets)
ship_pos = Vector2(100, 100)
asteroid_pos = Vector2(900, 900)
vel = Vector2(0, 0)
asteroid = Asteroid(2, asteroid_pos)
missile = Missile.from_ship(ship_pos, vel)
asteroid.interact_with_missile(missile, fleets)
assert not fi.scores
That passes, because I fixed the defect. Let’s unfix it to see it fail.
def interact_with_missile(self, missile, fleets):
if True or missile.are_we_colliding(self.position, self.radius):
self.score_missile_collision(fleets, missile)
Test fails as expected. Recall that we were not checking for a collision in interact_with_missile, so this replicates the defect. Undo that.
Green. Commit: improve tests. Compute asteroid score in init, not dynamically.
I was a bit slow to commit along there, could have gone in smaller steps.
Now we can remove some unneeded code. There are objects implementing this method:
@staticmethod
def scores_for_hitting_asteroid():
return [0, 0, 0]
That’s no longer needed anywhere. Find and remove. And there’s a weird test that checks them:
def test_score_list(self):
ship = Ship(u.CENTER)
assert ship.scores_for_hitting_asteroid() == [0, 0, 0]
missile = Missile.from_ship(u.CENTER, Vector2(0, 0))
assert missile.scores_for_hitting_asteroid() == [100, 50, 20]
saucer = Saucer.large()
assert saucer.scores_for_hitting_asteroid() == [0, 0, 0]
Remove that. And the other:
def test_everyone_supports_asteroid_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_asteroid()
assert missile.scores_for_hitting_asteroid()
assert saucer.scores_for_hitting_asteroid()
assert ship.scores_for_hitting_asteroid()
Reviewing the code, I see that this can be improved:
def interact_with_missile(self, missile, fleets):
if missile.are_we_colliding(self.position, self.radius):
self.score_missile_collision(fleets, missile)
def interact_with_saucer(self, saucer, fleets):
self.split_or_die_on_collision(fleets, saucer)
def score_missile_collision(self, fleets, missile):
fleets.append(Score(self.score_for_hitting(missile)))
self.split_or_die_on_collision(fleets, missile)
def split_or_die_on_collision(self, fleets, collider):
if collider.are_we_colliding(self.position, self.radius):
self.split_or_die(fleets)
We know we’re colliding in score... so we need not call the conditional method.
def score_missile_collision(self, fleets, missile):
fleets.append(Score(self.score_for_hitting(missile)))
self.split_or_die(fleets)
Green. Commit: tidying.
I’m not entirely sure that I like this. The method score_missile_collision does splitting but doesn’t say that it does.
def interact_with_missile(self, missile, fleets):
if missile.are_we_colliding(self.position, self.radius):
self.score_and_split(fleets, missile)
def score_and_split(self, fleets, missile):
fleets.append(Score(self.score_for_hitting(missile)))
self.split_or_die(fleets)
Better. Also, the usual convention—I claim—is that the fleets argument goes last. We have a couple of exceptions here. Fix them.
def score_and_split(self, missile, fleets):
fleets.append(Score(self.score_for_hitting(missile)))
self.split_or_die(fleets)
PyCharm kindly changes all my callers. I do the same here:
def split_or_die_on_collision(self, collider, fleets):
if collider.are_we_colliding(self.position, self.radius):
self.split_or_die(fleets)
Commit: move fleets arguments to last in calling sequences.
I wonder if there are other cases, so do a Find. I find a couple of others and refactor. PyCharm is quite obliging about it.
Commit: move fleets arguments to last in calling sequences.
I think we’re done here, at least for now.
Summary
With some advice from our friends, we have simplified the logic of scoring. We have put knowledge of the score for a saucer in Saucer, for asteroids in the individual Asteroid and confirmation of whether to score or not into Missile, the only object that can actually score points. We have essentially the same control flow, from the score-bearing object to the missile and back, but the responsibilities are better allocated, with fewer objects having to worry about scoring.
Along the way, we removed unused code from other objects, and arranged to make some code unused and then arranged it.
We did it driven by existing tests, which turned out to be quite robust except for the somewhat incredible defect of a missile that racked up scores while in flight. Nice to have but not in the spirit of the game. We fixed the bug right after creating it and wrote a test for it.
I really can’t imagine myself being smart enough or paranoid enough to have written a test to ensure that missiles didn’t just fly about racking up points, but the truth is that after this refactoring, it was happening, and the only reason I noticed it was that I tested the actual game. I believe this makes a case for exploratory testing. Probably a real tester would have thought of the problem, but it was in any case obvious when I did run the game. Teaches me not to believe that my tests are perfect, which I didn’t believe anyway.
I think the code is better. I remain amazed at how much meat for learning and considering we can find in a single little program. And I am grateful to Bill and Bruce and Hill and Rickard and Tomas and all of you, for reading these articles, thinking about them, and advising me. It makes this much more fun.
See you next time!