Mixins? Apparently Not.

The morning goes in a completely different direction than I expected. A good direction, though.

We’ll turn our attention this morning to the nearly duplicated code for creating an explosion on the Invaders screen. We have these cases, at least:

class ReservePlayer(Spritely, InvadersFlyer):
    def explode(self, fleets):
        frac = u.screen_fraction(self.position)
        player.play_stereo("explosion", frac)
        fleets.append(PlayerExplosion(self.position))
        fleets.remove(self)

class InvaderPlayer(Spritely, InvadersFlyer):
    def explode(self, fleets):
        frac = u.screen_fraction(self.position)
        player.play_stereo("explosion", frac)
        fleets.append(PlayerExplosion(self.position))
        fleets.remove(self)

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

class RobotPlayer(Spritely, InvadersFlyer):
    def explode(self, fleets):
        frac = u.screen_fraction(self.position)
        player.play_stereo("explosion", frac)
        fleets.append(PlayerExplosion(self.position))
        fleets.remove(self)

class PlayerShot(Spritely, InvadersFlyer):
    def explode(self, fleets):
        fleets.append(InvadersExplosion.shot_explosion(self.position, 0.125))
        fleets.remove(self)

That last one’s a bit different, and the duplication in explode_scream_and_die is kind of hidden. I suspect that there is another near-duplicate in here somewhere: I have the vague recollection that there were four.

My plan is to implement an ExplodeMixin that objects can inherit in order to support that capability rather than by duplicating the code.

However: As you’ll see below, what I do is nothing like that plan. A shiny squirrel appears, and I chase it.

chrome squirrel

Above, you see the most distracting object in human history, the Shiny Chrome Squirrel of Unending Distraction. There may be many like it, but this one is mine. Look at it. Try to forget it. You’re still thinking about it, aren’t you?

Let’s also review InvaderExplosion, which seems to deal with some explosions but not all of them.

The first thing I notice, to my dismay, is that I have a class named InvaderExplosion and a class named InvadersExplosion. Nasty. I guess I’m lucky there isn’t one named InvaderExp1osian as well. Lucky? Who do I think wrote this stuff? The one I want is InvadersExplosion:

class InvadersExplosion(InvadersFlyer):

    @classmethod
    def saucer_explosion(cls, position, time):
        maker = BitmapMaker()
        image = maker.saucer_explosion
        return cls(image, position, time)

    @classmethod
    def shot_explosion(cls, position, time):
        maker = BitmapMaker()
        image = maker.player_shot_explosion
        return cls(image, position, time)

    def __init__(self, image, position, time):
        self.image = image
        self.position = position
        self._mask = pygame.mask.from_surface(image)
        self._rect = image.get_rect()
        self._time = time

    def tick(self, delta_time, fleets):
        self._time -= delta_time
        if self._time < 0:
            fleets.remove(self)

    def draw(self, screen):
        self.rect.center = self.position
        screen.blit(self.image, self.rect)

OK, this is basically just draw some bitmap for some time. Wait, what was InvaderExplosion with no “s”?

class InvaderExplosion(InvadersFlyer):
    def __init__(self, position):
        self.position = position
        maker = BitmapMaker()
        self.image = maker.invader_explosion
        self._mask = pygame.mask.from_surface(self.image)
        self._rect = self.image.get_rect()
        # self.image.fill("red",self.rect, special_flags=pygame.BLEND_MULT)
        self.time = 0.125

    def tick(self, delta_time, fleets):
        self.time -= delta_time
        if self.time < 0:
            fleets.remove(self)

    def draw(self, screen):
        self.rect.center = self.position
        screen.blit(self.image, self.rect)

It seems to me that we could do this with the “s” version instead.

Shall we divert to do it? If we do, it will allow us to remove this entire class. Let’s try it.

Let’s rename InvadersExplosion to GenericExplosion. A test fails. Oh it’s our test for implementing the right interact methods.

Those methods in place, we are green. Rename the file and commit: rename InvadersExplosion to GenericExplosion.

Now let’s do a class method for InvaderExplosion.

class GenericExplosion(InvadersFlyer):

    @classmethod
    def invader_explosion(cls, position, time):
        maker = BitmapMaker()
        image = maker.invader_explosion
        return cls(image, position, time)

Find references to InvaderExplosion and change them. Here’s the current version:

class Invader ...
    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))
            fleets.append(InvaderExplosion(self.position))

Extract variable for convenience in editing:

    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 = InvaderExplosion(self.position)
            fleets.append(explosion)

Use the new explosion:

    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)

Test in game. Works just fine. Remove InvaderExplosion class. Test. Commit: remove InvaderExplosion class, using GenericExplosion.

Reflection

So that was interesting. We just replaced a multi-line class with a nice three-line method. There was duplicated code in the class we removed, but perhaps more importantly there was a duplicated concept that could be handled in one spot.

I wonder now about other explosion classes, if there are any. A quick check tells me that the only other one seems to be PlayerExplosion. That one is special, because it is animated. Let’s have a quick look.

class PlayerExplosion(InvadersFlyer):
    def __init__(self, position):
        maker = BitmapMaker.instance()
        self.players = maker.players  # one turret, two explosions
        self.player = self.players[1]
        self._mask = pygame.mask.from_surface(self.player)
        self._rect = self.player.get_rect()
        self._rect.center = position
        self._explode_time = 1

    ...

    def draw(self, screen):
        player = self.players[random.randint(1,2)]
        screen.blit(player, self.rect)

    def tick(self, delta_time, fleets):
        self._explode_time -= delta_time
        if self._explode_time <= 0:
            fleets.remove(self)

The “big difference” here is that when we draw, we randomly select from two bitmaps, the two images of the player exploding.

The entire file is 44 lines counting imports and boilerplate methods not shown. We could readily extend our GenericExplosion to accept two images and randomly display one of them … and of course most users would just provide one image. We’d either duplicate it or do randint(1,len(images)) to randomly select the only one we had.

GenericExplosion is a display-only class, so there are no tests. Should we TDD this? Maybe we should. We aren’t going to. Let’s refactor GenericExplosion:

class GenericExplosion(InvadersFlyer):

    @classmethod
    def invader_explosion(cls, position, time):
        maker = BitmapMaker()
        image = maker.invader_explosion
        return cls(image, position, time)

    ...

    def __init__(self, image, position, time):
        self.image = image
        self.position = position
        self._mask = pygame.mask.from_surface(image)
        self._rect = image.get_rect()
        self._time = time

    def tick(self, delta_time, fleets):
        self._time -= delta_time
        if self._time < 0:
            fleets.remove(self)

    def draw(self, screen):
        self.rect.center = self.position
        screen.blit(self.image, self.rect)

We are going to change the class to expect a collection of images and to randomly select one. Does Python have a random selection function? It probably has … random.choice. Nice.

I’ll do roughly this:

Change the class methods to enclose their image argument in a list;
Change the init to expect a list;
Change the member name to images.
Change the draw to use random.choice

The result:

class GenericExplosion(InvadersFlyer):

    @classmethod
    def invader_explosion(cls, position, time):
        maker = BitmapMaker()
        image = maker.invader_explosion
        return cls((image,), position, time)

    ...

    def __init__(self, images, position, time):
        self.images = images
        self.position = position
        self._mask = pygame.mask.from_surface(images[0])
        self._rect = images[0].get_rect()
        self._time = time

    def tick(self, delta_time, fleets):
        self._time -= delta_time
        if self._time < 0:
            fleets.remove(self)

    def draw(self, screen):
        self.rect.center = self.position
        screen.blit(random.choice(self.images), self.rect)

Works. Commit: GenericExplosion can support a list of images displayed randomly. Not yet used.

But now we can implement the player explosion in GenericExplosion class:

class InvaderPlayer(Spritely, InvadersFlyer):
    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)

A test fails, looking for invaderexplosion:

    def test_player_explodes(self):
        fleets = Fleets()
        fi = FI(fleets)
        fleets.append(InvaderPlayer())
        fleets.append(destructor := Destructor())
        fleets.perform_interactions()
        assert not fi.invader_players
        assert fi.invader_explosions

The test method in FI should just check for GenericExplosion. We could beef it up if need be but I don’t see the need.

class FleetsInspector:
    @property
    def invader_explosions(self):
        return self.select_class(GenericExplosion)

Green. Commit: InvaderPlayer uses GenericExplosion.

Who else uses PlayerExplosion? I suspect that RobotPlayer and ReservePlayer will be the culprits. Yes and there was a trivial test for the class. Remove those, remove the imports, safe delete the file.

Commit: Remove unused PlayerExplosion class.

Reflection and Summary

Well, that was interesting. I really thought we were going to try a “mixin” for the explode method that appears all around the farm, but instead the code beckoned us to consolidate explosions into a single class, which we renamed to GenericExplosion. The result was that we removed two entire classes, and their containing files, from the repo.

When we do the mixin, and we will, it will “save” about three or four lines of duplication in three or four classes. We have saved around 90 or 100 lines of code with GenericExplosion. Very nice find! It’s amazing what you can find when you keep your eyes open!

Reviewing the code with an eye to trying a mixin led us to a much larger opportunity, ultimately replacing two classes with about six lines of new code and a few very simple changes to the GenericExplosion class.

Since the mixin will save us perhaps a dozen or twenty lines of code, and since there are reasons not to like mixins at all, these changes were much more valuable.

Of course, if we had been implementing some new story, we would probably not have even discovered this opportunity. But suppose that we had been passing through some class, noticed the use of a specific explosion, and observed that we could probably use the generic one instead. Would we, should we, make the changes we made today?

By my lights, we certainly should do the simple one where all we had to do was create a new class method. Extending GenericExplosion to do animation was a bit more iffy, but since it is such a small object, the changes were quite trivial. I think I would have done it, and if you would have decided not to do it, I would understand that.

The benefits of changes like these are pretty intangible. The code is simpler, similar things are done in similar ways, necessary changes are centralized … all pretty hard to measure.

I believe, and I know some other guy who believes that making these improvements pays off, and that we do better to make improvements than to let the code slowly become less and less clear and cohesive. YMMV, but that other guy, and a woman I know, and maybe some other folx, do believe in continuous code improvement. It might be worth a look.

And I even learned a little thing: random.choice. I wondered whether Python had something like that and a quick search of the entire internet found it. Much better than the randint thing.

Next time, maybe a mixin. See you then!