Python Asteroids+Invaders on GitHub

I think there’s maybe one more explosion up in this program. Let’s see if we should add it to our mixin.

I had the surprise of noticing a toot yesterday, where someone was raving that “overengineering” is a big problem in our industry, done in the name of “best practices” and claiming benefits in maintainability and testability that are never validated.

The post referred to “hundreds of lines of code where a dozen would do”. I’m certainly not in favor of hundreds of lines of code in any form, certainly not when a dozen would do as well. I chose not to engage the OP, but I would almost like to see some of the “over engineered” code to which they referred. Most of the teams I’ve worked with were definitely not putting too much “quality” into their code, nor were they testing too much. Perhaps OP and I are having different experiences.

That said, I would be open to the idea that our Asteroids / Invaders program might be over-engineered. Since we know that the original programs were tiny and written in assembler, we can be certain that we have too much mechanism if all we were doing was writing one or the other of those games, and probably too much mechanism even if we wanted to have a single program that could play both of the games.

We are here to look at some ordinary code and see how we might improve it, as if it were part of a large software effort in a real software organization. We are doing things here that hardly pay off in the small program we have, just to see their effect, so that we can better decide what we want to do in the large. Well, really so that you can better decide. I myself do not plan to go back into the larger software world.

It seems to me that understanding a program is easier when the same kinds of things are done in the same kinds of ways, and that when the code for two things is similar but different, there is a reason for the difference. It should be the case that the two different things need to be different.

If I’m correct that programs are easier to understand when similar things are done in similar ways, and I’d win that debate, then there is some value to making things the same when they can be. It may not always pay off and it may not always be worth doing, but if it were free, the program would be better.

So we look for duplication in these articles and we explore how easy or hard it is to remove it. Why? Because if is often quite easy, and, in my view, it generally pays off. I could be wrong, even here chez Ron, and certainly could be wrong about your situation. That’s why I don’t tell you what to do, I just show you what I do, and what happens.

Today, I think we have another explosion to look at. When a player shot hits an invader, the invader explodes. Let’s see how that’s done.

class Invader(SpritelyMixin):
    def interact_with_group_and_playershot(self, shot, group, fleets):
        if self.colliding(shot):
            player.play_stereo("invaderkilled", u.screen_fraction(self.position))
            shot.hit_invader(self, fleets)
            group.kill(self)
            fleets.append(InvaderScore(self._score))
            explosion = GenericExplosion.invader_explosion(self.position, 0.125)
            fleets.append(explosion)

Recall what our current mixin looks like:

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)

We see some similarity between the Invader code and the mixin … and an important difference! Instead of the Invader removing itself from fleets, if tells the group to kill it. Why? Because Invader instances are not in the fleets mix, because they need to be managed as a group (and because I felt it would be wasteful to add 2500+ interactions to the main loop). Now it turns out that removing something that isn’t in the mix is harmless:

class Fleets:
    def remove(self, flyer):
        # more pythonic?
        try:
            self.flyers.remove(flyer)
        except ValueError:
            pass

So we can, with impunity, go ahead and remove the Invader from fleets. But it would be a bit tacky, wouldn’t it?

Let’s see about making the Invader code more like the standard.

Extract Variable:

    def interact_with_group_and_playershot(self, shot, group, fleets):
        if self.colliding(shot):
            frac = u.screen_fraction(self.position)
            player.play_stereo("invaderkilled", frac)
            shot.hit_invader(self, fleets)
            group.kill(self)
            fleets.append(InvaderScore(self._score))
            explosion = GenericExplosion.invader_explosion(self.position, 0.125)
            fleets.append(explosion)

Move some lines:

    def interact_with_group_and_playershot(self, shot, group, fleets):
        if self.colliding(shot):
            frac = u.screen_fraction(self.position)
            player.play_stereo("invaderkilled", frac)
            explosion = GenericExplosion.invader_explosion(self.position, 0.125)
            fleets.append(explosion)
            shot.hit_invader(self, fleets)
            group.kill(self)
            fleets.append(InvaderScore(self._score))

Implement a new explosion method in 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)

Call it:

    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))

Tests break. Add the mixin to the class definition.

class Invader(ExplosionMixin, SpritelyMixin):

Tests run again. Game works. Commit: Invader uses ExplosionMixin.

Remove two unused import lines. Commit again: tidying imports.

Reflection

So that went easily, didn’t it? Was it over-engineering? Not by my lights: YMMV. There is the minor matter of the fleets.remove(self), which is harmless but in principle should not be done. We could improve that. I can think of two ways:

First, we might add a flag to the make_the_explosion method and condition the remove with it. Second, we could remove the fleets.remove from the make... method, and add it to the individual explode_whatever methods. I think the latter is a poor idea because we could easily forget to do it. I don’t think the former is worth it, since removing something that is not present is intentionally benign.

A third1 approach, and you could possibly talk me into this one, would be not to do the remove in the mixin at all, requiring users of the mixin to do their own removal, by whatever means they might choose. I think that would have been a perfectly good idea, arguably better than what we have.

Why better? Because as it stands, the mixin really does two things. It adds an explosion with sound and visual effects, and it removes the object. I guess it’s convenient that it removes the object, and we could think of it as “replace an object with its explosion” to make it seem cohesive, but it’s perilously close to doing two different things, and we mostly prefer objects with single responsibilities.

What are we going to do about that? Nothing, because what we have now is better than what we had when we came in, and a step toward better is enough to satisfy us. We’re not trying to “over engineer”: we’re just making the code a little bit better.

I am noticing something that concerns me a bit more than the issues above. The ExplosionMixin now has three “public” methods, explode_invader, explode_saucer, and explode_player. The first two are used once each, while explode_player is used six times in the game and once in a test. When I was thinking that the mixin would just have one method, like explode, it seemed to me that we would all quickly learn that the method was in the mixin. But as it stands now, there are those three methods, and a given class uses only one of them.

We might want to reconsider whether a mixin is the best way to do this thing. Not today: I’m sure it’s better than what we had. But a developer’s skill is in making decisions among options, and we might just look at some options later on.

Summary

We have made the Invader code just a bit more expressive, by enhancing our mixin and deferring another explosion to it. We did add a couple of lines of code: a method definition line and a call to the method. But in so doing, we moved some complexity out of one of Invader’s methods and put it over in the mixin, where it isn’t seen as complex at all: it follows the same pattern as all the rest.

I think that’s a good thing. And I suspect we can do better than the mixin. Can you think what I might be considering? What options might you be considering?

See you next time!


  1. As almost always happens, as soon as I say how many ideas I have, I get another one. ↩