Python Asteroids+Invaders on GitHub

I have yet another idea for how to get rid of the conditionals in the saucer code. This one might actually be pretty decent.

The invaders saucer has a two-phase initialization, because it needs information about its surroundings, specifically the number of shots that the player has fired, and the number of aliens on the screen. That has been implemented1 using an undesirable boolean flag and four if statements. It looks like this:

class InvadersSaucer(InvadersFlyer):
    def __init__(self, direction=1):
        self.direction = direction
        ...
        self.initialized = False

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

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

    def update(self, delta_time, fleets):
        if self.initialized:
            self.move_or_die(fleets)

    def draw(self, screen):
        if self.initialized:
            screen.blit(self._map, self.rect)

When we look at the positive-side ifs, we see that we are just skipping over drawing and moving. That’s because our position and speed are not yet known. The two negative-side ifs are a bit more tricky. One of them checks to see if the invader count is less than 8 and just kills the saucer, because the saucer does not appear when there are too few aliens. We wouldn’t want to make it too easy. And other negative if checks to see if we have a player and if so, initializes our motion, using the player shot count.

Let’s look at one more method:

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

It seems to me that we could initialize motion right there. Let’s check:

    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)

Yes. If we did that, it seems to me that the code could be simpler, like this:

    def interact_with_invaderplayer(self, player, fleets):
        self._player = player
        if not self.initialized:
            self.init_motion(self._player.shot_count)

    def end_interactions(self, fleets):
        pass

That’s definitely simpler. Do we use the player for anything else? Yes, we use it to decide the saucer’s score, so we do have to keep the assignment.

One more change. I noticed the direction member up in __init__ and it turns out that we no longer use it. Let’s eliminate that. Done. Commit: tidying.

Now let’s see about getting rid of the if-checks of initialized and the flag itself.

I propose to replace the boolean initialized flag with a state object, maybe called readiness. There will be two possible values in there, Ready, or Unready. Those two little classes will each hold onto the saucer, their parent, and will implement four methods, for the four different things we do under control of the current flag, namely initialize motion, die if there are not enough invaders, update, and draw.

To make this easier, I want to extract a method for each of the references to the flag. Some of them are there already. We’ll start with this on:

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

First, I ask PyCharm to split the and. It gives me this:

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

Do you feel like committing a lot? We can commit now. Let’s do, it’s good practice. Commit: preparing for state objects.

Next there’s this:

    def interact_with_invaderplayer(self, player, fleets):
        self._player = player
        if not self.initialized:
            self.init_motion(self._player.shot_count)

We’ll let that stand. Let me comment on what I think I’m doing.

Comment
I’m not entirely sure exactly what I’m doing. I propose to write two little objects, one for ready and one for unready, storied in readiness, and for these four conditioned methods to call their corresponding method on readiness, which will either do something or not, depending on which is appropriate. I am confident that by the time we’re done, it will be quite clear.

But right now, I’m not clear just what we’ll need: I propose to find out by doing it. So I want to get what’s under the if statements down to one call to self, but I’m not quite sure whether there’s more to it than that.

Curiously, I don’t care about not knowing. I am confident that I’ll find out, and that if something needs to be a bit different, we can improve it.

Let’s see what happens.

Moving on to the next one:

    def end_interactions(self, fleets):
        pass

That one, I just remove. It can be inherited.

    def update(self, delta_time, fleets):
        if self.initialized:
            self.move_or_die(fleets)

That’s just fine, let it be.

    def draw(self, screen):
        if self.initialized:
            screen.blit(self._map, self.rect)

Here, I want to extract a method. I’ll call it … just_draw.

    def draw(self, screen):
        if self.initialized:
            self.just_draw(screen)

    def just_draw(self, screen):
        screen.blit(self._map, self.rect)

Commit: preparing for state objects.

Now we want to do the two little state objects. We’ll start with draw because it seems like one of the easiest.

I propose to have two state objects, Unready and Ready, which receive the saucer and save it, and which implement four methods for the four uses we have of the flag. I propose that we’ll have a new member, readiness, that will hole, first, an Unready instance and then, later, a Ready.

I proceed by intention2, first in __init__:

class InvadersSaucer(InvadersFlyer):
    def __init__(self):
        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._readiness = Unready(self)
        self.initialized = False

This demands a small class:

class Unready:
    def __init__(self, saucer):
        self._saucer = saucer

Green. Might as well commit: building Unready state. Note that I am committing this code. I expect to find it satisfactory and I’d rather commit it than not. Yes, I could branch, but I am not a branch kinda guy.

I was going to draw next, but I think the fleet interaction will be better. It looks like this:

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

Ah. I wanted to have what’s under the if call a specific self method. Extract Method:

    def interact_with_invaderfleet(self, invader_fleet, fleets):
        if not self.initialized:
            self.die_if_lonely(fleets, invader_fleet)

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

Now my plan, to the extent that I have one, is that in the interact we’ll call a method on _readiness. I think we’ll either call back or not, so let’s assume it’ll have the same signature as the method called by the if. So:

    def interact_with_invaderfleet(self, invader_fleet, fleets):
        self._readiness.die_if_lonely(invader_fleet, fleets)

Now, of course, we need that method on Unready:

class Unready:
    def __init__(self, saucer):
        self._saucer = saucer

    def die_if_lonely(self, invader_fleet, fleets):
        self._saucer.die_if_lonely(invader_fleet, fleets)

Tests have borked. I am distressed, but not very. They are there to break when things go wrong3. It’s not too surprising that the tests are the ones that check whether we die as advertised.

    def die_if_lonely(self, fleets, invader_fleet):
>       if invader_fleet.invader_count() < 8:
E       AttributeError: 'Fleets' object has no attribute 'invader_count'

Well, duh, I turned the arguments around. Well, PyCharm did it and I accepted it. Easy fix:

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

Green. Not going to commit yet, because we don’t have the other state object, nor are we using it. I don’t have a brilliant idea for how to test-drive this, but I would like some reason other than deep knowledge to cause me to write the Ready class and its corresponding method.

    def test_sets_up_ready(self):
        saucer = InvadersSaucer()
        assert isinstance(saucer._readiness, Unready)
        saucer.init_motion(0)
        assert isinstance(saucer._readiness, Ready)

That won’t compile for want of Ready. I’ll build it, going a bit beyond the scope of the test. I do have an idea here, and while it’s not in the test, it is in my head. We should do it the other way, but I am imperfect.

class Ready:
    def __init__(self, saucer):
        self._saucer = saucer

    def die_if_lonely(self, _invader_fleet, _fleets):
        pass

    def init_motion(self, shot_count):
        self._readiness = Ready(self)
        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)

Green. Commit: readiness objects handle die_if_lonely.

Reflection

We have enough in place to see what goes on with these two little objects. They are what we might call reflections of each other. If Unready does a thing, then Ready does not do it, and vice versa. So far there’s just the one method pair:

class Unready:
    def die_if_lonely(self, invader_fleet, fleets):
        self._saucer.die_if_lonely(invader_fleet, fleets)

class Ready:
    def die_if_lonely(self, _invader_fleet, _fleets):
        pass

We set up an Unready instance when we initialize the saucer:

class InvadersSaucer(InvadersFlyer):
    def __init__(self):
        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._readiness = Unready(self)  # <=====
        self.initialized = False

And we replace it with a Ready instance when we finish initialization:

    def init_motion(self, shot_count):
        self._readiness = Ready(self)
        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)

Let’s rename that init_motion to finish_initializing:

    def finish_initializing(self, shot_count):
        self._readiness = Ready(self)
        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)

The _readiness object contains one or the other of an Unready or Ready, and the saucer behaves a bit differently depending on _readiness. Just what we wanted, and it happens without checking the flag. Or it will, when we’re done.

Let’s do draw. It starts like this:

    def draw(self, screen):
        if self.initialized:
            self.just_draw(screen)

And we finish with:

    def draw(self, screen):
        self._readiness.just_draw(screen)

class Unready:
    def just_draw(self, screen):
        pass

class Ready:
    def just_draw(self, screen):
        self._saucer.just_draw(screen)

That one has to be tested in the game, and it works as advertised.

What’s left?

    def interact_with_invaderplayer(self, player, fleets):
        self._player = player
        if not self.initialized:
            self.finish_initializing(self._player.shot_count)

Right. This one gets action in Unready and pass in Ready:

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

class Unready:
    def __init__(self, saucer):
    def finish_initializing(self, shot_count):
        self._saucer.finish_initializing(shot_count)

class Ready:
    def finish_initializing(self, shot_count):
        pass

Commit, I keep forgetting to commit: readiness handles drawing and finishing init.

Just one more, the update:

    def update(self, delta_time, fleets):
        if self.initialized:
            self.move_or_die(fleets)

That becomes:

    def update(self, delta_time, fleets):
        self._readiness.move_or_die(fleets)

class Unready:
    def move_or_die(self, fleets):
        pass

class Ready:
    def move_or_die(self, fleets):
        self._saucer.move_or_die(fleets)

And we are green. Should be no references to the flag. Well, except for a test:

    def test_initialized(self):
        player = InvaderPlayer()
        saucer = InvadersSaucer()
        assert not saucer.initialized
        saucer.interact_with_invaderplayer(player, [])
        saucer.end_interactions([])
        assert saucer.initialized

Let’s just check the new thingie:

    def test_initialized(self):
        player = InvaderPlayer()
        saucer = InvadersSaucer()
        assert isinstance(saucer._readiness, Unready)
        saucer.interact_with_invaderplayer(player, [])
        saucer.end_interactions([])
        assert isinstance(saucer._readiness, Ready)

Now remove the member. Commit: Saucer now uses readiness state objects Unready and Ready to manage two-stage initialization.

Let’s review the code and sum up:

Review

class Unready:
    def __init__(self, saucer):
        self._saucer = saucer

    def die_if_lonely(self, invader_fleet, fleets):
        self._saucer.die_if_lonely(invader_fleet, fleets)

    def finish_initializing(self, shot_count):
        self._saucer.finish_initializing(shot_count)

    def just_draw(self, screen):
        pass

    def move_or_die(self, fleets):
        pass


class Ready:
    def __init__(self, saucer):
        self._saucer = saucer

    def die_if_lonely(self, _invader_fleet, _fleets):
        pass

    def finish_initializing(self, shot_count):
        pass

    def just_draw(self, screen):
        self._saucer.just_draw(screen)

    def move_or_die(self, fleets):
        self._saucer.move_or_die(fleets)

We have two symmetric classes, Unready and Ready, implementing the four items that we need to do conditionally. Two are done when unready, two when ready. The calls, which used to include if self.initialized, now read like this:

    def interact_with_invaderfleet(self, invader_fleet, fleets):
        self._readiness.die_if_lonely(invader_fleet, fleets)

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

    def update(self, delta_time, fleets):
        self._readiness.move_or_die(fleets)

    def draw(self, screen):
        self._readiness.just_draw(screen)

We could probably smooth that invaderplayer one to look like the others, by moving the player assignment into finish and passing player through the readiness guys. However, there are tests that are passing in the shot count, so we’ll hold off on that. Good enough for now. I’m tired.

Summary

Tired, yes, but I am also fairly pleased. There are those who feel that if statements in object-oriented programming are a small but significant code smell, often signalling that something needs improvement. The improvement always seems to involve some new object helping out, as we created here. We are left with a question, however:

Is this code actually better than what we had before?

Well, it depends what you like, I guess. The conditional versions did express, in each case, what was at stake (initialized?) and what was to be done depending on that flag. It was all right there in front of us.

In the new scheme, the conditionality is almost invisible. Of course when we look at what _readiness does, it’s pretty clear, but it does requires us to look at each of Ready and Unready to get the complete picture.

On the other hand, the new scheme packages all the conditional behavior together. We see all four of the conditioned actions and we can tell when they are called and when they are not.

For me, this code is more like what I like, even though quite often I do use conditionals and I don’t intend to stop. But these initialized ones have been bugging me for a while and, to me, this code looks more like the best O-O code I’ve eve seen. The interact_with_invader_player bugs me a bit, and I’ll almost certainly do what’s necessary to make it look more like the others.

I am nearly satisfied with this, subject only to the invader_player issue, which we’ll resolve either in an article or offline. I am definitely pleased with the change. YMMV and you get to use this idea, or not, at your pleasure.

If you want to kick the idea around a bit, toot me up.

See you next time!


  1. Look at me, using the passive voice to hide who has implemented this too-iffy code, like maybe spirits came in at night and did it while I slept. I chose the flag approach. And I’m also the one who doesn’t like it. ↩

  2. I learned the notion of “programming by intention” from Kent Beck. The idea is that you write calls to code you intend to have, and then you fulfill the intention by writing the necessary code. It’s a bit like TDD, except that your existing code that won’t compile is acting as the test. Programming by intention works quite nicely in conjunction with TDD as well. However, at least for now, I don’t propose to write new tests for the Ready and Unready objects. I think existing tests will cover most of what we need. ↩

  3. That actually means “When I inevitably do something wrong.” ↩