Python 105 - Let's get rid of a Fleet subclass
While I believe that my preparation work the last few days has been valuable, it doesn’t feel much to me like real progress. Today I want a better feeling. And I get it!
The Fleet class, the vanilla class that Fleets uses to hold a particular type of Flyer, has three remaining subclasses, MissileFleet, SaucerFleet, and ShipFleet, MissileFleet used to support two sets of Missile, ones fired by the ship and ones from the Saucer. The class manages the total number of missiles the flyer can fire. But Ship now manages that fact on its own, and the MissileFleet is only used for saucer missiles.
It seems to me that if I review how Ship does it, I can make Saucer do the same thing. I think we’ll want to be sure we have a test for this, lest the Saucer become even more of a nemesis than it already is.
class Ship(Flyer):
def begin_interactions(self, fleets):
self._missile_tally = 0
def interact_with_missile(self, missile, fleets):
self.tally_ship_missiles(missile)
self.explode_if_hit(fleets, missile)
def tally_ship_missiles(self, missile):
if missile.is_ship_missile:
self._missile_tally += 1
def fire_if_possible(self, fleets):
if self._can_fire and self._missile_tally < u.MISSILE_LIMIT:
fleets.add_flyer(self.create_missile())
self._can_fire = False
That seems rather straightforward: we init the tally, tally instances, and fire if there are few enough. Saucer is a bit different internally, as we’ll review in a moment, but first I want to know if we have any tests for saucer missiles. Rather than search the tests, I decide to break this:
class Saucer(Flyer):
def fire_if_missile_available(self, saucer_missiles, ships) -> bool:
return saucer_missiles.fire(self.create_missile, ships)
class MissileFleet:
def fire(self, callback, *args) -> bool:
if len(self) < self.maximum_number_of_missiles:
self.append(callback(*args))
return True
else:
return False
I’ll hack that to always allow firing:
def fire(self, callback, *args) -> bool:
if len(self) < 100 + self.maximum_number_of_missiles:
self.append(callback(*args))
return True
else:
return False
And two tests fail. Let’s review them.
One of them is test_missile_fleet, which is no help since we plan to remove the MissileFleet this morning. The other test_can_only_fire_two is more promising:
def test_can_only_fire_two(self):
saucer = Saucer()
saucer_missiles = MissileFleet([], u.SAUCER_MISSILE_LIMIT)
saucer.fire_if_possible(delta_time=0.1, saucer_missiles=saucer_missiles, ships=[])
assert not saucer_missiles
saucer.fire_if_possible(u.SAUCER_MISSILE_DELAY, saucer_missiles=saucer_missiles, ships=[])
assert len(saucer_missiles) == 1
saucer.fire_if_possible(u.SAUCER_MISSILE_DELAY, saucer_missiles=saucer_missiles, ships=[])
assert len(saucer_missiles) == 2
saucer.fire_if_possible(u.SAUCER_MISSILE_DELAY, saucer_missiles=saucer_missiles, ships=[])
assert len(saucer_missiles) == 2
Let’s rewire this to use our FleetsInspector to get the missiles for counting.
def test_can_only_fire_two(self):
fleets = Fleets()
fi = FI(fleets)
saucer = Saucer()
saucer.fire_if_possible(delta_time=0.1, saucer_missiles=fleets.saucer_missiles, ships=[])
assert not fi.saucer_missiles
saucer.fire_if_possible(u.SAUCER_MISSILE_DELAY, saucer_missiles=fleets.saucer_missiles, ships=[])
assert len(fi.saucer_missiles) == 1
saucer.fire_if_possible(u.SAUCER_MISSILE_DELAY, saucer_missiles=fleets.saucer_missiles, ships=[])
assert len(fi.saucer_missiles) == 2
saucer.fire_if_possible(u.SAUCER_MISSILE_DELAY, saucer_missiles=fleets.saucer_missiles, ships=[])
assert len(fi.saucer_missiles) == 2
With my 100 removed, both the tests run. I think that should set us up for the new scheme. I note that we have at least one parameter in fire_if_possible that we’ll want to be rid of, the saucer_missiles.
OK, let’s work on Saucer. I could imagine writing a test for the missile counting but could I possibly get it wrong? Well, yes, I could. I could fail to check for the right kind of missile. OK, a new test.
def test_counts_saucer_missiles(self):
saucer = Saucer()
assert saucer._missile_tally == 0
Test is red. Make it green. In __init__:
self._missile_tally = 0
More testing:
def test_counts_saucer_missiles(self):
fleets = Fleets()
saucer = Saucer()
assert saucer._missile_tally == 0
saucer._missile_tally = 5
saucer.begin_interactions(fleets)
assert saucer._missile_tally == 0
Doesn’t quite work, since we aren’t initializing in begin yet.
def begin_interactions(self, fleets):
self._missile_tally = 0
Tests are green. We could be committing. It’s a good habit. Commit: Saucer has missile tally and zeros it. No counting yet.
More testing:
def test_counts_saucer_missiles(self):
fleets = Fleets()
saucer = Saucer()
assert saucer._missile_tally == 0
saucer._missile_tally = 5
saucer.begin_interactions(fleets)
assert saucer._missile_tally == 0
saucer_missile = Missile.from_saucer(saucer)
saucer.interact_with_missile(saucer_missile, fleets)
assert saucer._missile_tally == 1
Fails on the 1, of course. We need to change this:
def interact_with_missile(self, missile, fleets):
if missile.are_we_colliding(self.position, self.radius):
fleets.add_score(Score(self.score_for_hitting(missile)))
self.explode(fleets)
To this:
def interact_with_missile(self, missile, fleets):
if missile.is_saucer_missile:
self._missile_tally += 1
if missile.are_we_colliding(self.position, self.radius):
fleets.add_score(Score(self.score_for_hitting(missile)))
self.explode(fleets)
I’ve gone beyond my remit in that I checked for is_saucer_missile.
I have somehow configured the missile incorrectly.
def test_counts_saucer_missiles(self):
fleets = Fleets()
saucer = Saucer()
assert saucer._missile_tally == 0
saucer._missile_tally = 5
saucer.begin_interactions(fleets)
assert saucer._missile_tally == 0
saucer_missile = Missile.from_saucer(saucer.position, Vector2(0, 0))
saucer.interact_with_missile(saucer_missile, fleets)
assert saucer._missile_tally == 1
Test is green. Commit: saucer tallies saucer missiles.
Beef up the test a bit:
def test_counts_saucer_missiles(self):
fleets = Fleets()
saucer = Saucer()
assert saucer._missile_tally == 0
saucer._missile_tally = 5
saucer.begin_interactions(fleets)
assert saucer._missile_tally == 0
saucer_missile = Missile.from_saucer(saucer.position, Vector2(0, 0))
saucer.interact_with_missile(saucer_missile, fleets)
assert saucer._missile_tally == 1
ship_missile = Missile.from_ship(saucer.position, Vector2(0, 0))
saucer.interact_with_missile(ship_missile, fleets)
assert saucer._missile_tally == 1
Now we show that ship missiles are not counted. Green. Commit: improve test to show ship missiles are not tallied.
OK, I think we have solid enough tests that we should be able to redo the firing logic in Saucer.
def fire_if_missile_available(self, saucer_missiles, ships) -> bool:
return saucer_missiles.fire(self.create_missile, ships)
If we’re here, the timer has expired so we just need to check the tally and create the missile as needed.
I get this far and meet an obstacle:
def fire_if_missile_available(self, saucer_missiles, ships) -> bool:
if self._missile_tally >= u.SAUCER_MISSILE_LIMIT:
return False
missile = self.create_missile(ships)
I need a way to launch the missile and the saucer_missiles fleet will not do. Also let me type in the return True before I forget. We call that from here:
def tick(self, delta_time, fleet, fleets):
player.play("saucer_big", self._location, False)
saucer_missiles = fleets.saucer_missiles
ships = fleets.ships
self.fire_if_possible(delta_time, saucer_missiles, ships)
self.check_zigzag(delta_time)
self.move(delta_time, fleet)
Let’s change the signature.
def fire_if_possible(self, delta_time, fleets, ships):
self.missile_timer.tick(delta_time, fleets, ships)
That has broken a couple more tests. We’ve got to finish this before it’s worth looking, I think. No, the refactoring broke things. Undo it.
Manually rename:
def fire_if_possible(self, delta_time, fleets, ships):
self.missile_timer.tick(delta_time, fleets, ships)
And down in tick:
def tick(self, delta_time, fleet, fleets):
player.play("saucer_big", self._location, False)
ships = fleets.ships
self.fire_if_possible(delta_time, fleets, ships)
self.check_zigzag(delta_time)
self.move(delta_time, fleet)
Now let’s add our missile:
def fire_if_missile_available(self, fleets, ships) -> bool:
if self._missile_tally >= u.SAUCER_MISSILE_LIMIT:
return False
missile = self.create_missile(ships)
fleets.add_saucer_missile(missile)
return True
One test is failing. Ah, the compiler is unhappy because it wants me to refer to fleets now, not saucer_missiles:
def test_can_only_fire_two(self):
fleets = Fleets()
fi = FI(fleets)
saucer = Saucer()
saucer.fire_if_possible(delta_time=0.1, fleets=fleets, ships=[])
assert not fi.saucer_missiles
saucer.fire_if_possible(u.SAUCER_MISSILE_DELAY, fleets=fleets, ships=[])
assert len(fi.saucer_missiles) == 1
saucer.fire_if_possible(u.SAUCER_MISSILE_DELAY, fleets=fleets, ships=[])
assert len(fi.saucer_missiles) == 2
saucer.fire_if_possible(u.SAUCER_MISSILE_DELAY, fleets=fleets, ships=[])
assert len(fi.saucer_missiles) == 2
Now the test is failing on the last line, with 3 missiles.
Oh. This test doesn’t give us a chance to tally the missiles. It’s the test’s fault. I think I’ll make it work and then probably delete it, because I know the game works. I couldn’t resist trying it.
def test_can_only_fire_two(self):
fleets = Fleets()
fi = FI(fleets)
saucer = Saucer()
saucer.fire_if_possible(delta_time=0.1, fleets=fleets, ships=[])
assert not fi.saucer_missiles
saucer.fire_if_possible(u.SAUCER_MISSILE_DELAY, fleets=fleets, ships=[])
assert len(fi.saucer_missiles) == 1
saucer.begin_interactions(fleets)
for m in fi.saucer_missiles:
saucer.interact_with_missile(m, fleets)
saucer.fire_if_possible(u.SAUCER_MISSILE_DELAY, fleets=fleets, ships=[])
assert len(fi.saucer_missiles) == 2
saucer.begin_interactions(fleets)
for m in fi.saucer_missiles:
saucer.interact_with_missile(m, fleets)
saucer.fire_if_possible(u.SAUCER_MISSILE_DELAY, fleets=fleets, ships=[])
assert len(fi.saucer_missiles) == 2
After we follow the protocol things work. Here is a place where Rickard Lindberg’s concern about calling the Interactor probably applies. If we were going through Interactor, the begin and end and all that would have been handled.
Be that as it may, we’re green. Commit: Saucer now fires based on internal missile tally. MissileFleet should be able to be removed.
Now over in Fleets, let’s see what we can do about that MissileFleet.
class Fleets:
def add_saucer_missile(self, missile):
self.saucer_missiles.append(missile)
What happens if we change this to add_flyer? Curiously, a test fails. Probably looking in the wrong place. Ah:
def test_unsafe_because_saucer_missile(self):
fleets = Fleets()
missile = Missile(u.CENTER, Vector2(0, 0), [0, 0, 0], [0, 0, 0])
assert not FI(fleets).ships
fleets.tick(u.SHIP_EMERGENCE_TIME - 1)
fleets.add_saucer_missile(missile)
fleets.tick(1)
assert not FI(fleets).ships
FI(fleets).clear_saucer_missiles()
fleets.tick(0.001)
assert FI(fleets).ships
It’ll be that clear_saucer_missiles. At least I hope it is. No, not quite. I have to change the remove, because:
class FleetsInspector:
def clear_saucer_missiles(self):
for m in self.saucer_missiles:
self.fleets.remove_saucer_missile(m)
The problem is in Fleets:
def add_saucer_missile(self, missile):
self.add_flyer(missile)
def remove_saucer_missile(self, missile):
self.remove_flyer(missile)
I put those two methods side by side so that I’d remember to change one if I changed the other. I didn’t. But my tests saved me.
Green. Commit: all missiles now stored in Fleets.others.
Now let’s clear out that MissileFleet class. And let’s rename others to flyers while we’re at it.
@property
def flyers(self):
return self.fleets[6]
I think I should make self.fleets a dictionary but we’re on another mission now.
class Fleets:
def __init__(self, asteroids=(), missiles=(), saucers=(), saucer_missiles=(), ships=()):
self.fleets = (
Fleet([]),
Fleet([]),
SaucerFleet([]),
Fleet([]),
ShipFleet([]),
Fleet([]), # explosions not used
Fleet([]))
for asteroid in asteroids:
self.add_asteroid(asteroid)
for missile in missiles:
self.add_missile(missile)
for saucer in saucers:
self.add_saucer(saucer)
for saucer_missile in saucer_missiles:
self.add_saucer_missile(saucer_missile)
for ship in ships:
self.add_ship(ship)
self.thumper = Thumper(self.beat1, self.beat2)
I should be able to make those missile ones None but let’s hold off on that. There are still references to MissileFleet, in various tests.
I remove these:
def test_missile_fleet(self):
missiles = []
fleet = MissileFleet(missiles, 3)
fired = fleet.fire(lambda: 666)
assert fired
assert len(missiles) == 1
assert missiles[-1] == 666
fired = fleet.fire(lambda: 777)
assert fired
assert len(missiles) == 2
assert missiles[-1] == 777
fired = fleet.fire(lambda: 888)
assert fired
assert len(missiles) == 3
assert missiles[-1] == 888
fired = fleet.fire(lambda: 999)
assert not fired
assert len(missiles) == 3
def test_missile_fleet_parameter(self):
missiles = []
fleet = MissileFleet(missiles, 2)
fired = fleet.fire(lambda m: m * 2, 333)
assert fired
assert len(missiles) == 1
assert missiles[-1] == 666
The rest of the references are unused imports. Removed. Remove the class. Green. Commit: MissileFleet class removed.
Reflection
We’ve moved a very small amount of behavior over to Saucer, to init a tally, increment it, and check it before firing a missile. In return we have removed an entire class, some tests, and simplified the firing logic a bit.
This is the kind of thing that I hope for in the decentralized style.
However, the Saucer is rather a complicated object and these changes, while small, didn’t make Saucer itself any simpler. If anything, they added a small amount of complexity to it with the new member _missile_tally. We should add some simplicity to Saucer, and I’ve made a note of that.
The tests served pretty well, failing when they should (and once when they needed updating) and I’m glad that I reviewed them and wrote those new ones first.
I’d like to do a bit more now, in Fleets, before I close out the morning.
First, let’s change our self.fleets from a list to a dictionary.
class Fleets:
def __init__(self, asteroids=(), missiles=(), saucers=(), saucer_missiles=(), ships=()):
self.fleets = (
Fleet([]),
Fleet([]),
SaucerFleet([]),
Fleet([]),
ShipFleet([]),
Fleet([]), # explosions not used
Fleet([]))
for asteroid in asteroids:
self.add_asteroid(asteroid)
for missile in missiles:
self.add_missile(missile)
for saucer in saucers:
self.add_saucer(saucer)
for saucer_missile in saucer_missiles:
self.add_saucer_missile(saucer_missile)
for ship in ships:
self.add_ship(ship)
self.thumper = Thumper(self.beat1, self.beat2)
This will be a bit tricky because of all these base accessors:
@property
def all_objects(self):
return list(chain(*self.fleets))
@property
def _asteroids(self):
return self.fleets[0]
@property
def asteroid_count(self):
return len(self._asteroids)
@property
def missiles(self):
return self.select(lambda m: isinstance(m, Missile))
@property
def saucers(self):
return self.fleets[2]
@property
def saucer_missiles(self):
return self.fleets[3]
@property
def testing_only_score(self):
keeper = next((k for k in self.flyers if isinstance(k, ScoreKeeper)), ScoreKeeper())
return keeper.score
@property
def ships(self):
return self.fleets[4]
@property
def explosions(self):
return self.fleets[5]
@property
def flyers(self):
return self.fleets[6]
We’ll want to change all the indexed ones to use the names of the fleets. If we create them in the same order, however, I think Python’s dictionary may work. I think they’re ordered. We’ll see. I expect things to break before we’re done here.
class Fleets:
def __init__(self, asteroids=(), missiles=(), saucers=(), saucer_missiles=(), ships=()):
self.fleets = dict(
asteroids=Fleet([]),
missiles=Fleet([]),
saucers=SaucerFleet([]),
saucer_missiles=Fleet([]),
ships=ShipFleet([]),
explosions=Fleet([]), # explosions not used
flyers=Fleet([]))
34 tests fail. I think they are mostly due to the key lookups, so I change all those like this:
@property
def _asteroids(self):
return self.fleets["asteroids"]
Other things need work. A bit of reading tells me that Python iterates a dictionary returning its keys. “I might have gone another way.”1 OK, so I need:
def tick(self, delta_time):
if self._asteroids and self.ships:
self.thumper.tick(delta_time)
else:
self.thumper.reset()
for fleet in self.fleets.values():
fleet.tick(delta_time, self)
And I change just this one other method:
@property
def all_objects(self):
return list(chain(*self.fleets.values()))
And I am green. (That used to just say self.fleets.)
Test in game. Glad I did, draw fails.
def draw(self, screen):
for fleet in self.fleets.values():
fleet.draw(screen)
We are green and good. Commit: Convert Fleets to dictionary as self.fleets.
Now I’d like to remove the Fleets that I think are no longer in use.
I can remove saucer_missiles Fleet with no tests breaking:
self.fleets = dict(
asteroids=Fleet([]),
missiles=Fleet([]),
saucers=SaucerFleet([]),
# saucer_missiles=Fleet([]),
ships=ShipFleet([]),
explosions=Fleet([]), # explosions not used
flyers=Fleet([]))
Missiles, however, breaks some tests. One refers to this in Fleets:
@property
def saucer_missiles(self):
return self.fleets["missiles"]
That seems wrong on the face of it.
@property
def saucer_missiles(self):
return self.select(lambda m: isinstance(m, Missile) and m.is_saucer_missile)
Tests are green. Commit: no longer provide missiles or saucer_missiles Fleets.
I comment out explosions and two tests fail.
One is this:
def test_all_objects(self):
fleets = Fleets([1], [2], [3], [4], [5])
fleets.explosions.append(6)
all_objects = fleets.all_objects
assert len(all_objects) == 6
assert sum(all_objects) == 21
I’m going to remove that test. Replicating it is a step further than I’m prepared to take. I may regret this. Then there’s this:
def test_frag_timeout(self):
frag = Fragment(position=u.CENTER, fragments=["ignored"])
fleets = Fleets()
frags = fleets.explosions
frags.append(frag)
frags.tick(0.1, fleets)
assert frags
frags.tick(u.FRAGMENT_LIFETIME, fleets)
assert not frags
OK, this one I’ll recode.
def test_frag_timeout(self):
frag = Fragment(position=u.CENTER, fragments=["ignored"])
fleets = Fleets()
fi = FI(fleets)
fleets.add_flyer(frag)
fleets.tick(0.1)
assert fi.fragments
frags.tick(u.FRAGMENT_LIFETIME, fleets)
assert not fi.fragments
FI does not know fragments, so:
class FleetsInspector:
@property
def fragments(self):
return self.select(lambda f: isinstance(f, Fragment))
However my test is not running. When I fix the second tick, it runs.
def test_frag_timeout(self):
frag = Fragment(position=u.CENTER, fragments=["ignored"])
fleets = Fleets()
fi = FI(fleets)
fleets.add_flyer(frag)
fleets.tick(0.1)
assert fi.fragments
fleets.tick(u.FRAGMENT_LIFETIME)
assert not fi.fragments
We are green. Remove my commented-out lines:
class Fleets:
def __init__(self, asteroids=(), missiles=(), saucers=(), saucer_missiles=(), ships=()):
self.fleets = dict(
asteroids=Fleet([]),
saucers=SaucerFleet([]),
ships=ShipFleet([]),
flyers=Fleet([]))
for asteroid in asteroids:
self.add_asteroid(asteroid)
for missile in missiles:
self.add_missile(missile)
for saucer in saucers:
self.add_saucer(saucer)
for saucer_missile in saucer_missiles:
self.add_saucer_missile(saucer_missile)
for ship in ships:
self.add_ship(ship)
self.thumper = Thumper(self.beat1, self.beat2)
Commit: Fleets no longer maintains useless collections for missiles, saucer missiles, or explosions.
Let’s sum up.
Summary
The first move, removing the MissileFleet, moving missile tally to the Saucer, went smoothly enough. That cleared the field for some simplification of Fleets, removing unused Fleet instances.
Why
I did that in two phases and while I think I knew the reason for the first intuitively, I don’t think I expressed it to myself, nor here.
Converting Fleets’ collection of Fleet instances to a dictionary was desirable because it used to be a list and removing Fleet instances from the list, which we wanted to do, would entail fixing up all those fleets[5] references. With a dictionary instead of a list, we had to fix them up once, and thereafter they’ll work just fine as we add and remove Fleet instances from the dictionary. That’s why I did that, even though I didn’t express it.
Once that was working, removing the unused Fleet instances was a simple-enough matter of commenting one out. chasing any tests that broke, and moving on. We removed three, leaving four, asteroids, saucers, ships, and flyers. We plan to move to just one collection, the flyers, by the time we’re done.
Feeling Better
Today felt more productive to me for two reasons.
First, we actually progressed things by moving missile tallying to Saucer and eliminating the Missile Fleet. This was a real move toward decentralization, not just a clean-up in preparation.
Second, though, the actual cleanup felt more productive to me. Converting to a dictionary makes the design better even if we’re not going to remove Fleet instances, because fleets[“asteroids”] is more clear than fleets[0]. And, of course, we reduced the actual complexity of Fleets substantially by removing those extra Fleet instances.
Since this approach felt more productive, I think I’ll continue down the path of moving capability to the Flyers and removing Fleet instances. We may find some preparatory steps needed, but we’ll try to let actual need drive them, not general cleanup.
A good morning. And a long article. I’m working toward a record, I guess.
See you next time!
-
Riddick, referring to the Necromancer ship’s design motif. ↩