Python Asteroids on GitHub

Let’s see if we can reduce accesses to the specialized lists in Fleets. As so often happens, the next step isn’t in the direction I expect. That’s OK.

The Fleets object now holds some specialized Fleets, and some vanilla ones. It initializes like this:

class Fleets:
    def __init__(self, asteroids=None, missiles=None, saucers=None, saucer_missiles=None, ships=None):
        asteroids = asteroids if asteroids is not None else []
        missiles = missiles if missiles is not None else []
        saucers = saucers if saucers is not None else []
        saucer_missiles = saucer_missiles if saucer_missiles is not None else []
        ships = ships if ships is not None else []
        self.fleets = (
            Fleet(asteroids),
            Fleet(missiles),
            SaucerFleet(saucers),
            MissileFleet(saucer_missiles, u.SAUCER_MISSILE_LIMIT),
            ShipFleet(ships),
            Fleet([]),  # explosions not used
            Fleet([]))
        self.thumper = Thumper(self.beat1, self.beat2)
        self.score = 0

Oh, we don’t need that score any more, do we?

I’m already distracted from what I thought I’d be doing, but this is worth going after. I’ve checked, and both Saucer and Asteroid add score by adding a Score object via Fleets, to be eaten by the ScoreKeeper. However Fleets is also maintaining the score:

class Fleets:
    def add_score(self, score):
        self.others.append(score)
        self.score += score.score

Let’s just start removing. I think a lot of tests are going to break, and I think we can fix it readily, if inefficiently.

As soon as I remove that line, five tests fail.

Here’s an example:

    def test_missile_asteroid_scores(self):
        pos = Vector2(100, 100)
        asteroid = Asteroid(2, pos)
        asteroids = [asteroid]
        missile = Missile.from_ship(pos, Vector2(0, 0))
        missiles = [missile]
        interactor = Interactor(Fleets(asteroids, missiles, [], [], []))
        interactor.interact_one_pair(asteroid, missile)
        assert not missiles
        assert interactor.score == 20
        assert len(asteroids) == 2

Interactor gets the score from its Fleets instance:

class Interactor:
    @property
    def score(self):
        return self.fleets.score

Let’s do this in Fleets:

    @property
    def score(self):
        keeper = next((k 
        	for k in self.others 
        	if isinstance(k, ScoreKeeper)))
        return keeper.score

We’ll fetch the ScoreKeeper and ask it. Slow but it’s only in testing. Twelve tests failing. I suspect none are primed with a ScoreKeeper.

Changing the test to this works:

    def test_missile_asteroid_scores(self):
        pos = Vector2(100, 100)
        asteroid = Asteroid(2, pos)
        asteroids = [asteroid]
        missile = Missile.from_ship(pos, Vector2(0, 0))
        missiles = [missile]
        fleets = Fleets(asteroids, missiles, [], [], [])
        fleets.add_scorekeeper(ScoreKeeper())
        interactor = Interactor(fleets)
        interactor.perform_interactions()
        interactor.perform_interactions()
        assert not missiles
        assert interactor.score == 20
        assert len(asteroids) == 2

I had to make two changes, one to add the ScoreKeeper, and the other to call perform_interactions twice, once to get the kill and once to consume the score. I’ll change the others that are similar.

The changes were all trivial, much like the one above, just tedious. Along the way I removed game’s access to the score, as it is not needed. The Fleets instance only provides it for the tests, I’m pretty sure. I’ll look for senders.

I rename the score properties in Interactor and Fleets to testing_only_score. That should keep my grubby mitts off of it.

Commit: access to score through Interactor and Fleets is only for testing. Game just lets it happen via Score and ScoreKeeper.

Summary

That’s not even what I had in mind when I came here, but I’ll take it.

My original plan was to begin to make the individual collections in Fleets virtual, adding everything to others and then using list comprehensions to pull out specialized lists, then tick through the use of those to remove everything but tests.

The design mistake was or is leaving access to those collections open at all, but the game was originally built around separate collections, so there are still some places that are looking for them. We’ll need to be sure we root them out.

I am not expecting two more magical moves and everything is suddenly moved into the decentralized mode. A refactoring this major takes time, even in a small program like this one. We have to tease out each thread, one at a time, until we’re done, or close enough.

I say “close enough” intentionally. In a real program being built with other people’s money, we do best to refactor just enough to keep our speed of building new capability high. We are not striving for some kind of perfection, we refactor because when we do the right amount, our speed of developing what people need is maximized. How much is enough? That requires good judgement. How do we get good judgement? By doing things a lot and paying attention. In many cases, that means by doing things wrong and learning why.

For safety, I add a default to the score thing:

    @property
    def testing_only_score(self):
        keeper = next((k 
        	for k in self.others 
        	if isinstance(k, ScoreKeeper)), ScoreKeeper())
        return keeper.score

Now that next refers to a collection of all the ScoreKeepers from self.others, plus one fresh one. So we can’t get an exception, only a zero score. In any case, the tests are all doing things right, at least for now.

I think we’ll call this a wrap. Commit: ensure test_only_score always finds a ScoreKeeper.

See you next time! Maybe I’ll do what I plan to do. Maybe.