Python Asteroids on GitHub

This morning I thought I’d clean up Fleets a bit. Is now the time for that?

Fleets class has a number of instances of Fleet in it, originally used to keep separate classes of object separate. The new scheme calls for Fleets not to distinguish objects by type at all, so it should ultimately come down to one simple inner collection.

It seems likely to me that Fleets class and Fleet class will merge to become just one class, since there will be no separate subclasses of Fleet carrying out special functions. Fleets currently has special methods, add_asteroid, remove_asteroid, and so on. I’ve started using the method add_flyer and remove_flyer for newer Flyer subclasses. Looking forward, I plan to convert all the other calls to those.

I’ll look for methods that are not used and remove them. Each simple step will leave Fleets a bit simpler and thereby make each next decision a little easier.

I immediately find asteroid_count, which is unused. Then safe_to_emerge, which got replaced a day or so ago. Then all_asteroids_are_away_from_center. That appears to be all the truly low-hanging fruit.

Commit: remove unused Fleets methods.

The method all_objects is used to assemble all the Fleet instances for iteration:

    @property
    def all_objects(self):
        return list(chain(*self.fleets.values()))

I think I’ll try changing various add / remove pairs to use add_flyer and remove_flyer. If I can change them all, the other collections will be able to be removed.

    def add_asteroid(self, asteroid):
        self.add_flyer(asteroid)

    def remove_asteroid(self, asteroid):
        self.remove_flyer(asteroid)

Tests pass. I feel the need to test the game but am confident it’ll work. (How confident am I, really? I am going to try it.) I’m glad I did that, becuase the beats don’t sound. There are three methods to consider here, not just two. In the case of asteroids, it’s _asteroids, which is this:

    @property
    def _asteroids(self):
        return self.select(lambda a: isinstance(a, Asteroid))

That fixes the sound. How does the Thumper work? I don’t think we really want to be counting asteroids on every beat.

class Fleets:
    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)

Right, make a note that we want to make that more efficient. We’re good, though. Commit: asteroids kept in flyers Fleet.

I could remove the asteroids collection from here:

    def __init__(self, asteroids=(), missiles=(), saucers=(), saucer_missiles=(), ships=()):
        self.fleets = dict(
            asteroids=Fleet([]),
            saucers=Fleet([]),
            ships=Fleet([]),
            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)

I think I’ll save that for later. No, let’s not. Remove that asteroids line. Tests green. Commit: remove asteroids fleet.

Moving right along, saucers.

    @property
    def saucers(self):
        return self.select(lambda s: isinstance(s, Saucer))

    def add_saucer(self, saucer):
        self.flyers.append(saucer)
    # no remove

    def remove_saucer(self, saucer):
        self.flyers.remove(saucer)

Commit: remove saucers fleet.

I think the only one left is ships:, same as above. Works. Commit: remove ships fleet.

Now there is something that is bothering me: those selects. They are clearly inefficient, but even worse, they mean that Fleet knows the class names of those kinds of flyers.

Let’s look for senders of those messages and see what we can do about them. The tests can use the FleetsInspector object for theirs.

No one asks for missiles, so I remove that one. Commit: remove missiles accessor. Same with saucers. Commit: remove saucers accessor. And saucer_missiles. Commit: remove saucer_missiles accessor.

Now it seems to me that we can convert everyone calling a particular method like add_missile to call add_flyer.

Commit: remove add_missile method. I think PyCharm should have been willing to inline those, but it was not. I pasted them instead.

Same with remove_missile. Commit. remove remove_missile.

It is so tempting to do more of these at once. Best not, it’s a bad habit.

Commits: remove add_asteroid, remove remove_asteroid, remove add_saucer, remove remove_saucer, remove add_saucer_missile, remove remove_saucer_missile, remove add_scorekeeper, remove add_score, remove add_ship, remove add_wavemaker, remove remove_score, remove remove_ship.

Whew. That took a bit of time, though it was basically trivial. I used the Replace in Files in PyCharm to make it easier.

I also do remove unused count., that method I built speculatively that has never been used other than in the test for it.

Now there’s no point to keeping the fleets member as a dictionary, we just have the one instance.

    def __init__(self, asteroids=(), missiles=(), saucers=(), saucer_missiles=(), ships=()):
        self.flyers = Fleet([])
        for asteroid in asteroids:
            self.add_flyer(asteroid)
        for missile in missiles:
            self.add_flyer(missile)
        for saucer in saucers:
            self.add_flyer(saucer)
        for saucer_missile in saucer_missiles:
            self.add_flyer(saucer_missile)
        for ship in ships:
            self.add_flyer(ship)
        self.thumper = Thumper(self.beat1, self.beat2)

A few lines had to be changed to refer to self.flyers and not to try to iterate fleets.values. I didn’t make the change very cleverly but it was fast. Commit: Fleets only has one Fleet instance now, flyers.

Fleets is now pretty lean and mean. I’d like to get rid of the callers of select that are referring to individual flyer classes:

    @property
    def _asteroids(self):
        return self.select(lambda a: isinstance(a, Asteroid))

    @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.select(lambda s: isinstance(s, Ship))

Let’s see about moving testing_only_score to FleetsInspector. I put this in:

class FleetsInspector:
    @property
    def scorekeeper(self):
        keepers = self.select(lambda s: isinstance(s, ScoreKeeper))
        if keepers:
            return keepers[0]
        else:
            return ScoreKeeper()

    @property
    def score(self):
        return self.scorekeeper.score

Then replace all the calls to testing_only_score with references to that, using a FleetsInspector instance as needed.

That was a bit tedious as well, and not educational in any notable way. Commit: remove testing_only_score from Interactor and Fleets, replace with FleetsInspector score.

I think we’re done here, let’s reflect and sum up.

We have done 26 commits this afternoon, most of them removing a method from Fleets, which is feeling quite trim. The changes were tedious but could have been done with Replace in Files if I were more inclined to just let that rip, but, for example, it was important to replace add_scorekeeper before doing add_score because the replace is not very clever. Nor am I, when it comes down to it.

Fleets is simplified to have just a single instance of Fleet, and I suspect we’ll quickly find that we can replace it with a simple list. We’ll leave that for a morning, when I’m fresh.

We still have two methods that are searching the flyers list at run time, _asteroids, and ships. Those are used in Thumper. If we make a ThumperFlyer, we should be able to remove those two searches.

It is worth noting that this design interacts every object with every other, and that is probably around one or two hundred interactions per cycle. Theoretically, I imagine it could get over 1000 interactions. Many if not most of those will resolve to a couple of quick method sends.

If it were done optimally, it would tend to be around, hmm, number of asteroids times two, plus number of asteroids times number of missiles on screen, perhaps as many as one hundred but more commonly less. On a 1980s computer, we would care. On my MacBook, we simply do not care. We have the luxury of writing code that expresses our design in ways that would not be accessible to us decades ago.

Of course, a modern game with thousands of interacting parts still can have performance issues and that is why architectures like Entity Component Systems are used. As I discussed in some previous articles, the ECS scheme doesn’t seem to pay off at Asteroids scale.

The Important Point

The important point remains that we can change the design of this program very substantially, over a period of many days, while keeping it running all the time.

Well, almost all the time. I have made mistakes and released a broken version a few times, but it should be easy to see that you would never have made those mistakes and that incremental refactoring toward a very different design is a perfectly viable thing to do.

We do want to have plenty of tests, and there are some areas that we’ve seen could use more. And it seems to me that with a video-oriented game, we always wind up needing to watch it run, both for confidence and because, every now and then, we discover a problem.

It is not given to us to be perfect, but we can be pretty darn good at what we do. Especially if we take very small steps.

See you next time!