Python Asteroids on GitHub

I thought this morning that I had broken the game. Why am I even doing this seemingly useless changing? How do you get to Carnegie Hall?1

As I came awake this morning, I became quite certain that I had broken the Coin and committed it. I thought that I had probably moved the fleets.clear() away from the beginning of at least one of the methods. I was drafting my title and apology as I rushed through my morning startup.

Fortunately, I was smarter than I had feared, and the Coin is just fine.

Why Are We Even Doing This?

Coin is a 36 line class and through all my four or five iterations yesterday, has always been somewhere in the range of 24 to 36 lines, including lots of white space. It was clear enough at the beginning, it was clear enough in every iteration, it is clear enough now, as we’ll see below.

So why was I massaging it?

Human languages are quite complex. To speak a human language well, we need to speak it a lot, and we need to make, and have corrected, a lot of mistakes. “I freeze, you froze, he has frozen.” Not “he has freezed”, though I might write that just to amuse myself.

Programming and programming languages are the same, in that we have to write a lot of code, correct it, and, ideally, have it reviewed by others if we are to get good at it. I welcome showing my code to my Zoom gang, because they will see things in it that I do not see and I will learn to do better.

But even when we do not have someone to review our words, we do well to rehearse our use of language and to review it ourselves. And in programming, we can always get a review from the compiler and from our tests.

In short, I made those many changes yesterday to learn, to discover, to devise, to discard, various ways of expressing the coin idea in Python.

And the coin idea isn’t really entirely unimportant, because what the game does depends entirely on how you start it up. If you just give it a WaveMaker, it will create four asteroids and they’ll coast around forever. Boring. If you just give it a ShipMaker, you’ll get a ship and you can fly it around and shoot, but there’s nothing to shoot at. Boring. If you leave out the Thumper, the game won’t make that scary sound.

So expressing the coins well has value. And since combinations of starting objects make a difference, there might even be some way of better expressing what combinations do. (We have not discovered that yet, if there is a way at all.)

So I was trying to learn better Python. The changes wouldn’t make the game better, but they might make me better.

And in that spirit

I want to try at least one more approach. Here’s Coin as it stands today:

class Coin:
    # This is the only class that knows what objects
    # make up a game, in the current case, Asteroids.
    # Want another game? Create another kind of coin.

    @classmethod
    def quarter(cls, fleets):
        fleets.clear()
        fleets.append(WaveMaker())
        fleets.append(ShipMaker())
        cls.append_common_elements(fleets)

    @classmethod
    def append_common_elements(cls, fleets):
        fleets.append(SaucerMaker())
        fleets.append(ScoreKeeper())
        fleets.append(Thumper())

    @classmethod
    def slug(cls, fleets):
        fleets.clear()
        fleets.append(WaveMaker())
        fleets.append(GameOver())
        cls.append_common_elements(fleets)

    @classmethod
    def no_asteroids(cls, fleets):
        fleets.clear()
        fleets.append(ShipMaker())
        cls.append_common_elements(fleets)

Do we like the order of these methods? Perhaps not: perhaps the code would be easier to understand if append_common_elements were last, instead of in between three top-level methods. And perhaps it should be marked private. Is this better?

class Coin:
    # This is the only class that knows what objects
    # make up a game, in the current case, Asteroids.
    # Want another game? Create another kind of coin.

    @classmethod
    def quarter(cls, fleets):
        fleets.clear()
        fleets.append(WaveMaker())
        fleets.append(ShipMaker())
        cls._append_common_elements(fleets)

    @classmethod
    def slug(cls, fleets):
        fleets.clear()
        fleets.append(WaveMaker())
        fleets.append(GameOver())
        cls._append_common_elements(fleets)

    @classmethod
    def no_asteroids(cls, fleets):
        fleets.clear()
        fleets.append(ShipMaker())
        cls._append_common_elements(fleets)

    @classmethod
    def _append_common_elements(cls, fleets):
        fleets.append(SaucerMaker())
        fleets.append(ScoreKeeper())
        fleets.append(Thumper())

I think that perhaps it is better.

Note
Should have committed right here. Didn’t. Bad Ron, no biscuit.

But why do we have a class when we never even create an instance? What is the Pythonic way of doing this? Perhaps it is top-level functions in a module.

def quarter(fleets):
    fleets.clear()
    fleets.append(WaveMaker())
    fleets.append(ShipMaker())
    _append_common_elements(fleets)


def slug(fleets):
    fleets.clear()
    fleets.append(WaveMaker())
    fleets.append(GameOver())
    _append_common_elements(fleets)


def no_asteroids(fleets):
    fleets.clear()
    fleets.append(ShipMaker())
    _append_common_elements(fleets)


def _append_common_elements(fleets):
    fleets.append(SaucerMaker())
    fleets.append(ScoreKeeper())
    fleets.append(Thumper())

Top level functions. To make that work, I had to change references to Coin. to coin, and be sure to import coin. The latter was already in place where it needed to be, but I came at it at an angle so had changed it to import coin as Coin temporarily.

I think I like this better. And I think it is decently Pythonic. And I hope that my more python-experienced readers will advise me if it isn’t, or if there is something better.

Do we have any decent tests? I’m not sure. Oh, look!

    def test_slug(self):
        fleets = Fleets()
        fi = FI(fleets)
        coin.slug(fleets)
        assert fi.saucermakers
        assert fi.scorekeepers
        assert fi.thumpers
        assert fi.wavemakers
        assert not fi.shipmakers

    def test_quarter(self):
        fleets = Fleets()
        fi = FI(fleets)
        coin.quarter(fleets)
        assert fi.saucermakers
        assert fi.scorekeepers
        assert fi.thumpers
        assert fi.wavemakers
        assert fi.shipmakers

    def test_no_asteroids(self):
        fleets = Fleets()
        fi = FI(fleets)
        coin.no_asteroids(fleets)
        assert fi.saucermakers
        assert fi.scorekeepers
        assert fi.thumpers
        assert not fi.wavemakers
        assert fi.shipmakers
Note
With these running, I absolutely could have committed now. Going to be a low-biscuit day for Ron today.

I want to make these better, however, so let’s insert an object that shouldn’t be there, and make sure that it isn’t there at the end. That will check for the clear, which was my mistaken concern this morning.

    def test_slug(self):
        fleets = Fleets()
        fi = FI(fleets)
        fleets.append(Asteroid())
        coin.slug(fleets)
        assert fi.saucermakers
        assert fi.scorekeepers
        assert fi.thumpers
        assert fi.wavemakers
        assert not fi.shipmakers
        assert not fi.asteroids

    def test_quarter(self):
        fleets = Fleets()
        fi = FI(fleets)
        fleets.append(Asteroid())
        coin.quarter(fleets)
        assert fi.saucermakers
        assert fi.scorekeepers
        assert fi.thumpers
        assert fi.wavemakers
        assert fi.shipmakers
        assert not fi.asteroids

    def test_no_asteroids(self):
        fleets = Fleets()
        fi = FI(fleets)
        fleets.append(Asteroid())
        coin.no_asteroids(fleets)
        assert fi.saucermakers
        assert fi.scorekeepers
        assert fi.thumpers
        assert not fi.wavemakers
        assert fi.shipmakers
        assert not fi.asteroids

There. Even better. What could go wrong? Well, what if we were to invent some new object, a SpaceFlea, that was supposed to be in every coin except slug? What if, right now, we mistakenly put an additional ScoreKeeper in the mix? What if we “accidentally” put a Score(10000) in there?

How could we write these tests so that they would break if things like that happened?

We could make a list of disallowed classes, classes that must not be in there initially, like Missile, Asteroid, and so on. There are a lot of them.

What if each test, instead of testing explicitly one at a time, had a list of expectations, and a list of all the other classes that should not be in there, and we checked both ways. And what if we also checked that all the subclasses of Flyer were in one or another of each list?

That seems like a lot, but maybe it wouldn’t be. How does FleetsInspector check for things like fi.asteroids?

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

Not quite what we need but couldn’t that be done this way:

    def select(self, condition):
        return self.fleets.select(condition)

    def select_class(self, klass):
        return self.select(lambda a: isinstance(a, klass))

    @property
    def asteroids(self):
        return self.select_class(Asteroid)

Yes. We should adjust FleetsInspector to always go through select_class, but for now, let’s focus on the test changes we need.

Note
Finally! At last he commits! Will the man never learn?

I need to commit before I do any more work here. What we have is good, and I forgot to commit it. Commit: move coin from class methods to top-level vanilla functions.

Note

For a while right here, I was messing about getting subclasses and trying to create lists of classes, kind of thrashing, and I need to settle down and rethink what additional tests may be worth adding. The answer could be “none” but I think there are some high risks.

As part of settling down, I’ll recast all the FleetsInspector properties to use the new select_class:

    @property
    def asteroids(self):
        return self.select_class(Asteroid)

    @property
    def explosions(self):
        return self.select_class(Explosion)


    @property
    def fragments(self):
        return self.select_class(Fragment)

And so on. I did that with multi-cursor editing, all in one go. Tests are all still green. Commit: tidying.

Now then, what do we want our coin tests to do:

  1. √ Check that the desired class instances are there;
  2. ? Check that no other class instances are there;
  3. X Check that every subclass of Flyer, even ones not known at the time of writing these tests, is checked by one of the preceding checks.

Suppose we started with a big list of all known classes as of now, created as a literal, not by calling Flyer.__subclasses__(). In each test, we create a list of the expected classes for that coin, WaveMaker, Thumper, whatever. We subtract those from a copy of the big list, then check that all the ones in expected are there, and all the ones in unexpected are not there.

Then, separately, we check every subclass of Flyer, to ensure that that class appears in our big list.

That latter test will be a pain from time to time, because it will fail every time we add a Flyer subclass. Of course, we have no intention of ever doing that again.

So why write this test? To learn how to better test this kine of thing. We’re sure the code is good. We might even be right. But the tests do not check everything that they reasonably could.

I need an alphabetized list of the classes. Alphabetized so that I can read the tests reasonably well.

This is harder than programming. Along the way I discover that for some reason, BeginChecker and EndChecker show up twice in Flyer.__subclasses__()

Fascinating!
I found a bug in a test. Here’s my new version of the slug test:
    @staticmethod
    def all_classes_except(classes):
        all_classes = [
            Asteroid, BeginChecker, EndChecker,
            Fragment, GameOver, Missile, Saucer, SaucerMaker,
            Score, ScoreKeeper, Ship, ShipMaker, Thumper, WaveMaker]
        return [k for k in all_classes if k not in classes]

    def test_slug(self):
        fleets = Fleets()
        fi = FI(fleets)
        fleets.append(Asteroid())
        coin.slug(fleets)
        desired = [SaucerMaker, ScoreKeeper, Thumper, WaveMaker]
        for klass in desired:
            assert fi.select_class(klass)
        undesired = self.all_classes_except(desired)
        for klass in undesired:
            assert not fi.select_class(klass)

I created an explicit list of all Flyer subclasses as of now, so that newly added classes will not be in there. I plan to check that shortly.

So now I list all the desired classes in a slug load and check them for being there and I check all other classes for not being there. And the test fails, because slug loads a GameOver and my test does not check it.

Fix the test:

    def test_slug(self):
        fleets = Fleets()
        fi = FI(fleets)
        fleets.append(Asteroid())
        coin.slug(fleets)
        desired = [GameOver, SaucerMaker, ScoreKeeper, Thumper, WaveMaker]
        for klass in desired:
            assert fi.select_class(klass)
        undesired = self.all_classes_except(desired)
        for klass in undesired:
            assert not fi.select_class(klass)

Change the other tests similarly:

    def test_quarter(self):
        fleets = Fleets()
        fi = FI(fleets)
        fleets.append(Asteroid())
        coin.quarter(fleets)
        desired = [SaucerMaker, ScoreKeeper, ShipMaker, Thumper, WaveMaker]
        undesired = self.all_classes_except(desired)
        for klass in desired:
            assert fi.select_class(klass)
        for klass in undesired:
            assert not fi.select_class(klass)

    def test_no_asteroids(self):
        fleets = Fleets()
        fi = FI(fleets)
        fleets.append(Asteroid())
        coin.no_asteroids(fleets)
        desired = [SaucerMaker, ScoreKeeper, ShipMaker, Thumper]
        undesired = self.all_classes_except(desired)
        for klass in desired:
            assert fi.select_class(klass)
        for klass in undesired:
            assert not fi.select_class(klass)

So that’s good. What about new subclasses not mentioned?

Extract a method:

    def all_classes_except(self, classes):
        all_classes = self.all_known_classes()
        return [k for k in all_classes if k not in classes]

    def all_known_classes(self):
        return [
            Asteroid, BeginChecker, EndChecker,
            Fragment, GameOver, Missile, Saucer, SaucerMaker,
            Score, ScoreKeeper, Ship, ShipMaker, Thumper, WaveMaker]

Green. Let’s commit: refactor test.

I’ll spare you repeated tries to get this right. An issue has arisen, I believe because pytest recompiles the tests and therefore the BeginChecker that I have in my list is not identical to the one that gets checked in this test:

    def test_no_unchecked_classes(self):
        # must check names because pytest recompiles.
        known_names = [k.__name__ for k in self.all_known_classes()]
        subs = Flyer.__subclasses__()
        new = [k for k in subs if k.__name__ not in known_names]
        assert not new

So this test as to jump through its own um orifice and check the class names because the classes won’t be identical but the names will match. Grr.

For now, I think this is good enough. Commit: add check to ensure all Flyer subclasses are checked in coin tests.

Reflection

This went nicely at first: I like the new top-level approach to the coin. I think it is Pythonic and I request advice from any readers who got all the way down here.

It seemed best to beef up the tests for coin to check all subclasses, not just the ones that I happened to think of when I wrote the test. When I did that, I discovered that the slug test was wrong, because it was not checking for GameOver. So I fixed that. That was an excellent thing to discover and made working on the tests worthwhile.

The tests are a bit deep in the bag, because they are checking a list of desired classes and subclasses. Getting the alphabetized list was somewhat tedious, but I wrote a little test whose failure printed the desired list:

    # def test_make_list(self):
    #     all_classes = Flyer.__subclasses__()
    #     names = list(map(lambda k: k.__name__, all_classes))
    #     names.sort()
    #     s = ""
    #     for name in names:
    #         s += name + ", "
    #     print(s)
    #     assert s == "hello"

Then I wrote that most recent test, that just compares the current subclasses of Flyer to the explicit list I created above, to fail if there are any new subclasses. That took me down a rat hole while I tried to figure out why BeginChecker and EndChecker were showing up as extra when in fact they’re right there in the list.

I’m pretty sure that’s because pytest recompiles your test files. I should probably put those extra test classes in a separate module that won’t be compiled by pytest, but I am tired now and will save that for later.

I believe that the tests for coin should either always use names or never use them. We’ll definitely save that for later as well.

Idea
I think I can do the difference method more nicely:
    def all_classes_except(self, classes):
        all_classes = self.all_known_classes()
        return all_classes - set(classes)

    def all_known_classes(self):
        return {
            Asteroid, BeginChecker, EndChecker,
            Fragment, GameOver, Missile, Saucer, SaucerMaker,
            Score, ScoreKeeper, Ship, ShipMaker, Thumper, WaveMaker}

Set subtraction: more clear than a list comprehension. Commit: use set instead of list comprehension.

Summary

I would completely agree with anyone who said that all these changes to coin don’t make the program enough better to justify the work I’ve put in on them. I would point out that we did discover a flaw in a test, but I’d still agree: the product did not benefit much, if any, from this work.

But the team did benefit. (That’s me.) I got lots of practice with organizing small methods with partial similarity to make them more clear. And I learned a new way of using top-level functions that doesn’t bother me as much as it used to. The fact that there is a module coin that can be referred to to fetch the functions makes them enough like what I’m used to that I can tolerate them.

And I think they are Pythonic and therefore I think I am improving with Python.

What we have here amounts to four or five hours of piano practice, using the real piano in the real concert hall. It wasn’t a concert, but it was darn good practice.

And that has value to me, and I show it to you in case it may have value to you, not in the code but in the idea that sometimes practice is, well, good practice.

See you next time!


  1. Practice, man, practice! ↩