Python Asteroids+Invaders on GitHub

Working on sound, we encounter an interesting refactoring, done in a very zen-like fashion. Small steps, add duplication until it resolves. This was really quite nice.

Note
If, despite my assumption that no one is reading this stuff, you happen to be reading this stuff, this article includes a very smooth and I think interesting refactoring. So maybe cruise on down.

I’m pretty sure no one reads these things, so I guess I’m just writing them to while away the morning. I do get a little more feedback when I write about social matters or other more general topics. Fact is, since Covid and my general retirement, my social life pretty much comes down to email and a weekly Zoom ensemble.

Still, if you have questions, ask me anything. I’ll respond directly, and/or in an article, depending on the question.

I’m not sure what came over me yesterday afternoon, but I decided to take a look at putting sound into Invaders. It turns out that I had already downloaded the .wav files, so all I had to do was move them into the project and define the new sounds in my Sounds class, with nine new lines similar to this one:

        self.add_sound("explosion", "sounds/explosion.wav", 0.1)

The first three, firing a shot, invader exploding, and player exploding, were easy. You just find the place where the event happens and add a call to play the sound:

class InvaderPlayer(InvadersFlyer):
    def hit_by_shot(self, fleets):
        player.play("explosion")
        fleets.append(PlayerExplosion(self.position))
        fleets.remove(self)

That was quite simple, and once I realized that these sounds were LOUD and needed a volume of 0.1 out of 1.0, the game got more interesting by virtue of the sounds. It does make a difference: it’s far more engaging with sound.

I’m left with a few things to do:

  1. The saucer makes a warbling sound by alternating two sounds rapidly;
  2. The game makes continuous background sound, a series of four tones that incrementally get faster and faster according to some arcane rule;
  3. The sound player supports stereo, so that my player shots, invader explosions, and the saucer’s not yet defined sounds should use that feature.

Let’s start with stereo. I think we have a decision to make about how to do it. Here’s the play method:

    def play(self, name, location=None, multi_channel=True):
        if name in self.catalog:
            sound = self.catalog[name]
            count = sound.get_num_channels()
            if multi_channel or count == 0:
                chan = self.catalog[name].play()
                if chan:
                    self.set_volume(chan, location)
                # else:
                    # print("channel came back None")
        else:
            print("missing sound", name)

    @staticmethod
    def set_volume(chan, location: MovableLocation):
        if location is None:
            return
        frac_right = location.stereo_right() if location else 0.5
        chan.set_volume(1-frac_right, frac_right)

It turns out that location is supposed to be an instance of MovableLocation, as we see there in set_volume, and we use it to set … wait!

What’s with the if in the setting of frac_right? I don’t think that can ever happen. Furthermore, shouldn’t the early exit be setting the channel to center also? Unless they default that way?

Don’t you just hate it when something like this happens? And I just asked the original programmer (me), and he doesn’t remember anything about this. Well, as usual, we will back away slowly, leaving things as they are. Except that we need to set stereo values for our Invaders objects, and they don’t use MovableLocation.

I see two ways to go.

  1. Create a MovableLocation inside each Invaders sound call, setting it up so that it will do the right thing;
  2. Refactor the play function to allow us to call it with some other stereo control value;
  3. Provide a back door way of setting the stereo value directly.

(As so often happens, I was mistaken about the “two”.)

I just read the play method more carefully. I think it could use a little improvement, so let’s see if a simple bit of safe refactoring can give us a new method. I have in mind a method that takes a fraction from 0 to 1 for the stereo value. Let’s see how MovableLocation does it before we go further.

class MovableLocation:
    def stereo_right(self):
        return self.position.x/u.SCREEN_SIZE

OK, that will return zero when the position is far left, and one when it is far right. Makes sense.

Look at the play logic again and plan a bit:

    def play(self, name, location=None, multi_channel=True):
        if name in self.catalog:
            sound = self.catalog[name]
            count = sound.get_num_channels()
            if multi_channel or count == 0:
                chan = self.catalog[name].play()
                if chan:
                    self.set_volume(chan, location)
                # else:
                    # print("channel came back None")
        else:
            print("missing sound", name)

    @staticmethod
    def set_volume(chan, location: MovableLocation):
        if location is None:
            return
        frac_right = location.stereo_right() if location else 0.5
        chan.set_volume(1-frac_right, frac_right)
Note
Here we begin a series of small changes. At the end, we’re going to have a very nice solution to our need for two ways of requesting stereo sounds. Your intrepid author doesn’t so much figure out how to do that, as to move the code around, including actually increasing duplication a few times, until, without warning or deep thinking, things just fall into place.

I think that’s important and almost profound.

Let’s move the channel setting inside play, instead returning the frac_right from set_volume. We’ll rename it get_volume to reflect its new role.

    def play(self, name, location=None, multi_channel=True):
        if name in self.catalog:
            sound = self.catalog[name]
            count = sound.get_num_channels()
            if multi_channel or count == 0:
                chan = self.catalog[name].play()
                if chan:
                    frac_right = self.get_volume(chan, location)  # <===
                    chan.set_volume(1-frac_right, frac_right)     # <===
        else:
            print("missing sound", name)

    @staticmethod
    def get_volume(chan, location: MovableLocation):
        return location.stereo_right() if location else 0.5

That was definitely not a machine refactoring, so I need to test in the game. Slightly irritating, but easily done, we’re good. In fact, commit: refactoring Sound toward providing stereo for Invaders.

Now, let’s work toward a low-level play function that accepts the sound and the stereo fraction, so that we can use it in Invaders. For best results, let’s think what Invaders wants. What is our “intention” here? In particular, what is the multi_channel bit about?

Note
Here, I accumulate a bit of understanding, and mostly then ignore it. I wonder what that code is doing, figure out that it’s deciding whether we can play or not, and then forget it. Later, we encapsulate that thought a bit better, and later still, we move the “can play” concept to a new place, to our great advantage.

None of this is planned. It’s just discover, try, discover.

It turns out that if we set that value to False, then when we go to play that sound, it will not play if it is already playing. If it’s set to true and the sound is playing, pygame will layer in another copy of the same sound. So, with multi-channel set to True, if one asteroid is exploding and another one explodes, you get a new channel, with the two sounds overlapping, which is what you want. But when the ship is accelerating, we only use one channel, so that you don’t get more and more acceleration sounds layering. Instead, the sound plays all the time that you have the button down, but only one layer at a time.

I think that in Invaders, we will be happy to have multi channel all the time. So we want something like this in Invaders:

    def hit_by_shot(self, fleets):
        frac = self.x_fraction()
        player.play_stereo("explosion", frac)
        fleets.append(PlayerExplosion(self.position))
        fleets.remove(self)
Note
This is perhaps the smartest move in this whole series, and you can’t exactly call it brilliant. “How about we see what we need?” Up until now I was thinking of refactoring the play logic, hoping to fit it to a need that I hadn’t fully solidified. This step makes what we need far more clear.

You know what we should do? We should just implement that function on Sounds and then improve the code. That makes more sense to me than trying to guess what the code should be. Here we go.

    def hit_by_shot(self, fleets):
        frac = self.x_fraction()
        player.play_stereo("explosion", frac)
        fleets.append(PlayerExplosion(self.position))
        fleets.remove(self)

    def x_fraction(self):
        x = self.rect.centerx - self.left
        denom = self.right - self.left 
        return x / denom

I think that’s about right. And now for play_stereo:

    def play_stereo(self, name, frac):
        try:
            sound = self.catalog[name]
        except KeyError:
            return
        chan = sound.play()
        if chan:
            chan.set_volume(1-frac, frac)

I’ve been told that try/except is more pythonic than checking for in. The code above works just fine, with the explosion coming out to the left or right as I move the player. Let’s do the other cases, the player firing and an invader exploding.

Note
Our larger story is how we refactor the play functions, but here I defer that refactoring to put my sounds into actual play. Made sense to me, digression from the refactoring story that I’d like us to focus on here. You can skip to the next Note if you wish.
    def fire(self, fleets):
        frac = self.x_fraction()
        player.play_stereo("shoot", frac)
        self.shot_count += 1
        fleets.append(PlayerShot(self.rect.center))

That one was easy. The Invader is about the same, with this version of x_fraction:

    def interact_with_group_and_playershot(self, shot, group, fleets):
        if self.colliding(shot):
            player.play_stereo("invaderkilled", self.x_fraction())
            shot.hit_invader(fleets)
            group.kill(self)
            fleets.append(InvaderScore(self._score))
            fleets.append(InvaderExplosion(self.position))

    def x_fraction(self):
        x = self.rect.centerx - u.BUMPER_LEFT
        denom = u.BUMPER_RIGHT - u.BUMPER_LEFT
        return x / denom

I should be committing all this good stuff. Commit: all current sounds are stereo. Sounds contains some duplication.

Note
Now we get back to refactoring.

Now let’s see about improving the sounds code:

    def play(self, name, location=None, multi_channel=True):
        if name in self.catalog:
            sound = self.catalog[name]
            count = sound.get_num_channels()
            if multi_channel or count == 0:
                chan = self.catalog[name].play()
                if chan:
                    frac_right = self.get_volume(chan, location)
                    chan.set_volume(1-frac_right, frac_right)
                # else:
                    # print("channel came back None")
        else:
            print("missing sound", name)

    def play_stereo(self, name, frac):
        try:
            sound = self.catalog[name]
        except KeyError:
            return
        chan = sound.play()
        if chan:
            chan.set_volume(1-frac, frac)

    @staticmethod
    def get_volume(chan, location: MovableLocation):
        return location.stereo_right() if location else 0.5
Note
Here I form a vague notion that the code will be better if it bottoms out in play_stereo. That turns out to be correct, although I certainly do not see at this point how it’s going to turn out. I’m just moving toward “better”, not toward any specific answer.

We’ll want to come down to calling play_stereo, I think. I’m trying to remain open to other possibilities. Let’s make the play look more like play_stereo by using the try/except.

    def play(self, name, location=None, multi_channel=True):
        try:
            sound = self.catalog[name]
        except KeyError:
            print("missing sound", name)
            return
        count = sound.get_num_channels()
        if multi_channel or count == 0:
            chan = self.catalog[name].play()
            if chan:
                frac_right = self.get_volume(chan, location)
                chan.set_volume(1-frac_right, frac_right)

    def play_stereo(self, name, frac):
        try:
            sound = self.catalog[name]
        except KeyError:
            print("missing sound", name)
            return
        chan = sound.play()
        if chan:
            chan.set_volume(1-frac, frac)
Note
I don’t have a very good reason for this next step. It just seems to me that getting the fraction sooner will be of value. Again, just moving the code toward “better” with no grand plan.

I think we’ll always want the stereo value, so let’s move it up:

    def play(self, name, location=None, multi_channel=True):
        try:
            sound = self.catalog[name]
        except KeyError:
            print("missing sound", name)
            return
        frac_right = self.get_volume(location)
        count = sound.get_num_channels()
        if multi_channel or count == 0:
            chan = self.catalog[name].play()
            if chan:
                chan.set_volume(1-frac_right, frac_right)

    def play_stereo(self, name, frac):
        try:
            sound = self.catalog[name]
        except KeyError:
            print("missing sound", name)
            return
        chan = sound.play()
        if chan:
            chan.set_volume(1-frac, frac)

    @staticmethod
    def get_volume(location: MovableLocation):
        return location.stereo_right() if location else 0.5
Note
A casual but valuable move here. With “chan” in the calling sequence, though it was no longer needed, possible moves were not as clear as they are without that unneeded parameter. Cleaning this up makes it easier to see what I can and should do. Again a simple move toward “better” pays off.

I also removed the unused chan parameter in get_volume. Let’s rename that to something about stereo and fraction.

    def play(self, name, location=None, multi_channel=True):
        try:
            sound = self.catalog[name]
        except KeyError:
            print("missing sound", name)
            return
        frac_right = self.get_stereo_fraction(location)
        count = sound.get_num_channels()
        if multi_channel or count == 0:
            chan = self.catalog[name].play()
            if chan:
                chan.set_volume(1-frac_right, frac_right)
Note
Another move toward “better” with no deep purpose:

That second references to catalog[name] can refer to sound:

    def play(self, name, location=None, multi_channel=True):
        try:
            sound = self.catalog[name]
        except KeyError:
            print("missing sound", name)
            return
        frac_right = self.get_stereo_fraction(location)
        count = sound.get_num_channels()
        if multi_channel or count == 0:
            chan = sound.play()
            if chan:
                chan.set_volume(1-frac_right, frac_right)
Note
Probably what caught my eye here was just that the count stuff looked busy. So I try to make it better.

Now what’s going on there with count is that we’re deciding whether or not to play. Let’s extract:

First extract variable, because PyCharm can’t refactor part of an if:

    def play(self, name, location=None, multi_channel=True):
        try:
            sound = self.catalog[name]
        except KeyError:
            print("missing sound", name)
            return
        frac_right = self.get_stereo_fraction(location)
        count = sound.get_num_channels()
        can_play = multi_channel or count == 0
        if can_play:
            chan = sound.play()
            if chan:
                chan.set_volume(1-frac_right, frac_right)
Note
The following move seemed better but once I got there, I didn’t like it. We’re going to back up a bit soon.

Ah. We have the sound already, not the name, and we needed the sound to get the channel info. Therefore, let’s extract a method play_sound_in_stereo from play_stereo:

    def play_stereo(self, name, frac):
        try:
            sound = self.catalog[name]
        except KeyError:
            print("missing sound", name)
            return
        chan = sound.play()
        if chan:
            chan.set_volume(1-frac, frac)

PyCharm detects both opportunities, so now we have:

    def play(self, name, location=None, multi_channel=True):
        try:
            sound = self.catalog[name]
        except KeyError:
            print("missing sound", name)
            return
        frac_right = self.get_stereo_fraction(location)
        count = sound.get_num_channels()
        can_play = multi_channel or count == 0
        if can_play:
            self.play_sound_in_stereo(sound, frac_right)

    def play_stereo(self, name, frac):
        try:
            sound = self.catalog[name]
        except KeyError:
            print("missing sound", name)
            return
        self.play_sound_in_stereo(sound, frac)

    def play_sound_in_stereo(self, sound, frac):
        chan = sound.play()
        if chan:
            chan.set_volume(1 - frac, frac)

Huh

So this is kind of goofy. I’d like now to get rid of the duplication of the try / except, but to do so is complicated by the fact that I have an early return. I can move everything below the return up into the try. I don’t think that helps.

Note
Here is the key idea being born. We do want to push the sound name down into the stereo code, but not with things as we stand. I decide to back up to a previous step and try a different direction.

Let’s push the name down all the way to play_sound_in_stereo. No. Belay that. Undo the previous refactoring. Back to this:

    def play(self, name, location=None, multi_channel=True):
        try:
            sound = self.catalog[name]
        except KeyError:
            print("missing sound", name)
            return
        frac_right = self.get_stereo_fraction(location)
        count = sound.get_num_channels()
        can_play = multi_channel or count == 0
        if can_play:
            chan = sound.play()
            if chan:
                chan.set_volume(1-frac_right, frac_right)

    def play_stereo(self, name, frac):
        try:
            sound = self.catalog[name]
        except KeyError:
            print("missing sound", name)
            return
        chan = sound.play()
        if chan:
            chan.set_volume(1-frac, frac)
Note
This is “make more duplication”. I’m making the two functions look more alike. I don’t actually think of it in those terms in what follows: it just seemed good to have the multi-channel pushed down. I think perhaps I foresaw the next step and didn’t mention it.

Ah. Let’s extend play_stereo with a defaulted multi_channel like play:

    def play_stereo(self, name, frac, multi_channel=True):
        try:
            sound = self.catalog[name]
        except KeyError:
            print("missing sound", name)
            return
        chan = sound.play()
        if chan:
            chan.set_volume(1-frac, frac)
Note
This is definitely “make more duplication”. I think I was sure that this was going to work out at this point. But the move is just “make more duplication”.

Now put the channel-checking code in there, to make the two functions look more alike:

    def play(self, name, location=None, multi_channel=True):
        try:
            sound = self.catalog[name]
        except KeyError:
            print("missing sound", name)
            return
        frac_right = self.get_stereo_fraction(location)
        count = sound.get_num_channels()
        can_play = multi_channel or count == 0
        if can_play:
            chan = sound.play()
            if chan:
                chan.set_volume(1-frac_right, frac_right)

    def play_stereo(self, name, frac, multi_channel=True):
        try:
            sound = self.catalog[name]
        except KeyError:
            print("missing sound", name)
            return
        count = sound.get_num_channels()
        can_play = multi_channel or count == 0
        if can_play:
            chan = sound.play()
            if chan:
                chan.set_volume(1-frac, frac)

Now it seems clear. We can just get the fraction up in the first method and call play_stereo.

Note
By Jove, he’s got it. Did he know before that move that it would turn out this nice? I don’t think so, but I think he just felt it would be better. It turns out to be so much better that we can stop.
    def play(self, name, location=None, multi_channel=True):
        frac_right = self.get_stereo_fraction(location)
        self.play_stereo(name, frac_right, multi_channel)

    def play_stereo(self, name, frac, multi_channel=True):
        try:
            sound = self.catalog[name]
        except KeyError:
            print("missing sound", name)
            return
        count = sound.get_num_channels()
        can_play = multi_channel or count == 0
        if can_play:
            chan = sound.play()
            if chan:
                chan.set_volume(1-frac, frac)

Now that’s what I call neat. The steps, not so neat. Let’s reflect.

Reflection

I think what happened here was more important and more valuable and less weird and mistaken than one might think.

It may seem to you that I just kind of mushed the code around, without really thinking, until I finally saw how to collapse it nicely. And that is exactly what really happened. And that, I claim, is a good thing.

Let’s look back at how things were right after I put in play_stereo:

    def play(self, name, location=None, multi_channel=True):
        if name in self.catalog:
            sound = self.catalog[name]
            count = sound.get_num_channels()
            if multi_channel or count == 0:
                chan = self.catalog[name].play()
                if chan:
                    frac_right = self.get_volume(chan, location)
                    chan.set_volume(1-frac_right, frac_right)
                # else:
                    # print("channel came back None")
        else:
            print("missing sound", name)

    def play_stereo(self, name, frac):
        try:
            sound = self.catalog[name]
        except KeyError:
            return
        chan = sound.play()
        if chan:
            chan.set_volume(1-frac, frac)

Now here’s our much nicer final result:

    def play(self, name, location=None, multi_channel=True):
        frac_right = self.get_stereo_fraction(location)
        self.play_stereo(name, frac_right, multi_channel)

    def play_stereo(self, name, frac, multi_channel=True):
        try:
            sound = self.catalog[name]
        except KeyError:
            print("missing sound", name)
            return
        count = sound.get_num_channels()
        can_play = multi_channel or count == 0
        if can_play:
            chan = sound.play()
            if chan:
                chan.set_volume(1-frac, frac)

Is there some brilliant thought sequence that would have let me just go ahead and implement that?

I can almost see it, now. Something like, “well, if play_stereo accepted multi-channel, then we could just get the fraction and call it”. But no. Seeing that almost requires us to have the second solution in front of us. I’m sure that it requires me to know the answer before I can see a direct path, if there even is one.

We could have gone back to the drawing board, perhaps literally, or done some cards with “look up name”, “compute fraction”, “what’s going on here with get channel”, and such, and then arranged them in various ways, drawn circles and arrows and paragraphs on the front of each one, and we might have come up with this sequence.

Instead of a speculative drawing or card game, we moved the actual code around , making it look more and more alike, pushing it into various arrangements until suddenly it looked so much alike that it was clear (to me) what needed to be done.

Summary: Deeper Reflection

This very nice result came out of nothing more than building some duplicated code, moving it around to make the bits more and more similar, until suddenly, it was possible to collapse one method into little more than a call to the other.

There was a time in my life when I would have treasured being so brilliant that I just saw immediately how to do this in one step from where it started to where it has ended. What I treasure now is finding steps so small that they seem almost not to require thinking, but instead just move the code in a direction that I think is probably better.

Sometimes it isn’t better. I get there and see that, and I undo or revert or put it back where it was or move it from there toward what I now see as probably better.

And after a while, there is it in front of me, a very nice solution resulting in better code that meets my needs.

I treasure not needing the brilliant insight, fueled by a half dozen Diet Cokes per day. I treasure just wandering peacefully through the code garden, pruning here, moving this plant over here a bit, grafting this to that, making tiny easy changes that make the garden a more and more pleasant place to be.

It looks chaotic. It looks unplanned. It is unplanned, but it is not chaotic. Things move toward “better”, and my notion of “better” is actually pretty well honed. I don’t always know how to get there, and once in a while I don’t see much difference between two arrangements, but by and large, my sense of “better” is consistent and leads to code that I can change more flexibly.

And I can do it without burning my mental candle at both ends. Which is good, because my candle is getting pretty stubby.

Would this work for you? Do you find it interesting? Or disgusting? Or frightening? Or tantalizing? Boring? Fascinating?

I find it fascinating. I just shape the code with my hands until I like the shape. It’s not so much thinking as it is sensing.

I think there’s value here. What do you think? Let me know.