Python Asteroids+Invaders on GitHub

Time to look at the Saucer and see if it needs some improvement. I feel sure that it does. A very nice refactoring sequence appears.

Let’s check Jira (ptui!):

Current

  • display mystery score
  • Flies at constant height and speed TBD
  • Direction: Comes from right if player shot count is even
  • kill the shot if it hits saucer
  • test missing the saucer
  • make an explosion
  • change to snatch shot_count, not player (too hard to understand)
  • Scores: 10 05 05 10 15 10 10 05 30 10 10 10 05 15 10 05
  • Score loops at 15, not 16 (replicate this bug?)

Displaying the score will be only semi-interesting. I can think of two ways to do it. We could just put some conditional logic in the saucer code to display the score for a while, or we could create a new little score-displaying object and toss it into the mix. Since the saucer already tosses its explosion into the mix, the second way is more appealing. In addition, it fits more with our design style of lots of independent cooperating objects making up the game. So we’ll probably choose the latter. There will be almost nothing to learn from doing it, although we might find some way of using or subclassing the explosion objects, which already display something for a while and then pass on.

Before we do that, however, we were looking at the existing code with an eye to simplifying it. Let’s look at it here and talk about things we might not like.

class InvadersSaucer(InvadersFlyer):
    def __init__(self, direction=1):
        self.direction = direction
        maker = BitmapMaker.instance()
        self._map = maker.saucer
        self._mask = pygame.mask.from_surface(self._map)
        self._rect = self._map.get_rect()
        half_width = self._rect.width // 2
        self._left = u.BUMPER_LEFT + half_width
        self._right = u.BUMPER_RIGHT - half_width
        self._speed = 0
        self._player = None
        self._score_list = [100, 50, 50, 100, 150, 100, 100, 50, 300, 100, 100, 100, 50, 150, 100]
        self.initialized = False

To begin with, this is a lot of initialization and we can see several topics in there, including:

  • setting up the graphic map, mask, and rectangle;
  • defining the left and right start/end points;
  • initializing speed to zero, suggesting that it gets its real init elsewhere;
  • setting player to None, also suggesting a later init;
  • defining a score list, which is the same for all instances;
  • setting the initialized flag, a clear indication that there’s more to happen later.

The whole class is just shy of 100 lines, which isn’t the worst thing ever, but makes it seem very likely to me that there’s more than one thing going on in here. And even without looking at the code, we know of things:

  • managing starting, movement, and stopping;
  • drawing;
  • interacting with player shots;
  • whatever is implied by the initialized flag;
  • scoring.

Now, frequent readers and I probably remember some of what goes on under those headings but the code should tell us if we aren’t sure. I skip over some boiler-plate and look at the interactions:

    def interact_with_invaderfleet(self, invader_fleet, fleets):
        if invader_fleet.invader_count() < 8:
            self.die(fleets)

    def interact_with_invaderplayer(self, player, fleets):
        self._player = player

    def interact_with_playershot(self, shot, fleets):
        if Collider(self, shot).colliding():
            explosion = InvadersExplosion.saucer_explosion(self.position, 0.5)
            fleets.append(explosion)
            fleets.append(InvaderScore(self.mystery_score()))
            self.die(fleets)

    def end_interactions(self, fleets):
        if not self.initialized and self._player:
            shot_count = self._player.shot_count % 2
            self.init_motion(shot_count)

Right away that first one catches my eye. If there are fewer than 8 invaders, we die. That’s not what’s supposed to happen! Well, it is, and it isn’t.

The rules for the saucer are that if there are fewer than 8 invaders on screen, the saucer does not appear. I assume that’s to keep it from being too easy to shoot it down, but whatever the reason that’s the rule. And we have two tests that check that:

    def test_does_not_run_with_7_invaders(self):
        ...

    def test_does_run_with_8_invaders(self):
        ...

But that code above will also cause the saucer to die immediately if you shoot the 8th remaining invader. That’s surely not what’s intended. We have noticed a defect! Shall we fix it? I’ve made a note. Let’s stick to our analysis for a bit longer.

Refactoring
A very nice refactoring sequence begins here. Says who? Well, I like it.

The end_interactions is interesting. If we’re not initialized and player is not None, we call init_motion. the variable is called shot count, but that is misleading. Let’s look at that method and the one it calls:

    def end_interactions(self, fleets):
        if not self.initialized and self._player:
            shot_count = self._player.shot_count % 2
            self.init_motion(shot_count)

    def init_motion(self, shot_count):
        self.initialized = True
        speed = 8
        if shot_count == 0:
            self._speed = -speed
            self.rect.center = Vector2(self._right, u.INVADER_SAUCER_Y)
        else:
            self._speed = speed
            self.rect.center = Vector2(self._left, u.INVADER_SAUCER_Y)

If we had to figure this out, we might find it confusing. That’s because it is not as clear as it could be.

Let’s refactor a bit. First, inline the init_motion function. We’ll leave it in place, because there are tests that use it. Python kindly offers that option, and we get this:

    def end_interactions(self, fleets):
        if not self.initialized and self._player:
            shot_count = self._player.shot_count % 2
            self.initialized = True
            speed = 8
            if shot_count == 0:
                self._speed = -speed
                self.rect.center = Vector2(self._right, u.INVADER_SAUCER_Y)
            else:
                self._speed = speed
                self.rect.center = Vector2(self._left, u.INVADER_SAUCER_Y)

    def init_motion(self, shot_count):
        self.initialized = True
        speed = 8
        if shot_count == 0:
            self._speed = -speed
            self.rect.center = Vector2(self._right, u.INVADER_SAUCER_Y)
        else:
            self._speed = speed
            self.rect.center = Vector2(self._left, u.INVADER_SAUCER_Y)

Now, in the end_interactions, inline shot_count:

    def end_interactions(self, fleets):
        if not self.initialized and self._player:
            self.initialized = True
            speed = 8
            if self._player.shot_count % 2 == 0:
                self._speed = -speed
                self.rect.center = Vector2(self._right, u.INVADER_SAUCER_Y)
            else:
                self._speed = speed
                self.rect.center = Vector2(self._left, u.INVADER_SAUCER_Y)

Now, extract self._player.shot_count and move it up a bit:

    def end_interactions(self, fleets):
        if not self.initialized and self._player:
            shot_count = self._player.shot_count
            self.initialized = True
            speed = 8
            if shot_count % 2 == 0:
                self._speed = -speed
                self.rect.center = Vector2(self._right, u.INVADER_SAUCER_Y)
            else:
                self._speed = speed
                self.rect.center = Vector2(self._left, u.INVADER_SAUCER_Y)

Now extract from self.initialized = True on down, back into init_motion. PyCharm will whine but will do it.

    def end_interactions(self, fleets):
        if not self.initialized and self._player:
            shot_count = self._player.shot_count
            self.init_motion(shot_count)

    def init_motion(self, shot_count):
        self.initialized = True
        speed = 8
        if shot_count % 2 == 0:
            self._speed = -speed
            self.rect.center = Vector2(self._right, u.INVADER_SAUCER_Y)
        else:
            self._speed = speed
            self.rect.center = Vector2(self._left, u.INVADER_SAUCER_Y)

Remove the previous copy of the init_motion method. Now inline the parameter:

    def end_interactions(self, fleets):
        if not self.initialized and self._player:
            self.init_motion(self._player.shot_count)

We are green. We are about to commit but let’s take a look here and see if it is more clear: I think it will be.

    def end_interactions(self, fleets):
        if not self.initialized and self._player:
            self.init_motion(self._player.shot_count)

    def init_motion(self, shot_count):
        self.initialized = True
        speed = 8
        if shot_count % 2 == 0:
            self._speed = -speed
            self.rect.center = Vector2(self._right, u.INVADER_SAUCER_Y)
        else:
            self._speed = speed
            self.rect.center = Vector2(self._left, u.INVADER_SAUCER_Y)

Yes! Now we see more clearly that when we init motion, we check to see if the shot count is even or odd. Want to make that more clear? Try this extract:

    def init_motion(self, shot_count):
        self.initialized = True
        speed = 8
        shot_count_is_even = shot_count % 2 == 0
        if shot_count_is_even:
            self._speed = -speed
            self.rect.center = Vector2(self._right, u.INVADER_SAUCER_Y)
        else:
            self._speed = speed
            self.rect.center = Vector2(self._left, u.INVADER_SAUCER_Y)

Let’s commit this and then I have an idea. Well, I have the idea now but let’s try it after we commit this: Improve clarity of saucer’s late init.

We might not like that conditional logic. What about something like this:

Extract a variable, zero_or_one:

    def init_motion(self, shot_count):
        self.initialized = True
        speed = 8
        zero_or_one = shot_count % 2
        shot_count_is_even = zero_or_one == 0
        if shot_count_is_even:
            self._speed = -speed
            self.rect.center = Vector2(self._right, u.INVADER_SAUCER_Y)
        else:
            self._speed = speed
            self.rect.center = Vector2(self._left, u.INVADER_SAUCER_Y)

We are about to do something that might be called “Replace Conditional with Table Lookup” or “Replace Conditional with Data”. I’m sure Martin Fowler must have written it up but I have no Internet until maybe tomorrow, so I’m guessing at the name. In any case, we’re going to replace those if statements with tiny indexed data structures.

Change the first reference to speed like this:

    def init_motion(self, shot_count):
        self.initialized = True
        speed = 8
        zero_or_one = shot_count % 2
        shot_count_is_even = zero_or_one == 0
        if shot_count_is_even:
            self._speed = (-speed, speed)[zero_or_one]
            self.rect.center = Vector2(self._right, u.INVADER_SAUCER_Y)
        else:
            self._speed = speed
            self.rect.center = Vector2(self._left, u.INVADER_SAUCER_Y)

Since zero_or_one is zero when shot_count is even, the new line selects -speed, as before.

Change the second one to be the same:

    def init_motion(self, shot_count):
        self.initialized = True
        speed = 8
        zero_or_one = shot_count % 2
        shot_count_is_even = zero_or_one == 0
        if shot_count_is_even:
            self._speed = (-speed, speed)[zero_or_one]
            self.rect.center = Vector2(self._right, u.INVADER_SAUCER_Y)
        else:
            self._speed = (-speed, speed)[zero_or_one]
            self.rect.center = Vector2(self._left, u.INVADER_SAUCER_Y)

That one, of course, selects speed.

We’re green, by the way. Let’s start committing just to keep Git happy. Commit: refactoring to remove conditional.

Note:
It would have been even better to have moved the assignment out of the if at this point. I didn’t think of that, and rather wish that I had. It would look even more magical. I could, of course, edit what follows, but no, I show what I actually did, not what some even more god-like version of myself might have done.

Now the center. Let’s extract variable left_or_right on both sides of the conditional:

    def init_motion(self, shot_count):
        self.initialized = True
        speed = 8
        zero_or_one = shot_count % 2
        shot_count_is_even = zero_or_one == 0
        if shot_count_is_even:
            self._speed = (-speed, speed)[zero_or_one]
            left_or_right = self._right
            self.rect.center = Vector2(left_or_right, u.INVADER_SAUCER_Y)
        else:
            self._speed = (-speed, speed)[zero_or_one]
            left_or_right = self._left
            self.rect.center = Vector2(left_or_right, u.INVADER_SAUCER_Y)

Commit, same message.

Change both branches to be the same using (self._right, self._left)[zero_or_one]:

    def init_motion(self, shot_count):
        self.initialized = True
        speed = 8
        zero_or_one = shot_count % 2
        shot_count_is_even = zero_or_one == 0
        if shot_count_is_even:
            self._speed = (-speed, speed)[zero_or_one]
            left_or_right = (self._right, self._left)[zero_or_one]
            self.rect.center = Vector2(left_or_right, u.INVADER_SAUCER_Y)
        else:
            self._speed = (-speed, speed)[zero_or_one]
            left_or_right = (self._right, self._left)[zero_or_one]
            self.rect.center = Vector2(left_or_right, u.INVADER_SAUCER_Y)
Note:

Here, the cool move, again, would have been to move one set of duplicated lines up remove the others, see the if statements as empty, and remove them. I wasn’t quite that cool, but I’m still happy with how this went, even if one of my many betters would have done it even more nicely.

Now the two branches are identical. Remove the if else and just keep one:

    def init_motion(self, shot_count):
        self.initialized = True
        speed = 8
        zero_or_one = shot_count % 2
        shot_count_is_even = zero_or_one == 0
        self._speed = (-speed, speed)[zero_or_one]
        left_or_right = (self._right, self._left)[zero_or_one]
        self.rect.center = Vector2(left_or_right, u.INVADER_SAUCER_Y)

Should have committed a couple of times there. Remove the useless line:

    def init_motion(self, shot_count):
        self.initialized = True
        speed = 8
        zero_or_one = shot_count % 2
        self._speed = (-speed, speed)[zero_or_one]
        left_or_right = (self._right, self._left)[zero_or_one]
        self.rect.center = Vector2(left_or_right, u.INVADER_SAUCER_Y)

Commit: remove conditional in init_motion.

I tested in the game and I’ll have you know that I hit the saucer, a legitimate shot. Woot!

Let’s reflect, because I want to bask in what just happened.

Reflection

In a series of very small steps, most of them machine refactorings, we went from this:

    def end_interactions(self, fleets):
        if not self.initialized and self._player:
            shot_count = self._player.shot_count % 2
            self.init_motion(shot_count)

    def init_motion(self, shot_count):
        self.initialized = True
        speed = 8
        if shot_count == 0:
            self._speed = -speed
            self.rect.center = Vector2(self._right, u.INVADER_SAUCER_Y)
        else:
            self._speed = speed
            self.rect.center = Vector2(self._left, u.INVADER_SAUCER_Y)

To this:

    def end_interactions(self, fleets):
        if not self.initialized and self._player:
            self.init_motion(self._player.shot_count)

    def init_motion(self, shot_count):
        self.initialized = True
        speed = 8
        zero_or_one = shot_count % 2
        self._speed = (-speed, speed)[zero_or_one]
        left_or_right = (self._right, self._left)[zero_or_one]
        self.rect.center = Vector2(left_or_right, u.INVADER_SAUCER_Y)

Let’s do one more rename, now that I look at this.

    def init_motion(self, shot_count):
        self.initialized = True
        speed = 8
        even_or_odd = shot_count % 2
        self._speed = (-speed, speed)[even_or_odd]
        left_or_right = (self._right, self._left)[even_or_odd]
        self.rect.center = Vector2(left_or_right, u.INVADER_SAUCER_Y)

I think that’s better, because it retains the notion of shot count being even or odd, which is how the rule is expressed in the ancient scrolls.

However we name it, the new code is two lines shorter and includes no conditional logic. It does do those two little table lookups, and, horses for courses, one might feel that that’s no easier … but I believe I’d leave it this way because even if it does take a second more to think about, it expresses what happens more compactly.

But the real point that I want to make is that we didn’t have to understand what the code was trying to do: every transformation we made was either a machine-controlled rename, extract or inline operation, with just two exceptions, the two table lookups, which, yes, we did have to look at two lines that were similar and see how to make them be the same.

The code now expresses the rule better than it did and the changes were, certainly not mindless—we did have a direction in which we were moving—but they were very simple and didn’t require us to juggle a lot of ideas in our brain.

Could we have done it all by just typing it in? Maybe you could have, but I didn’t even have in mind getting rid of the conditional until right before I did it. I just wanted to make things a bit more clear.

I find a particular joy in finding a series of machine refactorings and name choices that lead me to better code. Here, I did actually have to come up with and implement the idea of the lookups, but by the time I got there, everything else was so simple that even I could fit it into my brain.

Perhaps you, too, might enjoy finding tiny refactoring steps like this to improve your code. How could you find out?

Now What?

I am about to delete 200 lines of article, including some thinking that I found interesting and a couple of very minor refactorings, renames, if inversions, that sort of thing. I’m removing it all because I don’t think it’s interesting enough or helpful enough to trouble you with it. Herewith, we jump 200 lines to

Summary

After just a bit of warming up, we undertook a very nice sequence of refactoring steps that took us, inch by inch, step by step, to a somewhat nicer place. In all, we just removed a couple of lines and a couple of conditionals, but the sequence itself was almost entirely done with machine refactorings and had a nice pair of “replace conditional with data” steps.

Thereafter, in 200 deleted lines, I thought about ways to eliminate the checks for initialized, hoping to simplify the class, but while I did have some interesting ideas, none of them seem to hold water. Fine. Not every thought has to be brilliant. And we may yet some up with something better than what we have. If not, it’s already better than it was.

Enough. Rest your eyes, if you’re even here. See you next time!