Python Asteroids+Invaders on GitHub

There’s at least one more place where the ExplosionMixin could be used. Should we use it? Let’s find out.

Our ExplosionMixin could rightly be called PlayerExplosionMixin right now:

class ExplosionMixin:
    def explode(self, fleets):
        frac = u.screen_fraction(self.position)
        player.play_stereo("explosion", frac)
        explosion = GenericExplosion.player_explosion(self.position, 1.0)
        fleets.append(explosion)
        fleets.remove(self)

It’s only used by our three Player classes, InvaderPlayer, ReservePlayer, and RobotPlayer. Its sound and explosion are specific to the Player-shaped objects. We do have at least one other explosion that is similar:

class InvadersSaucer(SpritelyMixin, InvadersFlyer):
    def explode_scream_and_die(self, fleets):
        explosion = GenericExplosion.saucer_explosion(self.position, 0.5)
        fleets.append(explosion)
        fleets.append(InvaderScore(self._mystery_score()))
        self.play_death_sound()
        self._die(fleets)

    def play_death_sound(self):
        frac = (self.position.x - u.INVADER_SAUCER_X_MIN) / (u.INVADER_SAUCER_X_MAX - u.INVADER_SAUCER_X_MIN)
        player.play_stereo("ufo_highpitch", frac, True)

    def _die(self, fleets):
        fleets.remove(self)

This is “similar” in a very odd sense, isn’t it? It uses a different explosion and a different sound. And, right now, it doesn’t really even look that similar. Let’s do a little inlining and rearranging. But first, let me digress to discuss why I want to do that, because clearly we are smart enough to work from here.

Ron, why do you make things more similar when you know you’re just going to refactor them into an entirely different shape in just a moment?

I think it has to do with physical feelings, which are driven by mental / emotional feelings. When I am coding something that is tricky and that needs to be done all in one go or it won’t work, I often feel tension, usually in my neck. Sometimes I even notice that I’m feeling jittery. My concern, of course, is that it’s not going to work, someone will interrupt me and knock down my house of cards, or I’ll forget some detail and then something will break and then I’ll have to fix it and I might even have to back up. Oh, the horror!

When we are removing duplication, the closer the starting code looks to the other duplicates, the easier it is to see what needs to be parameterized or otherwise modified to make the change. The individual steps never break anything—well, almost never—well, hardly ever—and when they do, if it was just a tiny one-line change, it’s easy to undo and do it right. The tension tells me that I’m doing something that I consider risky, and the more sensitive to those feelings I allow myself to get, the smaller are the steps I take, and the that I do make are smaller and more transparent.

That’s why I do this. Now, let’s do it.

We’ll inline the sound and die methods:

    def explode_scream_and_die(self, fleets):
        explosion = GenericExplosion.saucer_explosion(self.position, 0.5)
        fleets.append(explosion)
        fleets.append(InvaderScore(self._mystery_score()))
        frac = (self.position.x - u.INVADER_SAUCER_X_MIN) / (u.INVADER_SAUCER_X_MAX - u.INVADER_SAUCER_X_MIN)
        player.play_stereo("ufo_highpitch", frac, True)
        fleets.remove(self)

This cannot fail, I used machine refactorings. Commit: refactoring toward ExplosionMixin.

Now rearrange some lines to make these two things more alike:

class ExplosionMixin:
    def explode(self, fleets):
        frac = u.screen_fraction(self.position)
        player.play_stereo("explosion", frac)
        explosion = GenericExplosion.player_explosion(self.position, 1.0)
        fleets.append(explosion)
        fleets.remove(self)


class InvadersSaucer(SpritelyMixin, InvadersFlyer):
    def explode_scream_and_die(self, fleets):
        explosion = GenericExplosion.saucer_explosion(self.position, 0.5)
        fleets.append(explosion)
        fleets.append(InvaderScore(self._mystery_score()))
        frac = (self.position.x - u.INVADER_SAUCER_X_MIN) / (u.INVADER_SAUCER_X_MAX - u.INVADER_SAUCER_X_MIN)
        player.play_stereo("ufo_highpitch", frac, True)
        fleets.remove(self)

Rearranging the latter method, we get:

    def explode_scream_and_die(self, fleets):
        fleets.append(InvaderScore(self._mystery_score()))
        frac = (self.position.x - u.INVADER_SAUCER_X_MIN) / (u.INVADER_SAUCER_X_MAX - u.INVADER_SAUCER_X_MIN)
        player.play_stereo("ufo_highpitch", frac, True)
        explosion = GenericExplosion.saucer_explosion(self.position, 0.5)
        fleets.append(explosion)
        fleets.remove(self)

Commit, same message.

Let’s use u.screen_fraction in our current method:

    def explode_scream_and_die(self, fleets):
        fleets.append(InvaderScore(self._mystery_score()))
        frac = u.screen_fraction(self.position)
        player.play_stereo("ufo_highpitch", frac, True)
        explosion = GenericExplosion.saucer_explosion(self.position, 0.5)
        fleets.append(explosion)
        fleets.remove(self)

Commit again, same message.

Extract all but the score part to a new method still in InvadersSaucer:

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

    def explode_saucer(self, fleets):
        frac = u.screen_fraction(self.position)
        player.play_stereo("ufo_highpitch", frac, True)
        explosion = GenericExplosion.saucer_explosion(self.position, 0.5)
        fleets.append(explosion)
        fleets.remove(self)

Move the method to ExplosionMixin, remove it from InvadersSaucer, and inherit the mixin;

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

class ExplosionMixin:
    def explode(self, fleets):
        frac = u.screen_fraction(self.position)
        player.play_stereo("explosion", frac)
        explosion = GenericExplosion.player_explosion(self.position, 1.0)
        fleets.append(explosion)
        fleets.remove(self)

    def explode_saucer(self, fleets):
        frac = u.screen_fraction(self.position)
        player.play_stereo("ufo_highpitch", frac, True)
        explosion = GenericExplosion.saucer_explosion(self.position, 0.5)
        fleets.append(explosion)
        fleets.remove(self)

Commit: InvadersSaucer uses ExplosionMixin to explode.

But now we have duplication in ExplosionMixin, and we would also like to rename the explode method.

First that. Rename to explode_player. I rename it, but need to do it by hand, because there are calls to explode outside our player space. The dark side of duck typing. Kotlin would have known which ones to change.

That was easily done. I do some in game testing but am confident, Commit: rename method.

Now the duplication:

    def explode_player(self, fleets):
        frac = u.screen_fraction(self.position)
        player.play_stereo("explosion", frac)
        explosion = GenericExplosion.player_explosion(self.position, 1.0)
        fleets.append(explosion)
        fleets.remove(self)

    def explode_saucer(self, fleets):
        frac = u.screen_fraction(self.position)
        player.play_stereo("ufo_highpitch", frac, True)
        explosion = GenericExplosion.saucer_explosion(self.position, 0.5)
        fleets.append(explosion)
        fleets.remove(self)

I could do this by hand, but if I use machine refactoring it is safer. Extract some variables:

    def explode_player(self, fleets):
        player_explosion_sound = "explosion"
        explosion = GenericExplosion.player_explosion(self.position, 1.0)
        frac = u.screen_fraction(self.position)
        player.play_stereo(player_explosion_sound, frac)
        fleets.append(explosion)
        fleets.remove(self)

    def explode_saucer(self, fleets):
        saucer_explosion_sound = "ufo_highpitch"
        explosion = GenericExplosion.saucer_explosion(self.position, 0.5)
        frac = u.screen_fraction(self.position)
        player.play_stereo(saucer_explosion_sound, frac, True)
        fleets.append(explosion)
        fleets.remove(self)

Now here is a perfect example of the reason why I make things look similar. I am finally noticing the True on the call to play_stereo. A quick check of the method tells me that the variable multi_channel defaults to True. So I can remove that difference. In this case, had I not noticed, it would have been OK, but in another situation it might not have been. Making things look the same highlights key differences and gives me a better chance of noticing.

Remove that. Commit: making duplication.

Extract Method.

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

Test, commit: remove duplication via ExtractMethod.

Let’s reflect and sum up.

Retrospective

Through a chain of machine refactorings and line moving operations, we have removed a rather strange-looking method from InvadersSaucer and used the ExplosionMixin instead. Along the way, we had to do one manual edit, where we changed calls to explode to calls to explode_player, because we had other methods named explode, and I didn’t trust PyCharm to know which ones to change.

That step was error-prone. If we forgot one, PyCharm would have warned us, and I believe that some tests would have failed. But if we had done an extra one, there might have been trouble. This is a fairly common effect in refactoring Python and similar duck-typing languages: renames are sometimes quite terribly bad. That makes us less inclined to rename things, which is wise on the one hand but leaves the code in somewhat inferior condition than it might have been. Definitely a check mark on the more strict language side of the ledger.

We created very similar code to the code in ExplosionMixin. We moved it to ExplosionMixin and called it there. Then we observed the duplication there, isolated it by extracting some variables, and used Extract Method refactoring to remove the duplication. PyCharm found both occurrences, by the way, and did both extracts mechanically for me. I just had to nod and tell it OK.

We might ask ourselves, selves, is this really better? The method didn’t originally look all that similar, why did we make i more similar and then fix it? Why not just leave it alone, bothering no one?

I think the answer is something like this:

Similar things should be done in similar ways. Things that are sufficiently similar tell us that there may be some idea in our mind that is not well-expressed in the code. Bringing similar things together helps us identify and express that idea. Later on, when we read this code, or when someone else reads this code, consolidating similar ideas will help them to understand the code and to be better prepared to modify it.

In short, making similar things more similar and then reducing the resulting duplication makes the program easier to understand.

Would I do this every time in a real product situation? I might not, but I would try to take every reasonable opportunity to do so, because, by my lights, it has a really good chance of making the code better.

YMMV, of course. I’m just here to show you what I do, try to explain why I do it, and to let you see what happens.

In this case, I rather like it. I hope that you do as well.

See you next time!