Python 102

Python Asteroids on GitHub

Let’s start with a question from Rickard, and then have a look at the Fleets / Fleet setup, to find one or more small steps toward our desired design. ALSO: Big mistake! Released broken game!

Testing Question

Rickard Lindberg asked a question, over on the elephant system. I don’t have it here in front of me, so I’ll probably not phrase it well, but he was wondering about my tendency to test objects by calling their begin, interact, end methods instead of setting up an Interactor, a Fleets or a Game and driving interactions from there.

I do this habitually, but not thoughtlessly. Let’s think of it this way:

Suppose we were absolutely certain that the game would create an Interactor, providing it with the Fleets, and we were absolutely certain that the Interactor would correctly perform the begin, interact, end sequence, and we were absolutely certain that the objects all correctly implemented interact_with(something) by saying something.interact_with_munchkin if the object was a munchkin.

If we are certain of those things, then testing the munchkin’s begin, interact_with_, and end is sufficient to give us certainty that the munchkin will properly play its part in the game.

Now of course I would not stake the life of my cat on all those “absolutely certain” things up there, but because I do have a pretty reasonable set of tests for things, I am quite certain that those things do work, so I gain most of the additional certainty I need by testing the individual objects.

This is the thing that Hill calls “microtests”. We mostly test individual things with simple tests, with only occasional tests of larger assemblies. If all the objects work, the system works.

This scheme is not perfect, even if we were perfect, and we’re not. But it works quite well, the test are easier to write, and we are careful to add new tests when something does slip through the net.

Is this the best way? That’s not for me to say. It’s the way I mostly work, and it seems to work better for me than writing larger integration tests, primarily because doing the large ones is exponentially more painful than the small ones, and I am weak, and therefore I test large things reluctantly. So I learn to test small things in a way that tends to make the system work.

What should you do? Well, thanks for asking. I am not the boss of you, but I’d recommend that you try writing lots of little tests and see what happens. You might find an improvement in your speed, your joy, or your manual dexterity. And if not, well, maybe you would find it best to stop doing that and try something else.

Fleets / Fleet

There are at least two things not to like with the Fleets/Fleet setup.

Separate Instances

First, there is a separate Fleet instance in Fleets for each of several kinds of Flyers, asteroids, missiles, and so on. The decentralized design idea would have the Game and Fleets have no knowledge of the data types of the Flyers. This is a questionable decision, in that by some folx’ lights, it is madness ever to lose information about the type of an object.

In some contexts, these folx are quite right. In a language like Kotlin or Java or C#, if you ever lose track of the type of an object, you basically can’t talk to it any more, because the compiler insists on knowing what calls can be made on every object. It is argued that this requirement makes writing correct code easier, especially in large systems with many objects and types. This argument has merit.

But there is this other language design notion, starting perhaps with Smalltalk, where objects understand certain calls, and any object of any class can respond to any message if the programmer so deems. Naturally, this determination needs to be made at run time, so these languages are slower, often called “less efficient”, because they determine at the last possible moment what can, or cannot, be said to any given object. We often call this scheme “duck typing”. Python is such a duck-typing language.

As such, in the case of our Asteroids program, since all our Flyer objects will respond to the messages we intend to send them, there is no reason for Fleets to know their types. It has no real concerns about those types. Therefore, I’d like to get rid of the individual Fleet instances in Fleets, putting all the ducks into a single row, as it were.

Game Knowledge

The individual Fleet classes can and do maintain information relating to the specific game, Asteroids. Still left in the system we have MissileFleet, which manages a limit on the number of missiles it can contain. While Ship no longer uses a MissileFleet, Saucer still does.

We have SaucerFleet, which maintains a timer and creates a new saucer seven seconds after the previous one went away. And we have ShipFleet, which manages creation of new ships after old ones are destroyed.

Apropos those two, we used to have an AsteroidsFleet that watched to see whether all the asteroids were used up and if so, created a new wave. That logic has been moved to a Flyer, WaveMaker, which hangs around in outer space and creates new asteroids as needed. We’ll surely use similar logic in replacing the SaucerFleet and ShipFleet.

Inappropriate Intimacy

I knew there were more than two. Always more than two there are¹. There are calls in Fleets to return any of the Fleet collections it contains. Once an object has grabbed one of those collections, it can in principle do anything. We should avoid this sort of thing as a matter of principle, often called “Tell, don’t ask” or “Law of Demeter”.

Whatever it’s called, I think we’ll find that the design is cleaner and more clear when we get rid of those methods, although some of them may be retained for testing purposes, where we really do want to count the number of thingies that have been created after destruction of the widget.

Well, I say “really”. There might be other ways to be sure of things. We’ll think about that as we look at tests that seem to need intimate knowledge. We might even resort to some test doubles, and we can always, if all else fails, oh here’s an idea, we could make an object that covers Fleets and rummages around inside it, for testing purposes only.

This morning, I plan to start looking around in Fleets, with an eye to finding a step in the direction of the overall design vision. I’m not looking for a “best” step, if there even is such a thing. I’m looking for small steps that seem to take me where I want to go.

Let’s get to it. I think I’ll start with the accessors to the individual Fleet instances:

class Fleets:
    @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.fleets[1]

    @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.others 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 others(self):
        return self.fleets[6]

Can you see how maybe I think this is way too invasive? People are grabbing this object by its asteroids, and frankly I do not like it. It’s pushing me to maintain a collection of asteroids that I really don’t want.

Who’s referring to that property? There are nine accesses, six of them inside Fleets itself.

class Fleets:
    def add_asteroid(self, asteroid):
        self.asteroids.append(asteroid)

I think that one is nearly legitimate, though in the fullness of time I think we’ll change all those to plain old add or append, since Fleets will no longer care about types.

class Fleets:
    def all_asteroids_are_away_from_center(self):
        for asteroid in self.asteroids:
            if asteroid.position.distance_to(u.CENTER) < u.SAFE_EMERGENCE_DISTANCE:
                return False
        return True

That is used for Ship emergence, and we’ll rig the ShipMaker, or whatever we call it, to check asteroids during interactions. So it’s OK for now.

class Fleets:
    @property
    def asteroid_count(self):
        return len(self.asteroids)

That one is used in Ship, to calculate the hyperspace destruction odds, and used many times in the WaveMaker tests, mostly making sure we create the right number of asteroids. We can allow ship to know how many asteroids there are by counting them, and since it already checks to see if it is hitting them, counting them will be inexpensive.

Let’s change those tests right now. Here’s the biggie:

    def test_timer(self):
        fleets = Fleets()
        maker = WaveMaker()
        maker.begin_interactions(fleets)
        assert not maker.saw_asteroids
        assert not fleets.asteroid_count
        maker.tick(0.1, None, fleets)
        assert not fleets.asteroid_count
        maker.tick(u.ASTEROID_DELAY, None, fleets)
        assert fleets.asteroid_count == 4

        self.clear_and_tick(fleets, maker)
        assert fleets.asteroid_count == 6

        self.clear_and_tick(fleets, maker)
        assert fleets.asteroid_count == 8

        self.clear_and_tick(fleets, maker)
        assert fleets.asteroid_count == 10

        self.clear_and_tick(fleets, maker)
        assert fleets.asteroid_count == 11

        self.clear_and_tick(fleets, maker)
        assert fleets.asteroid_count == 11

First I’ll extract one of those fleets.asteroid_count calls.

    def test_timer(self):
        fleets = Fleets()
        maker = WaveMaker()
        maker.begin_interactions(fleets)
        assert not maker.saw_asteroids
        assert not self.count_asteroids(fleets)
        maker.tick(0.1, None, fleets)
        assert not self.count_asteroids(fleets)
        maker.tick(u.ASTEROID_DELAY, None, fleets)
        assert self.count_asteroids(fleets) == 4

        self.clear_and_tick(fleets, maker)
        assert self.count_asteroids(fleets) == 6

        self.clear_and_tick(fleets, maker)
        assert self.count_asteroids(fleets) == 8

        self.clear_and_tick(fleets, maker)
        assert self.count_asteroids(fleets) == 10

        self.clear_and_tick(fleets, maker)
        assert self.count_asteroids(fleets) == 11

        self.clear_and_tick(fleets, maker)
        assert self.count_asteroids(fleets) == 11

    def count_asteroids(self, fleets):
        return fleets.asteroid_count

Now all I need to do is revise that count_asteroids method.

    @staticmethod
    def count_asteroids(fleets):
        asteroids = [a for a in fleets.all_objects if isinstance(a, Asteroid)]
        return len(asteroids)

I also had to turn another test-supporting method non-static, so it could call the new counter method:

    def clear_and_tick(self, fleets, maker):
        for asteroid in fleets.asteroids:
            fleets.remove_asteroid(asteroid)
        maker.begin_interactions(fleets)
        maker.tick(0.1, None, fleets)
        assert not self.count_asteroids(fleets)
        maker.tick(u.ASTEROID_DELAY, None, fleets)

With that done, the tests no longer refer to asteroid_count. Commit: tests no longer use Fleets.asteroid_count.

That was kind of a digression but it removed a pile of accesses to a method that needs to go away. The remaining call is in Ship, so I think we can be confident that it will go away soon enough.

We were looking at accesses to the asteroid property. Most of them are internal to Fleets. There are some tests.

    def test_access(self):
        asteroids = ["asteroid"]
        missiles = ["missile"]
        saucers = ["saucer"]
        saucer_missiles = ["saucer_missile"]
        ships = ["ship"]
        fleets = Fleets(asteroids, missiles, saucers, saucer_missiles, ships)
        assert fleets.asteroids.flyers == asteroids
        assert fleets.missiles.flyers == missiles
        assert fleets.saucers.flyers == saucers
        assert fleets.saucer_missiles.flyers == saucer_missiles
        assert fleets.ships.flyers == ships

That’s boring and will surely want to disappear sometime soon, after all the other accesses are gone.

    def test_collider_via_game_with_score(self):
        game = Game(True)
        asteroid = Asteroid(2, Vector2(100, 100))
        game.fleets.asteroids.append(asteroid)
        game.fleets.add_scorekeeper(ScoreKeeper())
        missile = Missile.from_ship(Vector2(100, 100), Vector2(3, 3))
        game.fleets.missiles.append(missile)
        game.process_interactions()
        game.process_interactions()
        assert game.fleets.testing_only_score == 20

That can use add_asteroid. Commit: remove access to Fleets.asteroids.

Then there’s this guy, who we were just looking at a moment ago:

    def clear_and_tick(self, fleets, maker):
        for asteroid in fleets.asteroids:
            fleets.remove_asteroid(asteroid)
        maker.begin_interactions(fleets)
        maker.tick(0.1, None, fleets)
        assert not self.count_asteroids(fleets)
        maker.tick(u.ASTEROID_DELAY, None, fleets)

I could fetch them the hard way here. Yes, I think I’ll do that.

    @staticmethod
    def find_asteroids(fleets):
        return [a for a in fleets.all_objects if isinstance(a, Asteroid)]

    def count_asteroids(self, fleets):
        asteroids = self.find_asteroids(fleets)
        return len(asteroids)

    def clear_and_tick(self, fleets, maker):
        for asteroid in self.find_asteroids(fleets):
            fleets.remove_asteroid(asteroid)
        maker.begin_interactions(fleets)
        maker.tick(0.1, None, fleets)
        assert not self.count_asteroids(fleets)
        maker.tick(u.ASTEROID_DELAY, None, fleets)

OK, we’re paying a price in the tests, but we’re removing accesses to the asteroids property in Fleets, which will pay off soon, because we plan to get rid of all those special collections. One at a time, step by step.

We’re left with the usages in Fleets itself and our one accessor test:

    def test_access(self):
        asteroids = ["asteroid"]
        missiles = ["missile"]
        saucers = ["saucer"]
        saucer_missiles = ["saucer_missile"]
        ships = ["ship"]
        fleets = Fleets(asteroids, missiles, saucers, saucer_missiles, ships)
        assert fleets.asteroids.flyers == asteroids
        assert fleets.missiles.flyers == missiles
        assert fleets.saucers.flyers == saucers
        assert fleets.saucer_missiles.flyers == saucer_missiles
        assert fleets.ships.flyers == ships

I’m pretty much hating that test now. But I’ll keep it until I can legitimately remove it. I will comment out the asteroids call, since we’re getting rid of that accessor.

    def test_access(self):
        asteroids = ["asteroid"]
        missiles = ["missile"]
        saucers = ["saucer"]
        saucer_missiles = ["saucer_missile"]
        ships = ["ship"]
        fleets = Fleets(asteroids, missiles, saucers, saucer_missiles, ships)
        # removing accessors as part of decentralization
        # assert fleets.asteroids.flyers == asteroids
        assert fleets.missiles.flyers == missiles
        assert fleets.saucers.flyers == saucers
        assert fleets.saucer_missiles.flyers == saucer_missiles
        assert fleets.ships.flyers == ships

Now all the reference to the asteroids property are inside Fleets.

Let’s mark it private but keep it.

class Fleets:
    @property
    def _asteroids(self):
        return self.fleets[0]

One of the uses of _asteroids is has_asteroid, used just once:

class Asteroid(Flyer):
    def split_or_die(self, fleets):
        if not fleets.has_asteroid(self):
            return  # avoid low probability double kill
        fleets.remove_asteroid(self)
        self.explode()
        if self.size > 0:
            a1 = Asteroid(self.size - 1, self.position)
            fleets.add_asteroid(a1)
            a2 = Asteroid(self.size - 1, self.position)
            fleets.add_asteroid(a2)

We could remove this and take our chances on trying to split the same asteroid twice. If it happens, you get four smaller asteroids instead of just two. I think we’ll do that. If we see the problem we can resolve it by giving the asteroid its own alive-dead flag, which would be faster anyway.

We do have a test for the case:

    # it's barely possible for two missiles to kill the
    # same asteroid. This used to cause a crash, trying
    # to remove the same asteroid twice.
    def test_dual_kills(self):
        asteroid = Asteroid(2, Vector2(0, 0))
        asteroids = [asteroid]
        fleets = Fleets(asteroids)
        asteroid.split_or_die(fleets)
        assert asteroid not in asteroids
        assert len(asteroids) == 2
        asteroid.split_or_die(fleets)
        assert len(asteroids) == 2  # didn't crash, didn't split again

Remove the test, take that. Remove has_asteroid method. Commit: remove has_asteroid call, possibly allowing an asteroid to be killed twice. Odds strongly against it.

I think I owed myself a commit for those other tests. Oh well, they’re committed now.

Here’s a use of _asteroids that is troublesome:

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:
            fleet.tick(delta_time, self)

Ah. For a moment I wasn’t sure how we’d resolve this. We’ll resolve it by having a FlyingThumper that lives in the mix, counts asteroids and ships, and thumps as desired. Not to worry. Might be a next good thing to do, though.

I think we have in fact narrowed down the uses of _asteroids as much as we can without creating new flyers. And we have over 400 lines of article, where a line is actually a paragraph, fortunately short because most of my paragraphs are short. However you count it, we have enough for a morning’s work. I’ll take a break, and I recommend that you do the same.

Summary

We are pecking away at changing the program so that all the Asteroids-specific logic is in the Flyers. This is a process involving many many very small steps. And I’m doing it that way for a higher purpose, which is to demonstrate that often — if not always — we can accomplish even major design changes very incrementally. Aside from two mistakes quickly corrected, we’ve been at this deep architecture change for many steps, with more left to be taken, and we have not broken the program for more than a couple of minutes at a time.

I am not proud of that record. Zero would be a record to be proud of, and in both cases it was carelessness on my part that left the system broken for a few minutes. I am not perfect. I am fairly good. You have every possibility of being better at this than I am.

We progress, not quite in a straight line, but always in the general direction of our goal. That’s how you do that.

See you next time!

Added in Post

ARRGH!!!

As soon as I make a claim of having not broken the system with these refactorings, I find a problem. In the game, when I fire a missile at an asteroid, sometimes it kills the asteroid and flies on through, and sometimes it kills one of the fragments and flies on through. I’ve seen one missile kill as many as three asteroids before timing out.

I don’t think I added this problem today. I think it will have been there for a while. Let’s look at Missile vs Asteroid.

class Missile(Flyer):
    def interact_with_asteroid(self, asteroid, fleets):
        if asteroid.are_we_colliding(self.position, self.radius):
            self.die(fleets)

    def die(self, fleets):
        fleets.remove_missile(self)

class Fleets:
    def remove_missile(self, missile):
        self.missiles.remove(missile)

There’s yer problem rat there. When the ship fires a missile, it no longer goes into the missiles collection:

    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

We add the missile as a flyer, so we should remove it as a flyer.

    def remove_missile(self, missile):
        self.remove_flyer(missile)

Six tests fail. First, I test in game play to see if the fix works. It does.

To the tests:

    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.testing_only_score == 20
        assert len(asteroids) == 2

We know that the missiles fleet is no longer used. This test needs revision.

    def test_missile_asteroid_scores(self):
        fleets = Fleets()
        pos = Vector2(100, 100)
        asteroid = Asteroid(2, pos)
        fleets.add_asteroid(asteroid)
        missile = Missile.from_ship(pos, Vector2(0, 0))
        fleets.add_flyer(missile)
        fleets.add_scorekeeper(ScoreKeeper())
        interactor = Interactor(fleets)
        interactor.perform_interactions()
        interactor.perform_interactions()
        missiles = [m for m in fleets.all_objects if isinstance(m, Missile)]
        assert not missiles
        assert interactor.testing_only_score == 20
        asteroids = [a for a in fleets.all_objects if isinstance(a, Asteroid)]
        assert len(asteroids) == 2

That one runs green.

Some of the others are readily fixed but something has gone wrong. I better roll back.

We are green with the bug.

I think of an interim fix:

class Fleets:
    def remove_missile(self, missile):
        self.missiles.remove(missile)
        self.others.remove(missile)

Removes never fail, so I’ll just remove the missile from both missiles and others. Tests run green. Test in game. We are good.

Commit: fix bug allowing missiles not to die after use.

OK, that patch makes the game work and keeps the tests running. But why did the tests not catch the bug? Well, because for the tests, the remove code was removing from the missiles Fleet, where the tests expected it, but there was no test for missiles disappearing from others, which is what they really want to do.

I’ll add one test that tests how things really work.

    def test_missile_asteroid_scores_with_missiles_in_others(self):
        fleets = Fleets()
        pos = Vector2(100, 100)
        asteroid = Asteroid(2, pos)
        fleets.add_asteroid(asteroid)
        missile = Missile.from_ship(pos, Vector2(0, 0))
        fleets.add_flyer(missile)
        fleets.add_scorekeeper(ScoreKeeper())
        interactor = Interactor(fleets)
        interactor.perform_interactions()
        interactor.perform_interactions()
        missiles = [m for m in fleets.all_objects if isinstance(m, Missile)]
        assert not missiles
        assert interactor.testing_only_score == 20
        asteroids = [a for a in fleets.all_objects if isinstance(a, Asteroid)]
        assert len(asteroids) == 2

That’s the test I needed. It’s a couple of days ago now, so I’m not sure what I was thinking. I think probably the fact that I had all those interaction tests gave me more confidence than I deserved.

So, Rickard. Interestingly, it wasn’t lack of a high-level test that bit me, it was the lack of a lower-level one. Would a higher-level one have caught the problem? I’m not sure.

What I am sure of is that I have embarrassed myself again, but I’m used to it. I make mistakes. This was a serious one, that “should” have been caught yesterday or the day before.

I need one more thing, but not now. I need to do better than those list comprehensions to fetch things like asteroids. I think a wrapper for the Fleets might be the answer but I am tired, more tired than I was before I found this bug.

Enough. See you next time!