Choices

Python Asteroids+Invaders on GitHub

Let’s see about this explosion mixin. Now that we’ve sat on it for a few days, so the glitter has worn off, do we still love it?

The point of a mixin, it seems to me, is to allow a class to plug in and “inherit” a bit of code that it has in common with other classes. The mixin can be thought of as providing some advantages over alternatives:

1. It reduces duplication, which we always prefer;
2. It provides a common name for an idea that is common among multiple classes;
3. It centralizes common code, which gives a better chance that if it needs changing, we’ll only have to change one copy;
4. It uses inheritance, but is not an “is-a” relationship, because “is_a” can often lead to problems and conflicts from full-on multiple inheritance;
5. It explicitly expresses that it is a mixin, if we name things carefully;
6. It simplifies the classes that use it;

There are, however, other ways of centralizing and standardizing common code:

1. Free-standing top-level functions can be used, at least in a language like Python;
2. A separate class can be created to support the desired capability;
3. Inherited concrete behavior can be put in a true superclass.

Perhaps you’ve thought of others. If so, let me know: I’d be happy to have more examples for this admittedly small issue.

In our present case, ExplosionMixin, we do have common behavior, but we also have three ways of calling that common behavior:

class ExplosionMixin:
    def explode_invader(self, fleets):
        invader_explosion_sound = "invaderkilled"
        explosion = GenericExplosion.invader_explosion(self.position, 0.125)
        self._make_the_explosion(invader_explosion_sound, explosion, fleets)

    def explode_player(self, fleets):
        player_explosion_sound = "explosion"
        explosion = GenericExplosion.player_explosion(self.position, 1.0)
        self._make_the_explosion(player_explosion_sound, explosion, fleets)

    def explode_saucer(self, fleets):
        saucer_explosion_sound = "ufo_highpitch"
        explosion = GenericExplosion.saucer_explosion(self.position, 0.5)
        self._make_the_explosion(saucer_explosion_sound, explosion, fleets)

    def _make_the_explosion(self, player_explosion_sound, explosion, fleets):
        frac = u.screen_fraction(self.position)
        player.play_stereo(player_explosion_sound, frac)
        fleets.append(explosion)
        fleets.remove(self)

The actual common behavior is in the final private method, _make_the_explosion, while there are three separate methods that provide the parameters used for specific explosions.

This means that our user classes, InvadersPlayer, ReservePlayer, RobotPlayer, InvadersSaucer, and Invader make one of three possible calls into the mixin. The common aspect is a bit hidden, as they all use a different name, so it is even less clear than it might be that they are all using the mixin.

What might we do about this? Suppose we wanted each of those five classes to make it more clear that they were using common code to get their explosions made. How might we do it?

Option: Require specific members

We could say that the class (or instance) using our mixin must implement, say, _explosion_key, and refer to those members in a single common explode method. We do something a bit similar with the SpritelyMixin, which does expect its user classes to implement _sprite, or to override the sprite property somehow. The users all just implement _sprite.

Code like this:

class InvadersSaucer(ExplosionMixin, SpritelyMixin, InvadersFlyer):
    def explode_scream_and_die(self, fleets):
        fleets.append(InvaderScore(self._mystery_score()))
        self.explode_saucer(fleets)

Might change to something like this:

class InvadersSaucer(ExplosionMixin, SpritelyMixin, InvadersFlyer):
_explosion_key = "saucer"
    def explode_scream_and_die(self, fleets):
        fleets.append(InvaderScore(self._mystery_score()))
        self.explode(fleets)

This would require some changes to sound names, and perhaps some different kind of lookup in GenericExplosion. ANd it would be slightly more error-prone if the keys didn’t match up.

Option: Require key parameter

A somewhat simpler approach would be to pass a key as part of the call to a common explode method, something like this:

class InvadersSaucer(ExplosionMixin, SpritelyMixin, InvadersFlyer):
    def explode_scream_and_die(self, fleets):
        fleets.append(InvaderScore(self._mystery_score()))
        self.explode("saucer", fleets)

This has the same concern as the preceding idea, in that the key might not be present in the mixin’s lookup table.

Option: Use object class rather than key

We could have a common explode method in EplosionMixin that uses the class of the object to look up the sound and explosion. This would make it easier to use the mixin, but the issue remains that the table of sound and image might not contain the desired class. In this case, and the preceding two, we could provide a standard default explosion and void any crashes. Or we could just exit without playing a sound or displaying an explosion at all.

Option: Make the mixin a helper class

We could change the Mixin to a free-standing class, implement the current methods as class methods and go from there. The user code would make more sense:

class InvadersSaucer(SpritelyMixin, InvadersFlyer):
_explosion_key = "saucer"
    def explode_scream_and_die(self, fleets):
        fleets.append(InvaderScore(self._mystery_score()))
        Exploder.explode_saucer(fleets)

Reflection

Having laid out those options, I have to say that I much prefer the last one. I prefer it so much that I propose that we refactor to that state and get rid of the mixin.

Would you really do this?

I might not. I do think this will be better, by my lights, but once the mixin is working, it probably doesn’t improve things much to make the upcoming change. On the other hand, I think we’ll find it rather easy to do, and that it will make both the code that uses the new class, and the code for the new class itself, notably more clear.

Let’s see if we can refactor our way to this with minimal breakage.

First, in the same file as the ExplosionMixin, we’ll implement the new class and use it. I start with this:

class Exploder():
    def explode(self, position, sound, explosion, fleets):
        frac = u.screen_fraction(position)
        player.play_stereo(sound, frac)
        fleets.append(explosion)
        fleets.remove(self)

Note:

Doing it in the same file was supposed to save me time. It does not, as you’ll see below.

As soon as I typed that in, it became clear that we’ll need the position. That’s no real surprise, but I hadn’t thought about it. At this moment, our new class is unused.

We can use it right in the mixin, so that our real user classes don’t get troubled until we have things sorted out:

    def explode_invader(self, fleets):
        invader_explosion_sound = "invaderkilled"
        explosion = GenericExplosion.invader_explosion(self.position, 0.125)
        Exploder().explode(self.position, invader_explosion_sound, explosion, fleets)

That works just fine. But I have in mind using class methods in the new class, so we can now refactor to this:

class ExplosionMixin:
    def explode_invader(self, fleets):
        Exploder.explode_invader(self.position, fleets)

class Exploder():
    @classmethod
    def explode_invader(self, position, fleets):
        invader_explosion_sound = "invaderkilled"
        explosion = GenericExplosion.invader_explosion(position, 0.125)
        Exploder().explode(position, invader_explosion_sound, explosion, fleets)

I’m not sure that I love the Exploder implementation but it works solidly. Replicate for the other two cases:

class Exploder():
    @classmethod
    def explode_invader(self, position, fleets):
        invader_explosion_sound = "invaderkilled"
        explosion = GenericExplosion.invader_explosion(position, 0.125)
        Exploder().explode(position, invader_explosion_sound, explosion, fleets)

    @classmethod
    def explode_player(self, position, fleets):
        player_explosion_sound = "explosion"
        explosion = GenericExplosion.player_explosion(position, 1.0)
        Exploder().explode(position, player_explosion_sound, explosion, fleets)

    @classmethod
    def explode_saucer(self, position, fleets):
        saucer_explosion_sound = "ufo_highpitch"
        explosion = GenericExplosion.player_explosion(position, 1.0)
        Exploder().explode(position, saucer_explosion_sound, explosion, fleets)

    def explode(self, position, sound, explosion, fleets):
        frac = u.screen_fraction(position)
        player.play_stereo(sound, frac)
        fleets.append(explosion)
        fleets.remove(self)


class ExplosionMixin:
    def explode_invader(self, fleets):
        Exploder.explode_invader(self.position, fleets)

    def explode_player(self, fleets):
        Exploder.explode_player(self.position, fleets)

    def explode_saucer(self, fleets):
        Exploder.explode_saucer(self.position, fleets)

Something broke a bunch of tests. I went too fast. Ah. In fact I think something must have failed earlier without my noticing. Anyway, I can’t remove the objects in the Exploder: it does not have them. Quick fix is this:

class ExplosionMixin:
    def explode_invader(self, fleets):
        Exploder.explode_invader(self.position, fleets)
        fleets.remove(self)

    def explode_player(self, fleets):
        Exploder.explode_player(self.position, fleets)
        fleets.remove(self)

    def explode_saucer(self, fleets):
        Exploder.explode_saucer(self.position, fleets)
        fleets.remove(self)

We spoke this morning about the remove being a bit naff. Let’s assume that we will do without it in Exploder, putting it more where it belongs, in the class that wants to be removed.

I could revert, but decide to keep on moving forward.

We can now go to each of the users of our mixin and inline their call:

    def interact_with_group_and_playershot(self, shot, group, fleets):
        if self.colliding(shot):
            self.explode_invader(fleets)
            shot.hit_invader(self, fleets)
            group.kill(self)
            fleets.append(InvaderScore(self._score))

Inline to:

    def interact_with_group_and_playershot(self, shot, group, fleets):
        if self.colliding(shot):
            Exploder.explode_invader(self.position, fleets)
            fleets.remove(self)
            shot.hit_invader(self, fleets)
            group.kill(self)
            fleets.append(InvaderScore(self._score))

In this case, we don’t want the remove, so:

    def interact_with_group_and_playershot(self, shot, group, fleets):
        if self.colliding(shot):
            Exploder.explode_invader(self.position, fleets)
            shot.hit_invader(self, fleets)
            group.kill(self)
            fleets.append(InvaderScore(self._score))

Continue to find the other users. I’m just going to the method in the mixin and Command+B to find senders. Better yet … will it inline them all from the mixin?

It will:

    def explode_scream_and_die(self, fleets):
        fleets.append(InvaderScore(self._mystery_score()))
        Exploder.explode_saucer(self.position, fleets)
        fleets.remove(self)

The big one is the Player one, with seven callers. PyCharm seems willing and does the job. Tests remain green. Can I do a safe delete on the mixin? Not as things stand, since I put the Exploder class in the same file. Easily done just search and remove by hand.

Note Followup:

It would have been smarter to have put Exploder in its own file, then I could have deleted the mixin file and I think PyCharm would have been more helpful. No real harm done, just a bit more work and a slightly higher chance of getting it wrong.

Commit: replace ExplosionMixin with Exploder class.

Let’s review where we are. I make some slight changes first:

class Exploder():
    @classmethod
    def explode_invader(cls, position, fleets):
        invader_explosion_sound = "invaderkilled"
        explosion = GenericExplosion.invader_explosion(position, 0.125)
        cls.explode(position, invader_explosion_sound, explosion, fleets)

    @classmethod
    def explode_player(cls, position, fleets):
        player_explosion_sound = "explosion"
        explosion = GenericExplosion.player_explosion(position, 1.0)
        cls.explode(position, player_explosion_sound, explosion, fleets)

    @classmethod
    def explode_saucer(cls, position, fleets):
        saucer_explosion_sound = "ufo_highpitch"
        explosion = GenericExplosion.saucer_explosion(position, 0.5)
        cls.explode(position, saucer_explosion_sound, explosion, fleets)

    @classmethod
    def explode(cls, position, sound, explosion, fleets):
        frac = u.screen_fraction(position)
        player.play_stereo(sound, frac)
        fleets.append(explosion)

I’ve done everything on the class side here. I’m not sure that I prefer that, and it is something that, sometime in my past, I learned was generally not a good idea. But it works nicely for now. Commit: Exploder is all class-side code. Questionable?

Our users all look like this now:

    def explode_scream_and_die(self, fleets):
        fleets.append(InvaderScore(self._mystery_score()))
        Exploder.explode_saucer(self.position, fleets)
        fleets.remove(self)

They refer to Exploder, which is more explicit than the prior mixin code, and points directly to the class tat implements the method. The user classes do not rely on Exploder to remove the thing exploding: that is left to the thing itself. And the Exploder, like the mixin before it, knows what all the explosions should look and sound like.

What’s not to like? Well, the object has all its code on the class side. We could change that, up to and including passing in the position, sound, and explosion in a constructor and then calling explode in the class methods. Or we could balance things a little differently.

Be that as it may, all the weirdness is now encapsulated in Exploder, as it was in ExplosionMixin, and the code is a bit more what we’re used to, with real classes doing things.

I think I like this better than the mixin, although I certainly don’t regret the mixin, both because we learned a bit about mixins and because the mixin was a good step toward consolidating the common elements of exploding.

One thing we might not like is that the relationship between a thing and its explosion is represented in specific separate bits of code, and it might make more sense to have a compact definition of an object that included all its shapes and sounds and such. There are ideas there that are not well expressed in the code. Would it be better if they were expressed? Almost certainly. Does that mean we should do it? Quite likely not, but there could be advantages.

I wonder … could we perhaps get some substantial code compression by factoring out differences in display and sound and explosion style and such? Is there some way of really collapsing our current code down? That would be really quite fine, to discover a substantial code reduction at this late date. I don’t know if it’s possible or not … but it’s worth thinking about!

One of the “big” lessons in all this is that we can improve the design of a running program in very small and safe steps, which will enable us to keep the code alive for longer, rather than nailing random chunks onto it. That’s the main lesson to take home from all these small changes: small changes are easy, and they can make the program substantially better.

And that’s the truth, as far as I know.

See you next time!