Python Asteroids+Invaders on GitHub

I’ve run into a small issue with the Sounds object. I think I have a solid idea for it.

You may recall that the sound-playing functions allow a parameter multi-channel, defaulting to True. You may even vaguely recall that the meaning of this parameter is that, if multi_channel is True, and the sound in question is already playing, it sill be played again on another channel, and if is it False, the sound will not be layered in again.

This has two very nice properties:

  1. If we set explosions to have multi_channel True, there can be several overlapping explosion sounds running at ones. This is quite nice, especially in Asteroids, where lots of things can be blowing up at the same time. But

  2. If we set multi_channel False, a sound like the eerie whistling of the saucer only plays once, but if the saucer keeps trying to play it every time around, as soon as the sound stops, it starts again, making for continuous sound without the need for accurate timing.

But there is a problem with the InvadersSaucer: its sound is quite long in duration, to the point where it is only started once for the saucer’s entire flight. That means that the stereo effect doesn’t work: if it starts on the right, the sound comes from the right even as it flies off to the left.

We could “fix” that by setting the sound just to play from center. But I want to try to do better. This may not work, but I want to try setting the stereo position every time through, even though we do not restart the sound.

Here’s the current method that we call:

class Sounds:
    def play_stereo(self, name, stereo_fraction_right, multi_channel=True):
        try:
            sound = self.catalog[name]
        except KeyError:
            return
        if multi_channel or sound.get_num_channels() == 0:
            chan = sound.play()
            if chan:
                chan.set_volume(1 - stereo_fraction_right, stereo_fraction_right)

Since multi-channel is false, and the sound is playing, the big if will be false and we will not play the sound, not get the channel, and not set stereo, except for the first time through.

I think we need to save the most recent channel used for each sound, and use the sound name to set the stereo. Like this:

class Sounds:
    def __init__(self):
        self.catalog = {}
        self.channels = {}

    def play_stereo(self, name, stereo_fraction_right, multi_channel=True):
        try:
            sound = self.catalog[name]
        except KeyError:
            return
        if multi_channel or sound.get_num_channels() == 0:
            chan = sound.play()
            self.channels[name] = chan
        chan = self.channels[name]
        if chan:
            chan.set_volume(1 - stereo_fraction_right, stereo_fraction_right)

Yay! The sound now warbles across the screen! Perfect! I have a small concern about this code. It is sequentially coupled, by which I mean, unless we have set self.channels[name] first, later the assignment could throw an exception. I don’t think this can happen in practice, at least not as the code stands now, but it’s a bit scary.

In addition, I don’t like the reuse of the name chan. In fact, now that I think about it, I don’t even like the name. Do you suppose the product got delivered a lot sooner because I saved typing nel a time or two?

Let’s inline one and rename the other:

    def play_stereo(self, name, stereo_fraction_right, multi_channel=True):
        try:
            sound = self.catalog[name]
        except KeyError:
            return
        if multi_channel or sound.get_num_channels() == 0:
            self.channels[name] = sound.play()
        channel = self.channels[name]
        if channel:
            channel.set_volume(1 - stereo_fraction_right, stereo_fraction_right)

And now let’s protect ourselves from the dictionary not having an answer:

    def play_stereo(self, name, stereo_fraction_right, multi_channel=True):
        try:
            sound = self.catalog[name]
        except KeyError:
            return
        if multi_channel or sound.get_num_channels() == 0:
            self.channels[name] = sound.play()
        try:
            channel = self.channels[name]
            if channel:
                channel.set_volume(1 - stereo_fraction_right, stereo_fraction_right)
        except KeyError:
            pass

OK, that’s solid, I think, so let’s commit: Saucer sound now in moving stereo across screen.

But this code does not please me. Since our next section is going to be about reviewing the code, let’s continue right here. I think this code is cluttered. It’s kind of jagged, zig-zagging in and out, making it tricky to follow.

The first try/except is acting as a Guard Clause: we can’t play a sound if we don’t know it. But it has that odd double indent to get to the return. We could, certainly, guard the whole method with a single try/except. Let’s “try” that.

    def play_stereo(self, name, stereo_fraction_right, multi_channel=True):
        try:
            sound = self.catalog[name]
            if multi_channel or sound.get_num_channels() == 0:
                self.channels[name] = sound.play()
            channel = self.channels[name]
            if channel:
                channel.set_volume(1 - stereo_fraction_right, stereo_fraction_right)
        except KeyError:
            pass

Well, maybe that’s a bit less jagged, but now it is more nested. I do not like that the channel = could throw: it’s no longer clear. We could have left the try in there but nested … that would be even worse.

Let’s try some Extract Method operations:

    def play_stereo(self, name, stereo_fraction_right, multi_channel=True):
        try:
            self.play_if_possible(name, multi_channel)
            self.set_stereo(name, stereo_fraction_right)
        except KeyError:
            pass

    def play_if_possible(self, name, multi_channel):
        sound = self.catalog[name]
        if multi_channel or sound.get_num_channels() == 0:
            self.channels[name] = sound.play()

    def set_stereo(self, name, stereo_fraction_right):
        channel = self.channels[name]
        if channel:
            channel.set_volume(1 - stereo_fraction_right, stereo_fraction_right)

Hm, that’s almost good. What if we move the try/excepts back inside now?

    def play_stereo(self, name, stereo_fraction_right, multi_channel=True):
        self.play_if_possible(name, multi_channel)
        self.set_stereo(name, stereo_fraction_right)

    def play_if_possible(self, name, multi_channel):
        try:
            sound = self.catalog[name]
        except KeyError:
            return
        if multi_channel or sound.get_num_channels() == 0:
            self.channels[name] = sound.play()

    def set_stereo(self, name, stereo_fraction_right):
        try:
            channel = self.channels[name]
        except KeyError:
            return
        if channel:
            channel.set_volume(1 - stereo_fraction_right, stereo_fraction_right)

Nearly good. We can commit but let’s note that the if channel is redundant in set_stereo.

    def set_stereo(self, name, stereo_fraction_right):
        try:
            self.channels[name].set_volume(1 - stereo_fraction_right, stereo_fraction_right)
        except KeyError:
            return

Note that I was able to inline the setting of the temporary variable channel and remove the temp.

Would we prefer the play_if_possible this way:

    def play_if_possible(self, name, multi_channel):
        try:
            sound = self.catalog[name]
            if multi_channel or sound.get_num_channels() == 0:
                self.channels[name] = sound.play()
        except KeyError:
            return

I think we would. And why not give that if a name, at least with a couple of Explaining Variable Names?

    def play_if_possible(self, name, multi_channel):
        try:
            sound = self.catalog[name]
            sound_not_playing = sound.get_num_channels() == 0
            channel_available = multi_channel or sound_not_playing
            if channel_available:
                self.channels[name] = sound.play()
        except KeyError:
            return

I think I like that. Here’s the whole megillah:

    def play_stereo(self, name, stereo_fraction_right, multi_channel=True):
        self.play_if_possible(name, multi_channel)
        self.set_stereo(name, stereo_fraction_right)

    def play_if_possible(self, name, multi_channel):
        try:
            sound = self.catalog[name]
            sound_not_playing = sound.get_num_channels() == 0
            channel_available = multi_channel or sound_not_playing
            if channel_available:
                self.channels[name] = sound.play()
        except KeyError:
            return

    def set_stereo(self, name, stereo_fraction_right):
        try:
            self.channels[name].set_volume(1 - stereo_fraction_right, stereo_fraction_right)
        except KeyError:
            return

Compare to:

    def play_stereo(self, name, stereo_fraction_right, multi_channel=True):
        try:
            sound = self.catalog[name]
        except KeyError:
            return
        if multi_channel or sound.get_num_channels() == 0:
            self.channels[name] = sound.play()
        try:
            channel = self.channels[name]
            if channel:
                channel.set_volume(1 - stereo_fraction_right, stereo_fraction_right)
        except KeyError:
            pass

17 lines now, vs 13 before. But now each bit of code has a name and a reason for being. I prefer this, and expect to prefer it if I ever come back to work on Sounds again. YMMV, and if you disagree, I’d be happy to discuss it on Mastodon or the AgileMentoring Slack.

I think we’ve covered enough ground for this morning. Let’s sum up.

Summary

The idea for how to allow stereo setting in the middle of playing a sound worked out well: we just cache the channel that sounds are playing on and fetch it if we want a new stereo setting.

The code didn’t please me. Let me foreshadow the upcoming article by summarizing the things that I noticed.

  1. A variable name was abbreviated to no particular advantage;
  2. The code reused that variable name in a potentially confusing way;
  3. The code was sequentially coupled, such that appeared that the dictionary might not be set properly before use;
  4. The code was cluttered and jagged.

Four different calls for improvement, “code smells” if you will, in a dozen lines of code. I am curious about what trouble indicators I observe in my code, and that’s what we’ll explore in an upcoming article.

See you next time!