Python 162
Saturday: Remove unneeded abstract method. Discuss design. Sunday: a nice refactoring series.
Saturday afternoon.
I begin this session with this change: Commit: make interact_with_fragment non-abstract and remove from all implementors, as it was always pass.
The convention we follow in this program is something like this:
The various
interact_with_*methods should be thought of as if they are events, to which one may subscribe, by implementing the method, or not subscribe, by not implementing it. Some of the methods are marked as@abstractmethodin the Flyer superclass, which is used to require certain methods to be explicitly implemented instead of justpassing silently.
This is done for what we might call the “solid” objects, the ones that fly visibly in space and which can collide with other objects: asteroid, missile, saucer, and ship. These methods have been shown to be too easy to forget when creating new flyers and were made abstract to force explicit implementation, hopefully preventing errors.
It’s as if there are events that are required to be handled. Even if you choose to ignore them, your object must explicitly say so. Other events are purely optional.
In the Kotlin version of asteroids, after GeePaw Hill delivered some heavy schooling to me, he devised an event system with a sort of default behavior, using delegation. The relevant articles start near here, for those of you interested in ancient history.
The whole point of that exercise was to avoid the use of implementation inheritance, which Hill considers to be anathema and I consider to be tricky, risky, dangerous, and in some cases a really fine idea. In the case of Asteroids, it may or may not be fine, but it is compact and frankly I find it rather simple. Perhaps it is I myself who is rather simple.
At present, all the interacting objects that cooperate to make the game asteroids are subclasses of Flyer, and therefore must implement those four key methods, interact_with_asteroid, ..._missile, ..._saucer, ..._ship, even though most of them pay attention to only a few of them, if any,
Fragment really interacts with nothing else. GameOver, nothing else. SaucerMaker, only with Saucer. Score, none of the big four. ScoreKeeper, none. ShipMaker, all four. Thumper, with Asteroid and Ship. WaveMaker, just Asteroid.
We could, in principle, save a few empty methods by making a superclass for some or all of those objects, allowing them to inherit the empty methods. Honestly, I think that would be a mistake. It would be a bit more obscure: now, everyone has to implement the big four, period. And no one really wants another superclass just to save a few lines of code.
Design thoughts …
I think that one essential characteristic of this “distributed intelligence” design is that it is tricky to understand as a whole, because there is no central game logic. In the least general tightest code version of Asteroids, we can have explicit code for every kind of interaction. It has the advantage that, once you get the scheme under your belt, design of individual objects is simpler, because you only consider how they interact with the ones you care about.
However, it’s going to be a bit tricky no matter how we do it. The ancient assembly language version is itself a bit complicated in the interactions, because it has a fixed table for the objects, and they go through the table in a nested loop, checking state in the table and sometimes, it appears, relying on the fact that if the one in your left hand is a missile, the one in your right hand must be an asteroid or a ship. Something like that, anyway.
The game’s rules are inherently asymmetric. Asteroids collide with anything except themselves. Missiles only score points if they are from the ship. And so on. Given the simplicity of Asteroids’ four main objects, the conventional design, which I’ve done a few times, with explicit interact these specific ones with those specific ones, leads to code that is easier to understand, at least when it comes down to asteroid vs missile sorts of considerations.
Bottom line, though this distributed version, I would say, is a bit much for a simple game like Asteroids, but offers interesting possibilities at scale. It reminds me a bit of micro-services, but the event orientation makes it a bit more like SIMSCRIPT or other event-oriented simulation languages.
Anyway, it has amused me.
Sunday morning.
A bit of a nudge from Bill Wake encourages me to take a harder look at the Saucer’s use of its size member. It is either 1, signifying a small saucer, or 2, signifying a large one, and we use it like this:
class Saucer(Flyer): @property
def always_target(self):
return self._size == 1
def create_surface_class_members(self):
raw_dimensions = Vector2(10, 6)
saucer_scale = 4 * self._size
Saucer.offset = raw_dimensions * saucer_scale / 2
saucer_size = raw_dimensions * saucer_scale
Saucer.saucer_surface = SurfaceMaker.saucer_surface(saucer_size)
def score_for_hitting(self, attacker):
return attacker.scores_for_hitting_saucer()[self._size - 1]
def update(self, delta_time, fleets):
if self._size == 2:
player.play("saucer_big", self._location, False)
else:
player.play("saucer_small", self._location, False)
self.fire_if_possible(delta_time, fleets)
self.check_zigzag(delta_time)
self._move(delta_time, fleets)
class TestSaucer:
def test_saucer_sizing(self):
fleets = Fleets()
fi = FI(fleets)
fleets.append(keeper := ScoreKeeper())
fleets.append(maker := SaucerMaker())
fleets.perform_interactions()
keeper.score = 0
fleets.tick(u.SAUCER_EMERGENCE_TIME)
saucers = fi.saucers
assert saucers
saucer = saucers[0]
assert saucer._size == 2
fleets.remove(saucer)
keeper.score = u.SAUCER_SCORE_FOR_SMALL
fleets.tick(u.SAUCER_EMERGENCE_TIME)
saucers = fi.saucers
assert saucers
saucer = saucers[0]
assert saucer._size == 1
The issue with all this is that it’s rather cryptic, and the values one and two are almost arbitrary. (They are not quite arbitrary, because they are the correct-ish multipliers for the saucer scale, only off by a factor of four.)
What Bill suggested was class / factory methods, and he wasn’t wrong. Let’s see how we create the Saucer.
When I ask PyCharm for reference to Saucer class, it finds 51! (That’s 51 excited, not 51 factorial. 51 factorial would be bad. Vanilla 51 is bad enough.) Fortunately, there’s only one place where we construct the Saucer in the actual game:
class SaucerMaker(Flyer):
def create_saucer(self, fleets):
if self._scorekeeper and self._scorekeeper.score >= u.SAUCER_SCORE_FOR_SMALL:
size = 1
else:
size = 2
fleets.append(Saucer(size))
I think we’re going to have to live with size for a while, but let’s begin here. Programming by intention, we’ll break things and fix them back up:
def create_saucer(self, fleets):
if self._scorekeeper and self._scorekeeper.score >= u.SAUCER_SCORE_FOR_SMALL:
saucer = Saucer.small()
else:
saucer = Saucer.large()
fleets.append(saucer)
Tests break, because those methods do not exist.
class Saucer(Flyer):
@classmethod
def small(cls):
return cls(1)
@classmethod
def large(cls):
return cls(2)
We’re green. Commit: introduce saucer creation methods small and large.
Let’s bite the bullet and fix up all the tests to use these methods. A quick look makes me think that a global replace may help me out here. I start with Saucer() going to Saucer.large(). That works just as one would hope, so I have now done them all.
Commit: All saucer creators are using class methods large or small.
Now back to Saucer. We’ll slowly winnow out the uses of _size and move them into our factory methods. Let’s do the sound first, because it’s the one that (I think) Bill was talking about.)
We start with this:
def update(self, delta_time, fleets):
if self._size == 2:
player.play("saucer_big", self._location, False)
else:
player.play("saucer_small", self._location, False)
self.fire_if_possible(delta_time, fleets)
self.check_zigzag(delta_time)
self._move(delta_time, fleets)
We provide this:
class Saucer(Flyer):
@classmethod
def small(cls):
return cls(1, "saucer_small")
@classmethod
def large(cls):
return cls(2, "saucer_big")
def __init__(self, size, sound):
...
self._sound = sound
self._zig_timer = Timer(u.SAUCER_ZIG_TIME)
self.missile_tally = 0
self.missile_head_start = 2*self._radius
self.create_surface_class_members()
def update(self, delta_time, fleets):
player.play(self._sound, self._location, False)
self.fire_if_possible(delta_time, fleets)
self.check_zigzag(delta_time)
self._move(delta_time, fleets)
I’ll test that in the game just to be sure. It’s good. Commit: saucer sound does not consider member _size.
Let’s tick through references to _size until it doesn’t matter.
@classmethod
def small(cls):
return cls(size=1, sound="saucer_small", always_target=True)
@classmethod
def large(cls):
return cls(size=2, sound="saucer_big", always_target=False)
I add always_target as a member, remove the old size-checking property, and we’re green.
Commit: always_target no longer considers size.
Now there’s this:
def create_surface_class_members(self):
raw_dimensions = Vector2(10, 6)
saucer_scale = 4 * self._size
Saucer.offset = raw_dimensions * saucer_scale / 2
saucer_size = raw_dimensions * saucer_scale
Saucer.saucer_surface = SurfaceMaker.saucer_surface(saucer_size)
We want a scale of either 4 or 8. Let’s pass that in on construction and pass the value here as a a parameter.
@classmethod
def small(cls):
return cls(size=1, sound="saucer_small", always_target=True, scale=4)
@classmethod
def large(cls):
return cls(size=2, sound="saucer_big", always_target=False, scale=8)
def __init__(self, size, sound, 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()
self._location = MovableLocation(position, velocity)
self._radius = 10*size # getting ready for small saucer
self._ship = None
self._size = size
self._sound = sound
self._zig_timer = Timer(u.SAUCER_ZIG_TIME)
self.always_target = always_target
self.missile_tally = 0
self.missile_head_start = 2*self._radius
self.create_surface_class_members(scale)
@staticmethod
def create_surface_class_members(drawing_scale):
raw_dimensions = Vector2(10, 6)
Saucer.offset = raw_dimensions * drawing_scale / 2
drawing_size = raw_dimensions * drawing_scale
Saucer.saucer_surface = SurfaceMaker.saucer_surface(drawing_size)
I renamed a few variables and accepted python’s suggestion about a static method. That does suggest that surface creation isn’t really in the right place. We’re after different game now. Test. Commit: surface creation no longer considers _size.
We’re left with this in Saucer:
def score_for_hitting(self, attacker):
return attacker.scores_for_hitting_saucer()[self._size - 1]
I don’t see anything great to do here without changing a lot of other classes. Let’s just do this:
def score_for_hitting(self, attacker):
index = 0 if self.is_small_saucer else 1
return attacker.scores_for_hitting_saucer()[index]
Of course we don’t know that but we can change our construction so that we do:
@classmethod
def small(cls):
return cls(size=1, sound="saucer_small", is_small=True, always_target=True, scale=4)
@classmethod
def large(cls):
return cls(size=2, sound="saucer_big", is_small=False, always_target=False, scale=8)
And of course we set the is in __init__:
def __init__(self, size, sound, is_small, always_target, scale):
...
self.is_small_saucer = is_small
There’s a lot of state here. We need to think about that. But first, let’s get rid of _size. We can commit: Saucer knows is_small_saucer without reference to _size.
Uses of the _size member are now just the two checks in this test:
def test_saucer_sizing(self):
fleets = Fleets()
fi = FI(fleets)
fleets.append(keeper := ScoreKeeper())
fleets.append(maker := SaucerMaker())
fleets.perform_interactions()
keeper.score = 0
fleets.tick(u.SAUCER_EMERGENCE_TIME)
saucers = fi.saucers
assert saucers
saucer = saucers[0]
assert saucer._size == 2
fleets.remove(saucer)
keeper.score = u.SAUCER_SCORE_FOR_SMALL
fleets.tick(u.SAUCER_EMERGENCE_TIME)
saucers = fi.saucers
assert saucers
saucer = saucers[0]
assert saucer._size == 1
def test_saucer_sizing(self):
fleets = Fleets()
fi = FI(fleets)
fleets.append(keeper := ScoreKeeper())
fleets.append(maker := SaucerMaker())
fleets.perform_interactions()
keeper.score = 0
fleets.tick(u.SAUCER_EMERGENCE_TIME)
saucers = fi.saucers
assert saucers
saucer = saucers[0]
assert not saucer.is_small_saucer
fleets.remove(saucer)
keeper.score = u.SAUCER_SCORE_FOR_SMALL
fleets.tick(u.SAUCER_EMERGENCE_TIME)
saucers = fi.saucers
assert saucers
saucer = saucers[0]
assert saucer.is_small_saucer
Commit: test no longer refers to _size.
Now we can remove the member and the creation parameters. In doing that, I realize that radius was being created using the input size, so we wind up with this:
@classmethod
def small(cls):
return cls(radius=10, sound="saucer_small", is_small=True, always_target=True, scale=4)
@classmethod
def large(cls):
return cls(radius=20, sound="saucer_big", is_small=False, always_target=False, scale=8)
def __init__(self, radius, 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()
self._location = MovableLocation(position, velocity)
self._radius = radius
self._ship = None
self._sound = sound
self._zig_timer = Timer(u.SAUCER_ZIG_TIME)
self.always_target = always_target
self.is_small_saucer = is_small
self.missile_tally = 0
self.missile_head_start = 2*self._radius
self.create_surface_class_members(scale)
We’re good. Commit: Remove _size, no longer used.
Reflection
In eight commits, with no real issues, and a couple of alerts from tests that I didn’t mention, we have removed some conditionals based on member _size, and a couple of odd calculations that used it. It is gone, out, removed, absent, and no longer present.
Are there still legitimate concerns? I think so. Let’s consider the two factory methods:
class Saucer(Flyer):
@classmethod
def small(cls):
return cls(
radius=10,
sound="saucer_small",
is_small=True,
always_target=True,
scale=4)
@classmethod
def large(cls):
return cls(
radius=20,
sound="saucer_big",
is_small=False,
always_target=False,
scale=8)
Five creation parameters seems like a lot. We could certainly imagine that the radius and scale could be combined somehow, but it’s not likely to make the code more clear. The is_small and always_target seem to be in lock-step, but that’s an artifact of a particular feature choice about large vs small, not as tight a binding as drawing scale and radius are.
I think that’s all OK and anything we did to improve it would make initializing and/or using these values less clear.
The Saucer does have a lot of members, a dozen in fact.
We could move always_target to the Gunner, and probably should. It is the only user of that variable. Let’s do.
class Gunner:
def __init__(self, always_target):
self._always_target = always_target
self._timer = Timer(u.SAUCER_MISSILE_DELAY)
def fire_available_missile(self, fleets, saucer, ship_or_none):
if ship_or_none and self.should_target():
solution = ShotOptimizer(saucer, ship_or_none).targeted_solution
else:
solution = ShotOptimizer(saucer, ship_or_none).random_solution
fleets.append(solution.saucer_missile())
def should_target(self):
return self._always_target or random.random() < u.SAUCER_TARGETING_FRACTION
Tests are breaking. They are not passing always_target when creating Gunner. Those are all easily fixed, just passing False, and True in one case that probably didn’t care.
Commit: Saucer does not retain always_target, passes to Gunner on creation.
That’s the only member where I see an easy removal. It seems likely that Saucer, as one of our most complex objects, may still be hinting that it might be composed of more smaller objects, but it’s not bad and I don’t see anything within easy reach.
Let’s sum up.
Summary
In a small system, perhaps checking saucer size was not a big deal. In a large system, an obscure number like that would cause head-scratching and confusion, slowing down progress and increasing the chance of defects. Getting rid of the confusing magic number was easy and done in many small steps, which could be done over days or weeks if need be.
Note that I went at the changes directly, finding uses of _size and moving desired values to the class methods, changing the code that referred to _size one bit at a time. The use of the value was mostly isolated to Saucer, so that made good sense. If the number had been more public, we might have created small property methods and used them externally. For example, we could have said:
@property
def is_small_saucer:
return self._size == 1
Having said that, we can keep the value internally, if we need it, while providing a more reasonable interface from outside.
We could even deprecate use of _size, allow a month for people to link up, etc etc. Obviously, it’s better to do each change across the whole code base, but that’s not always possible.
The point of this isn’t just to make this small Asteroids program better, though I freely grant that I simply love finding code that seemed perfectly reasonable at the time I wrote it, but that can be improved substantially. I’m not here to write perfect code. I’m here to change code to be more capable and more clear, day after day after week after month.
And what’s most delightful to me is that the kinds of issues we find here are so often the same ones we find in code in the wild, and the changes to improve things are the same changes we’d make out there as well as in here.
Things are different all over … and yet, they are very much the same.
See you next time!