Code Signs, Signals, Observations, Portents

Python Asteroids+Invaders on GitHub

I propose to identify and discuss the things I see in code, what they make me think, and perhaps a bit about what I might do about them. This does not go as I expected.

There are probably a number of compendia of code signals¹ out there on the Internet and in books, and for all I know, rock and roll albums and scary movies. I am not daunted by this fact, because what I’m going to do here is try to observe myself, showing what we can find in a program written by a very experienced person who tries hard to keep things in good order.

I think we’ll discover that there is always plenty of room for improvement, at least in my code. The hard part will come in deciding, over and over, which way we prefer, and why.

The examples here will be in Python, but I think the impact of the signs will be clear even if your preferred language is other than Python.

Discovery Process

I’m just going to open files in this application and scan through them for portents. I’m not sure how the article will be arranged.

Note

It turns out to be arranged in chronological order by time discovered. So far, no larger organization has really appeared to me.

Naming

    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

This code was just revised yesterday. Today I’m looking at the two variable names sound_not_playing, and channel_available.

channel_available is really a boolean, saying whether there is a channel available for us to play on. But the name reads a bit as if it is the name of a channel that is available. Similarly sound_not_playing reads as if it is the name of a sound that is not playing. We might rename them:

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

The pattern in use there is “Explaining Temporary Variable”², where we create an unnecessary temp variable just to name the concept. Without those variables, we would have to do more thinking to figure out what was going on:

    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

As always, use of these patterns is about judgment. As my brother GeePaw Hill puts it, “The code works for me, I don’t work for the code”. We might even prefer an intermediate state:

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

Your house, your rules.

Condition Handling

While we’re here, let’s consider the try/except form used in this method. I used it because I read somewhere that try/except is considered to be the more “pythonic” way of doing things, so I have been trying it. However there are other alternatives including this one:

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

I think that I prefer that. We’ll go with it.

Complicated Method

Here’s a method worth looking at:

class InvadersSaucer:
    def _move_or_die(self, fleets):
        x = self.position.x + self._speed
        if x > self._right or x < self._left:
            self._die(fleets)
        else:
            frac = (x - self._left)/(self._right - self._left)
            player.play_stereo("ufo_lowpitch", frac, False)
            self.position = (x, self.position.y)

There are a number of things going on here:

1. Compute a new x position for the saucer;
2. Check to see if it is now past its travel limits;
3. Die if so;
4. Otherwise play a sound, and
5. Move to the new position.

The code does not tell us any of those things. The worst bit is the code under the else, which is doing two things in three lines.

We have that temp, ‘x’, which, it appears, is just there to avoid multiple accesses to self.position.x.

Let’s move and then test.

    def _move_or_die(self, fleets):
        self.position = (self.position.x + self._speed, self.position.y)
        if self.position.x > self._right or self.position.x < self._left:
            self._die(fleets)
        else:
            frac = (self.position.x - self._left)/(self._right - self._left)
            player.play_stereo("ufo_lowpitch", frac, False)

Extract that ifcondition to a method: (what vs how)

    def _move_or_die(self, fleets):
        self.position = (self.position.x + self._speed, self.position.y)
        if self._going_off_screen():
            self._die(fleets)
        else:
            frac = (self.position.x - self._left)/(self._right - self._left)
            player.play_stereo("ufo_lowpitch", frac, False)

    def _going_off_screen(self):
        return self.position.x > self._right or self.position.x < self._left

Extract the else lines to a method: (what vs how)

    def _move_or_die(self, fleets):
        self.position = (self.position.x + self._speed, self.position.y)
        if self.going_off_screen():
            self._die(fleets)
        else:
            self._adjust_stereo_position()

    def _adjust_stereo_position(self):
        frac = (self.position.x - self._left) / (self._right - self._left)
        player.play_stereo("ufo_lowpitch", frac, False)

Now I don’t like the if/else as much as I might, and I see that I reorder things without harm:

    def _move_or_die(self, fleets):
        self.position = (self.position.x + self._speed, self.position.y)
        self._adjust_stereo_position()
        if self._going_off_screen():
            self._die(fleets)

The above is not exactly the same as the preceding version, because now we update the stereo before dying. You won’t hear the difference, because dying kills the sound anyway.

The code above does a calculation, calls a method, executes a conditional, and optionally calls another method. There are really three ideas there and we can express them, using the ComposedMethod pattern.

We extract two new methods (what vs how):

    def _move_or_die(self, fleets):
        self._move_along_x()
        self._adjust_stereo_position()
        self._die_if_done(fleets)

And the whole thing looks like this:

    def _move_or_die(self, fleets):
        self._move_along_x()
        self._adjust_stereo_position()
        self._die_if_done(fleets)

    def _move_along_x(self):
        self.position = (self.position.x + self._speed, self.position.y)

    def _adjust_stereo_position(self):
        frac = (self.position.x - self._left) / (self._right - self._left)
        player.play_stereo("ufo_lowpitch", frac, False)

    def _die_if_done(self, fleets):
        if self._going_off_screen():
            self._die(fleets)

    def _going_off_screen(self):
        return self.position.x > self._right or self.position.x < self._left

One more thing, which I just learned:

    def _going_off_screen(self):
        return not self._left <= self.position.x <= self._right

And yes, that not does exactly what we would like. I have a test for it.

Note that this change adds a bit more “what” understanding to “how” we determine whether we’re off screen. Python gives us some nice notation here.

Reflection

There are at least two ways to think of this. One is that we have moved from these eight lines:

    def _move_or_die(self, fleets):
        x = self.position.x + self._speed
        if x > self._right or x < self._left:
            self._die(fleets)
        else:
            frac = (x - self._left)/(self._right - self._left)
            player.play_stereo("ufo_lowpitch", frac, False)
            self.position = (x, self.position.y)

To just these four:

    def _move_or_die(self, fleets):
        self._move_along_x()
        self._adjust_stereo_position()
        self._die_if_done(fleets)

Another way to think of it, of course, is that we have moved from eight lines to 14 non-blank lines:

    def _move_or_die(self, fleets):
        self._move_along_x()
        self._adjust_stereo_position()
        self._die_if_done(fleets)

    def _move_along_x(self):
        self.position = (self.position.x + self._speed, self.position.y)

    def _adjust_stereo_position(self):
        frac = (self.position.x - self._left) / (self._right - self._left)
        player.play_stereo("ufo_lowpitch", frac, False)

    def _die_if_done(self, fleets):
        if self._going_off_screen():
            self._die(fleets)

    def _going_off_screen(self):
        return not self._left <= self.position.x <= self._right

Eight to four is clearly an improvement. One could argue about eight moving to fourteen, but if you are an aficionado of the small methods style, this is exactly what you look for.

Summary

This did not go as I expected. What I thought I would do was to observe a wide range of issues in the code, identifying some dimensions of code along which we might choose to change things, and to arrange those in some sensible order, discussing each one, and drawing general conclusions.

Instead, I spotted things that could arguably be improved and changed them in directions that might be better. No real general rules so much as just a few simple change types. Let’s see:

1. We renamed things to make it easier to see what we meant.

2. We replaced a try/except with an if, resulting in somewhat simpler code. This suggests that the supposedly “pythonic” preference for try/except might not always lead to code that we’ll prefer.

3. We then turned out attention to a different, slightly complicated method. We replaced computing a temp and reusing it with computing the value multiple times. This seems even now like a very odd thing to do, but in the end we wound up with code that I prefer. Was replacing that temp necessary to that discovery? It may have been.

4. We reordered the method a bit, to execute the move immediately and then continue with the rest. It is possible that this change slightly changed visible behavior, but not in any easily noticeable fashion.

5. We replaced a moderately complex if statement with a call to a method that did the complex action and returned the result. The calling code then expressed what it wanted and the called code, how it was done.

6. We subsequently replaced that how code with a simpler version that made it more clear as well.

7. We extracted two lines to a method saying what (adjust stereo), calling a method that says how, computing a fraction between zero and one and calling some internal sound method.

8. We reordered the lines so that the stereo change was made before checking for the object dying, which eliminated an else clause.

9. We did two more Extract Method refactorings to give the method its final form, saying what is done, with all details left in the methods called:

    def _move_or_die(self, fleets):
        self._move_along_x()
        self._adjust_stereo_position()
        self._die_if_done(fleets)

I had honestly expected to draw some general observations about what I look for, and how I respond based upon what I see, but in fact that just didn’t happen. I do think that I see some general notions, vaguely:

General Notions (Vaguely)

Name What is Done

Whether extracting temporary values with useful names, or extracting methods with useful names, use names that describe what is happening in terms of the bigger picture, not how it is done in detail. Leave the details to the implementation of the thing named.

Eliminate Alternatives

Prefer an if to an if/else or try/except. Tend to prefer if/else to try/except, but keep trying both and build up a better feeling for when to use either.

Composed Method

Rather than have a method that implicitly does multiple things one after another, each with one or more statements, explicitly state the things that it does and implement the details in separate called methods. Prefer methods that only call other methods over methods that call other methods and also make decisions or perform loops. Put each conditional or looping idea in its own method where possible.³

Summary Summary

I am often surprised by what I do, but it’s usually at the level of the code. Today I am surprised at a higher level, thinking that I was going to be working on big picture items and instead making things better by small steps without much concern for the big picture.

Fascinating.

See you next time!