Python Asteroids on GitHub

The double dispatch is doing its job. Let’s see about cleaning up the references to fleets and whatever else comes to mind.

Part of my cunning plan is to change code like this not to say fleets.asteroids:

class Asteroid:
    def interact_with_ship(self, ship, fleets):
        self.split_or_die(fleets.asteroids)

    def split_or_die(self, asteroids):
        if self not in asteroids:
            return # already dead
        asteroids.remove(self)
        self.explode()
        if self.size > 0:
            a1 = Asteroid(self.size - 1, self.position)
            asteroids.append(a1)
            a2 = Asteroid(self.size - 1, self.position)
            asteroids.append(a2)

The thing is, we shouldn’t be pulling the asteroids collection out of fleets. There is a general guideline about this sort of thing, referred to as “tell, don’t ask”, or as the Law of Demeter. What we have here is a perfect example of why we should “never” do this sort of thing.

Our longer-range plan is that Fleets class will no longer have separate collections of asteroids and missiles and saucers and ships and fragments, it will just have one collection of everything that can occur. Code like we have above gets in the way of changes like that one, and it’s best never to get in this position.

Of course, I am merely human, as far as you know, and I don’t always do what I “should”. Sometimes I don’t even notice when I’m not doing as I “should”. Anyway our current plan goes like this:

  1. As we make our changes to this design, we’ll replace code like the above with specialized calls to methods like add_asteroid and remove_asteroid, which will use the special collections for now and then later …

  2. We’ll funnel all those methods, all nicely hidden inside Fleets class so that they all go to the same collection, and then later …

  3. We’ll replace all the add_special_thing methods with just add.

I’m not going to test-drive these new methods, because if they don’t exist the game won’t compile, and if they access the wrong collections, I’m sure some existing test will break.

Of course, when we make change number two, we will have some tests to fix up, but that should be easy enough.

Therefore:

I have five occurrences of calls to split_or_die passing in an asteroids collection. In the case of the Asteroid class, I’ll just pass fleets instead, like this:

class Asteroid:
    def score_for_hitting(self, attacker):
        return attacker.scores_for_hitting_asteroid()[self.size]

    def split_or_die(self, fleets):
        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)

This demands those two method on Fleets.

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

    def remove_asteroid(self, asteroid):
        self.asteroids.remove(asteroid)

One test is failing because it’s not following the new rule:

    def test_dual_kills(self):
        asteroid = Asteroid(2, Vector2(0, 0))
        asteroids = [asteroid]
        asteroid.split_or_die(asteroids)
        assert asteroid not in asteroids
        assert len(asteroids) == 2
        asteroid.split_or_die(asteroids)
        assert len(asteroids) == 2  # didn't crash, didn't split again

We’ll have to chuck in a Fleets instance here.

Interesting. The fixed test still fails. Let me include the test’s comment this time because it’s telling us why:

    # 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

There was an early out in split_or_die, as you can see if you look back, and I took it out, blindly thinking that it was just there to be sure that we don’t do a double delete. In fact, we’re trying to avoid a single asteroid trying to split twice.

Nothing for it, I think, but to ask:

    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)

And:

class Fleets:
    def has_asteroid(self, asteroid):
        return asteroid in self.asteroids

I don’t like this. Why? Because it is a question from a single object, one asteroid, to the universe of objects, presently called Fleets. Our planned design calls for our objects to be independent of the game (and the universe), instead gleaning the information they need from their interactions.

I think we’ll surely encounter this code again, when we combine all the fleet instances inside Fleet, so let me just put a comment in here.

    def has_asteroid(self, asteroid):
        # this code violates the decentralized design
        # by asking a question of the Fleet.
        # Fix it up when we fold the fleet instances together.
        return asteroid in self.asteroids

We’re green. Commit: asteroid uses fleets.add_ and remove_asteroid in split_or_die.

Aside
I am having a lot of fun here, pushing this code around like clay, making the program have more and more the shape I have in mind, supported well enough by tests that I can confidently do so.

Missile

Let’s move on to this case:

class Missile:
    def interact_with(self, attacker, _missiles, fleets):
        attacker.interact_with_missile(self, fleets)

    def interact_with_asteroid(self, asteroid, fleets):
        fleets.missiles.remove(self)

    def interact_with_missile(self, missile, fleets):
        pass

    def interact_with_saucer(self, saucer, fleets):
        fleets.missiles.remove(self)

    def interact_with_ship(self, ship, fleets):
        fleets.missiles.remove(self)

It’s clear enough what we want here, isn’t it? Well, not quite, because while we could just edit each of those, there is a good reason not to. We will need to be checking for an actual collision, so we should extract a method here. And, I think you could argue that we should anyway, because this code says how to do the thing but not what the thing is.

Extract:

    def interact_with_ship(self, ship, fleets):
        self.die(fleets)

    def die(self, fleets):
        fleets.missiles.remove(self)

I let PyCharm replace all the calls, of course. We’re green, shall we commit? Sure. Commit: extract die method. Always commit when you can, first one into the pool saves a merge with others.

Now:

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

Tests fail all around of course, so we:

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

Green. Commit: missile collision uses remove_missile.

Should we look for other calls to Fleets.missiles? I’m sure there are some, for example when the ship or saucer fire a missile. Let’s hold off, we’ll continue with our cleaning of the interact stuff.

Saucer already has the desired method extracted:

class Saucer:
    def explode(self, fleets):
        player.play("bang_large", self._location)
        player.play("bang_small", self._location)
        fleets.saucers.remove(self)

Change:

    def explode(self, fleets):
        player.play("bang_large", self._location)
        player.play("bang_small", self._location)
        fleets.remove_saucer(self)

Tests break. Add the method. Just for fun, I change the method to this:

    def explode(self, fleets: Fleets):
        player.play("bang_large", self._location)
        player.play("bang_small", self._location)
        fleets.remove_saucer(self)

After importing Fleets, PyCharm tells me from here that the method doesn’t exist, and offers to add it. I’ll allow it, though I bet it puts it at the end of the class, but it’ll still at least find the right file for me.

Meh. Recursive import. Change it back, but the method is in Fleets now.

class Saucer:
    def explode(self, fleets):
        player.play("bang_large", self._location)
        player.play("bang_small", self._location)
        fleets.remove_saucer(self)

class Fleets:
    def remove_saucer(self, saucer):
        self.saucers.remove(saucer)

Green. Commit: saucer now uses remove_saucer.

There is probably some clever way to avoid that recursive import but removing the reference worked. Python should be more reasonable about imports, if they expect type hints to catch on.

Now Ship:

class Ship:
    def explode(self, ship_fleet, fleets):
        player.play("bang_large", self._location)
        if self in ship_fleet: ship_fleet.remove(self)
        fleets.explosion_at(self.position)

First do the standard change:

    def explode(self, ship_fleet, fleets):
        player.play("bang_large", self._location)
        fleets.remove_ship(self)
        fleets.explosion_at(self.position)

Add the method all by my own self:

class Fleets:
    def remove_ship(self, ship):
        self.ships.remove(ship)

Green. Commit: ship explode uses remove_ship. However, we have a parameter we don’t use in explode. Let’s change signature.

    def explode(self, fleets):
        player.play("bang_large", self._location)
        fleets.remove_ship(self)
        fleets.explosion_at(self.position)

PyCharm does it right! Well done, PyCharm!

Commit: remove extra parameter from ship.explode.

Let’s sum up.

Summary

In a few easy edits we have reduced the system’s knowledge of how Fleets class works, instead focusing the code on what Fleets can do, which is add and remove things. This means that less code in the system is dependent on the current implementation of Fleets. This is a good thing. It’s the sort of thing that we might not do without a reason but that probably pays off enough to make it worth-while to have done it this way the first time.

You’ll recall, however, that we started with raw collections of asteroids and all that jazz, then moved things inside Fleet instances and then those inside the Fleets object, so we probably already had raw references all over.

The most nearly right thing, and I know it well, is always to wrap native collections with meaningful collections as soon as they start to have meaning. The sooner we wrap, the more clear our code becomes.

I think next time we’ll search and destroy other references to the Fleets’ individual collection properties, directing all references inside. Unless I get a more interesting idea before then.

See you next time!