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

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]
``````

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):
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:
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._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
``````

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:
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(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):
``````

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

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

``````

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):
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):
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.init_motion(0)
``````

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

pass

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]
``````

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:

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._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
``````

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

``````    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]
``````

Let’s rename that `init_motion` to `finish_initializing`:

``````    def finish_initializing(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]
``````

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):

def just_draw(self, screen):
pass

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)
``````

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

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

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):

def move_or_die(self, fleets):
pass

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):
assert not saucer.initialized
saucer.end_interactions([])
assert saucer.initialized
``````

Let’s just check the new thingie:

``````    def test_initialized(self):
saucer.end_interactions([])
``````

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 finish_initializing(self, shot_count):
self._saucer.finish_initializing(shot_count)

def just_draw(self, screen):
pass

def move_or_die(self, fleets):
pass

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

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._player = player

def update(self, delta_time, fleets):

def draw(self, 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.”