P-285: Remove Conditionals

Python Asteroids+Invaders on GitHub

I propose to remove some if statements today. Why might we do that? Do we like the result? Readers may disagree. I might even disagree with myself.

We’ve been looking at PlayerMaker, and today we’re looking at its conditional statements.PlayerMaker looks like this:

class PlayerMaker(InvadersFlyer):
    def __init__(self):
        self.reserve = None
        self.player_missing = True

    def interact_with(self, other, fleets):
        other.interact_with_playermaker(self, fleets)

    def begin_interactions(self, _fleets):
        self.reserve = None
        self.player_missing = True

    def interact_with_invaderplayer(self, _player, _fleets):
        self.player_missing = False

    def interact_with_reserveplayer(self, reserve, _fleets):
        self.remember_rightmost_reserve_player(reserve)

    def remember_rightmost_reserve_player(self, reserve: ReservePlayer):
        if not self.reserve:
            self.reserve = reserve
        elif reserve.is_to_the_right_of(self.reserve):
            self.reserve = reserve

    def end_interactions(self, fleets):
        if self.player_missing:
            if self.reserve:
                self.give_player_another_turn(fleets)
            else:
                self.game_over(fleets)

    def give_player_another_turn(self, fleets):
        fleets.remove(self)
        delay_until_new_player = 2.0
        a_bit_longer = 0.1
        self.provide_new_player(delay_until_new_player, fleets)
        self.provide_new_maker(delay_until_new_player + a_bit_longer, fleets)

    def provide_new_player(self, delay, fleets):
        player_capsule = TimeCapsule(delay, InvaderPlayer(), self.reserve)
        fleets.append(player_capsule)

    def provide_new_maker(self, delay, fleets):
        maker_capsule = TimeCapsule(delay, PlayerMaker())
        fleets.append(maker_capsule)

    def game_over(self, fleets):
        coin.slug(fleets)

Let’s focus in on just the methods with conditionals, and their initial states:

    def begin_interactions(self, _fleets):
        self.reserve = None
        self.player_missing = True

    def remember_rightmost_reserve_player(self, reserve: ReservePlayer):
        if not self.reserve:
            self.reserve = reserve
        elif reserve.is_to_the_right_of(self.reserve):
            self.reserve = reserve

    def end_interactions(self, fleets):
        if self.player_missing:
            if self.reserve:
                self.give_player_another_turn(fleets)
            else:
                self.game_over(fleets)

What we’re going to do today is see if we can remove those if statements, and if we prefer the code having done so, if everything goes as I have planned, as if it ever does. One important question is why we would even want to do this. I can think of some reasons, but I suspect that the main reason this came to mind is a bit of discussion from last night’s Zoom Ensemble, where we spoke about the execution pipeline in a processor.

We were looking at a statement in Ken’s program, which was accounting for 80 percent of the CPU time:

    if (*p++) {

I jokingly remarked that we were unlikely to be able to optimize that, and Hill remarked that he had seen a case where inverting an if condition had sped things up by a factor of four, because the if condition, as written, was stalling the CPU pipeline, because the compiler had assumed incorrectly which branch was most probable.

Well, I’m pretty sure that’s not the problem in my Python code, because the CPU pipeline is way the heck away from anything you can write in Python, but there is a similar effect, metaphorically speaking, on the human mind when we try to understand conditional code. We kind of have to stall and begin to think, at least a bit, about both possibilities, and then perhaps pick one to explore, and then, very likely, have to come back to explore the other.

So one reason for avoiding conditionals is arguably to keep the cognitive load low as we read (and write) the code. A related reason is that conditionals are a big source of errors, as we can get them wrong quite easily. This becomes more and more true as we make them more complicated, as, for some reason, we often do.

Here’s a place where I am tempted to do something complicated:

    def remember_rightmost_reserve_player(self, reserve: ReservePlayer):
        if not self.reserve:
            self.reserve = reserve
        elif reserve.is_to_the_right_of(self.reserve):
            self.reserve = reserve

Both sides of this if do the same operation, setting self.reserve. So we should be able to make that decision in a single if. Should it be this?

    def remember_rightmost_reserve_player(self, reserve: ReservePlayer):
        if not self.reserve or reserve.is_to_the_right_of(self.reserve):
            self.reserve = reserve

It turns out that that works. It is also excruciatingly hard to reason about, with that not and or in there, and relying on the fact that if not self.reserve is True, the second clause will not be executed, because even Python is at least that smart. If Python did not optimize and and or, that code would fail.

Hard to reason about. And ever so tempting to do the clever thing. FA now, FO later.

So, enough rationalizing. We’re here to explore removing these conditionals, to see whether we prefer things without them. Let’s get to it.

Analyzing the Flow

Let’s consider what goes on when PlayerMaker runs.

Look first at the begin and end:

    def begin_interactions(self, _fleets):
        self.reserve = None
        self.player_missing = True

    def end_interactions(self, fleets):
        if self.player_missing:
            if self.reserve:
                self.give_player_another_turn(fleets)
            else:
                self.game_over(fleets)

If no interactions happen, we’ll do self.game_over(fleets). When there is no player and no reserve, it’s game over.

And, if there is a player (not player_missing), we do nothing.

And, if there is no player and there is a reserve, we give_player_another_turn.

Now a glance at the player interaction:

    def interact_with_invaderplayer(self, _player, _fleets):
        self.player_missing = False

OK, pretty simple there.

Now I am really just about ready to do this. Here is what I can see so far:

Have a member variable final_action;
Initialize it to game_over;
When we see the player, set it to do_nothing;

But what about the reserve? If there is no player, we care about the reserve but we don’t care otherwise. We do not know whether we’ll see the player or the reserve first.

We haven’t looked at the interaction for reserveplayer for a while. It is this:

    def interact_with_reserveplayer(self, reserve, _fleets):
        self.remember_rightmost_reserve_player(reserve)

    def remember_rightmost_reserve_player(self, reserve: ReservePlayer):
        if not self.reserve:
            self.reserve = reserve
        elif reserve.is_to_the_right_of(self.reserve):
            self.reserve = reserve

So, how about this:

Have a member variable final_action;
Initialize it to missing_player_action;
When we see the player, set it to do_nothing;
Have a member variable ...

I don’t quite see it. Let’s refactor to get there.

First, let’s extract a method from the end_interactions:

    def end_interactions(self, fleets):
        self.deal_with_missing_player(fleets)

    def deal_with_missing_player(self, fleets):
        if self.player_missing:
            if self.reserve:
                self.give_player_another_turn(fleets)
            else:
                self.game_over(fleets)

Ah. Commit that. Now I can see a step that we can take:

Have a member variable final_action;
Initialize it to deal_with_missing_player;
Call it from end_interactions.

    def begin_interactions(self, _fleets):
        self.reserve = None
        self.player_missing = True
        self.final_action = self.deal_with_missing_player

    def end_interactions(self, fleets):
        self.final_action(fleets)

    def deal_with_missing_player(self, fleets):
        if self.player_missing:
            if self.reserve:
                self.give_player_another_turn(fleets)
            else:
                self.game_over(fleets)

That’s solid. Commit: add final_action member.

Add a do_nothing method:

    def do_nothing(self, fleets):
        pass

When we see the player, set final_action to do_nothing:

    def interact_with_invaderplayer(self, _player, _fleets):
        self.final_action = self.do_nothing

This breaks a test.

    def test_notices_player(self):
        maker = PlayerMaker()
        maker.begin_interactions(None)
        assert maker.player_missing
        maker.interact_with_invaderplayer(None, None)
        assert not maker.player_missing

We can recast that test thus:

    def test_notices_player(self):
        maker = PlayerMaker()
        maker.begin_interactions(None)
        final = maker.final_action
        maker.interact_with_invaderplayer(None, None)
        assert maker.final_action is not final

Passes. We can commit, so let’s do: player arrival sets final_action to do_nothing.

But now look here:

    def deal_with_missing_player(self, fleets):
        if self.player_missing:
            if self.reserve:
                self.give_player_another_turn(fleets)
            else:
                self.game_over(fleets)

We now only get here if the player is missing, so we can remove that check. And remove the player_missing variable altogether.

    def deal_with_missing_player(self, fleets):
        if self.reserve:
            self.give_player_another_turn(fleets)
        else:
            self.game_over(fleets)

    def begin_interactions(self, _fleets):
        self.reserve = None
        self.final_action = self.deal_with_missing_player

We have removed one if. Commit: remove player_missing and conditional behavior.

What now? How about a reserve_action? It should be initialized to game_over.

    def begin_interactions(self, _fleets):
        self.reserve = None
        self.final_action = self.deal_with_missing_player
        self.reserve_action = self.game_over

    def interact_with_reserveplayer(self, reserve, _fleets):
        self.remember_rightmost_reserve_player(reserve)
        self.reserve_action = self.give_player_another_turn

And with those in place, can’t we do this?

    def deal_with_missing_player(self, fleets):
        self.reserve_action(fleets)

We can. Another if gone. Commit: remove conditional behavior from deal_with_missing_player.

We now only have one if statement left in the object:

    def remember_rightmost_reserve_player(self, reserve: ReservePlayer):
        if not self.reserve:
            self.reserve = reserve
        elif reserve.is_to_the_right_of(self.reserve):
            self.reserve = reserve

Maybe ReservePlayer could help us. One possibility would be to initialize our self.reserve to a ReservePlayer that will never be right-most. We’ll do it by hand and then see whether we can do something more intelligent.

    def begin_interactions(self, _fleets):
        self.reserve = ReservePlayer(-999)
        self.final_action = self.deal_with_missing_player
        self.reserve_action = self.game_over

A test breaks. Easily fixed:

    def test_captures_reserve(self):
        maker = PlayerMaker()
        maker.interact_with_reserveplayer(ReservePlayer(1), None)
        maker.interact_with_reserveplayer(correct :=ReservePlayer(2), None)
        maker.interact_with_reserveplayer(ReservePlayer(0), None)
        assert maker.reserve == correct
        maker.begin_interactions(None)
        assert maker.reserve.reserve_number < 0

Now there is always a ReservePlayer available, so we can remove part of our if:

    def remember_rightmost_reserve_player(self, reserve: ReservePlayer):
        if reserve.is_to_the_right_of(self.reserve):
            self.reserve = reserve

Let’s get some help from ReservePlayer:

    def remember_rightmost_reserve_player(self, reserve: ReservePlayer):
        self.reserve = self.reserve.rightmost_of(reserve)

    def rightmost_of(self, another_reserve_player):
        if self.is_to_the_right_of(another_reserve_player):
            return self
        else:
            return another_reserve_player

I know that looks like we just traded one if for another even worse if, but give me a moment here.

We are still green as your finger after wearing your Captain Midnight Secret Decoder Ring all week, so commit: no conditionals in PlayerMaker.

class PlayerMaker(InvadersFlyer):
    def __init__(self):
        self.reserve = ReservePlayer(-999)
        self.final_action = self.deal_with_missing_player
        self.reserve_action = self.game_over

    @property
    def mask(self):
        return None

    @property
    def rect(self):
        return None

    def interact_with(self, other, fleets):
        other.interact_with_playermaker(self, fleets)

    def begin_interactions(self, _fleets):
        self.reserve = ReservePlayer(-999)
        self.final_action = self.deal_with_missing_player
        self.reserve_action = self.game_over

    def interact_with_invaderplayer(self, _player, _fleets):
        self.final_action = self.do_nothing

    def interact_with_reserveplayer(self, reserve, _fleets):
        self.remember_rightmost_reserve_player(reserve)
        self.reserve_action = self.give_player_another_turn

    def remember_rightmost_reserve_player(self, reserve: ReservePlayer):
        self.reserve = self.reserve.rightmost_of(reserve)

    def end_interactions(self, fleets):
        self.final_action(fleets)

    def deal_with_missing_player(self, fleets):
        self.reserve_action(fleets)

    def give_player_another_turn(self, fleets):
        fleets.remove(self)
        delay_until_new_player = 2.0
        a_bit_longer = 0.1
        self.provide_new_player(delay_until_new_player, fleets)
        self.provide_new_maker(delay_until_new_player + a_bit_longer, fleets)

    def provide_new_player(self, delay, fleets):
        player_capsule = TimeCapsule(delay, InvaderPlayer(), self.reserve)
        fleets.append(player_capsule)

    def provide_new_maker(self, delay, fleets):
        maker_capsule = TimeCapsule(delay, PlayerMaker())
        fleets.append(maker_capsule)

    def game_over(self, fleets):
        coin.slug(fleets)

    def do_nothing(self, fleets):
        pass

We’ll assess that code in a moment, but first we have another conditional to get rid of, lest we be laughed off the field.

class ReservePlayer(InvadersFlyer):
    def is_to_the_right_of(self, another_reserve_player):
        return self.reserve_number > another_reserve_player.reserve_number

    def rightmost_of(self, another_reserve_player):
        if self.is_to_the_right_of(another_reserve_player):
            return self
        else:
            return another_reserve_player

Simples.

    def rightmost_of(self, another_reserve_player):
        return max(self, another_reserve_player)

Only one problem: we do not yet support max. We need greater-than behavior.

    def __gt__(self, other):
        return self.reserve_number > other.reserve_number

We are green. Commit: remove conditional, implement gt.

We’ve removed all the conditionals from PlayerMaker and quickly removed the one we added to ReservePlayer. Let’s reflect:

Reflection

In ReservePlayer, we just did barely enough to make the max function work. It’s sufficient, but not very robust. And, thinking about how to robustify it, I have the feeling that we’ll need some conditional behavior. But it is sufficient to our purpose, at least for now.

What about PlayerMaker?

I think that as it stands, it’s not quite easy enough to see what’s going to happen. Let me try some renaming.

class PlayerMaker(InvadersFlyer):
    def __init__(self):
        self.reserve = ReservePlayer(-999)
        self.final_action = self.final_deal_with_missing_player
        self.reserve_action = self.reserve_absent_game_over

    @property
    def mask(self):
        return None

    @property
    def rect(self):
        return None

    def interact_with(self, other, fleets):
        other.interact_with_playermaker(self, fleets)

    def begin_interactions(self, _fleets):
        self.reserve = ReservePlayer(-999)
        self.final_action = self.final_deal_with_missing_player
        self.reserve_action = self.reserve_absent_game_over

    def interact_with_invaderplayer(self, _player, _fleets):
        self.final_action = self.final_do_nothing

    def interact_with_reserveplayer(self, reserve, _fleets):
        self.reserve = self.reserve.rightmost_of(reserve)
        self.reserve_action = self.reserve_give_player_another_turn

    def end_interactions(self, fleets):
        self.final_action(fleets)

    def final_deal_with_missing_player(self, fleets):
        self.reserve_action(fleets)

    def final_do_nothing(self, fleets):
        pass

    def reserve_absent_game_over(self, fleets):
        coin.slug(fleets)

    def reserve_give_player_another_turn(self, fleets):
        fleets.remove(self)
        delay_until_new_player = 2.0
        a_bit_longer = 0.1
        self.provide_new_player(delay_until_new_player, fleets)
        self.provide_new_maker(delay_until_new_player + a_bit_longer, fleets)

    def provide_new_player(self, delay, fleets):
        player_capsule = TimeCapsule(delay, InvaderPlayer(), self.reserve)
        fleets.append(player_capsule)

    def provide_new_maker(self, delay, fleets):
        maker_capsule = TimeCapsule(delay, PlayerMaker())
        fleets.append(maker_capsule)

I’ve renamed the two methods that might be called from final_action to have final in their name, and similarly for the reserve methods.

My assessment at this moment is that we have gained something and lost something. With the conditionals removed, we don’t have to start thinking about the states of the various flags, whether they are covered, and so on. In fact, at first glance, we don’t even see that this object has conditional behavior.

We would already know that it probably has conditional behavior, because any Flyer that interacts almost certainly behaves differently depending on what it interacts with, but this one is on the tricky side. Any Flyer that has behavior in end_interactions has surely saved up some information for later use, and is therefore a bit more complicated.

So we gain by not worrying about conditions … but when we set out to understand how this object actually works, is our job made easier by this implementation, or harder?

The answer will depend on what we’re used to, but I’d listen to someone arguing that, hey, when the conditionals were there, at least I knew where to look. Now it’s harder to see where the conditional behavior happens.

For someone who is more heavily into tiny objects, pluggable behavior, strategy kinds of objects, and the like, this will seem more OK.

A major thing that I do not like about this implementation as it stands is that there are two methods that appear to be calling a method and are in fact calling a pluggable member variable that contains a method:

    def end_interactions(self, fleets):
        self.final_action(fleets)

    def final_deal_with_missing_player(self, fleets):
        self.reserve_action(fleets)

Let’s try something. Let’s add pluggable to those two member variables.

    def end_interactions(self, fleets):
        self.pluggable_final_action(fleets)

    def final_deal_with_missing_player(self, fleets):
        self.pluggable_reserve_action(fleets)

Oh, I think I like that. I’ll put all the relevant code below for a final review, but first a summary:

Summary

We have seen a series of simple steps, with commits in between, that allowed us to replace conditional statements with pluggable behavior. We have successfully accomplished what we set out to do, which was to remove all the conditionals in this object.

Did you notice that when I tried to think through the changes, I was unable to do it, but that extracting a method with a somewhat reasonable name opened the door to a sensible series of changes? I find that that often happens. When I let the code participate in my thinking, I seem often to make better progress. (Thanks to Kent Beck for that metaphor.)

It is certainly a matter of taste whether this is “better”. I think there’s unquestionably value in knowing how to do this, and that it is a capability that needs to be done wisely. I’ll leave it to you to decide whether it was truly wise here, or not.

One final thought: there is probably another way to have done this, using objects where we used simple pluggable methods. We might explore that notion here, or elsewhere, in a subsequent article.

See you next time!

class PlayerMaker(InvadersFlyer):
    def __init__(self):
        self.reserve = ReservePlayer(-999)
        self.pluggable_final_action = self.final_deal_with_missing_player
        self.pluggable_reserve_action = self.reserve_absent_game_over

    @property
    def mask(self):
        return None

    @property
    def rect(self):
        return None

    def interact_with(self, other, fleets):
        other.interact_with_playermaker(self, fleets)

    def begin_interactions(self, _fleets):
        self.reserve = ReservePlayer(-999)
        self.pluggable_final_action = self.final_deal_with_missing_player
        self.pluggable_reserve_action = self.reserve_absent_game_over

    def interact_with_invaderplayer(self, _player, _fleets):
        self.pluggable_final_action = self.final_do_nothing

    def interact_with_reserveplayer(self, reserve, _fleets):
        self.reserve = self.reserve.rightmost_of(reserve)
        self.pluggable_reserve_action = self.reserve_give_player_another_turn

    def end_interactions(self, fleets):
        self.pluggable_final_action(fleets)

    def final_deal_with_missing_player(self, fleets):
        self.pluggable_reserve_action(fleets)

    def final_do_nothing(self, fleets):
        pass

    def reserve_absent_game_over(self, fleets):
        coin.slug(fleets)

    def reserve_give_player_another_turn(self, fleets):
        fleets.remove(self)
        delay_until_new_player = 2.0
        a_bit_longer = 0.1
        self.provide_new_player(delay_until_new_player, fleets)
        self.provide_new_maker(delay_until_new_player + a_bit_longer, fleets)

    def provide_new_player(self, delay, fleets):
        player_capsule = TimeCapsule(delay, InvaderPlayer(), self.reserve)
        fleets.append(player_capsule)

    def provide_new_maker(self, delay, fleets):
        maker_capsule = TimeCapsule(delay, PlayerMaker())
        fleets.append(maker_capsule)

Congratulations, you made it to the end!