Python Asteroids+Invaders on GitHub

One of Kent Beck’s perceptive Rules of Simple Design is the one about expressing all design ideas. Some code up in this thing bothers me.

We’ve worked on this object more than once, and as I was looking at it yesterday, I realized that it still doesn’t express what it does very well at all.

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

    @property
    def mask(self):
        return None

    @property
    def rect(self):
        return None

    def perform(self, callable_function, arg):
        return callable_function(arg)

    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 interact_with_robotplayer(self, bumper, fleets):
        self.pluggable_reserve_action = self.final_do_nothing
        self.pluggable_final_action = self.final_do_nothing

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

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

    def final_do_nothing(self, fleets):
        pass

    def reserve_absent_game_over(self, fleets):
        fleets.remove(self)
        fleets.append(InvadersGameOver())
        robot = RobotPlayer()
        capsule = TimeCapsule(2.0, robot)
        fleets.append(capsule)
        maker = PlayerMaker()
        maker_capsule = TimeCapsule(2.1, maker)
        fleets.append(maker_capsule)

    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)

If you want to take a moment to try to figure that out, please do. After you’ve had a look, I’ll try to explain what this thing does.

The object’s purpose is to notice when there is no Player in the mix, which will happen after the Player gets hit by an invader shot. (It will also happen if the invaders get to the bottom of the screen.)

If there is a Player, the object should do nothing.

If there is no Player, this object decides what needs to happen next and then does what’s needed:

If there is at least one ReservePlayer:

  1. Create a new InvaderPlayer and schedule it for creation in two seconds, using a TimeCapsule.
  2. Create a new PlayerMaker and schedule it for creation a bit after that.
  3. Remove itself, so that no further decisions will be made regarding the current missing Player.

If there are no ReservePlayers left, the game is over:

  1. Create a GameOver object and add it to the mix immediately.
  2. Create a new RobotPlayer and schedule it for creation in two seconds, using a TimeCapsule.
  3. Create a new PlayerMaker and schedule it for creation a bit after that.
  4. Remove itself, so that no further decisions will be made regarding the current missing Player.

I kind of went out of my way to express those two cases with the same words, because I started to notice that they are quite similar, and that the code for them is somewhat duplicated but not quite:

    def reserve_absent_game_over(self, fleets):
        fleets.remove(self)
        fleets.append(InvadersGameOver())
        robot = RobotPlayer()
        capsule = TimeCapsule(2.0, robot)
        fleets.append(capsule)
        maker = PlayerMaker()
        maker_capsule = TimeCapsule(2.1, maker)
        fleets.append(maker_capsule)

    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)

The object, as currently written (yes, I wrote it this way, I’m not claiming that elves wrote it or that I found it in the road), uses pluggable behavior in two member variables.

The pluggable_final_action either does final_do_nothing (if a Player is seen) or does final_deal_with_missing_player if the player is missing.

The pluggable_reserve_action either does reserve_absent_game_over (if there is no ReservePlayer) or does reserve_give_player_another_turn if there is a reserve left.

I honestly don’t remember what we had before this. That was way back around Python 285, who can remember that far back.\? In those dark days last November the object was implemented with some if statements that kept track of what had been seen. In a couple of articles, we moved the code to what we have here and, at the time, I preferred it.

Hmm
Careful readers may notice that I have a tendency to like a day’s work on that day, and a lesser but still significant tendency to be unsatisfied later. I wonder if there is a bit of sunk-cost thinking affecting me, or maybe it’s just that when I’m familiar with the code it bothers me less, and when I’ve lost familiarity I see it in a new, possibly unflattering light.

Of course I really enjoy trying to make code more clear, so it could just be practice.

Be that as it may, this object isn’t pleasing me now, because what it does is simple and how it does it, not so much.

We’re stuck with the overall structure of collecting information during interactions and then dealing with the resulting understanding later. But does it have to be this intricate?

In the current design, as events happen independently, the object makes decisions about what it’s going to do later. It sees a Player, decides to do nothing later. It sees a ReservePlayer and decides that later, if need be, it can create a new InvaderPlayer for the user. If it doesn’t see a ReservePlayer (the default assumption), it is predisposed to call a GameOver and start a RobotPlayer.

The object is creating a sort of plan and then executing the plan. The plan itself is represented in those two “pluggable” variables, which contain method pointers. Very cool. Not very clear.

Let’s see if we can do better … and let’s try to do it in small steps in which we can be confident.

Make the End the Same

There are those two complex outcomes, where we actually do something. Let’s make them look more similar. Again, they start like this:

    def reserve_absent_game_over(self, fleets):
        fleets.remove(self)
        fleets.append(InvadersGameOver())
        robot = RobotPlayer()
        capsule = TimeCapsule(2.0, robot)
        fleets.append(capsule)
        maker = PlayerMaker()
        maker_capsule = TimeCapsule(2.1, maker)
        fleets.append(maker_capsule)

    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)

Let’s inline the code in the first one. I just want to see what it looks like. Let me show you all my steps.

Inline methods:

    def reserve_absent_game_over(self, fleets):
        fleets.remove(self)
        fleets.append(InvadersGameOver())
        robot = RobotPlayer()
        capsule = TimeCapsule(2.0, robot)
        fleets.append(capsule)
        maker = PlayerMaker()
        maker_capsule = TimeCapsule(2.1, maker)
        fleets.append(maker_capsule)

    def reserve_give_player_another_turn(self, fleets):
        fleets.remove(self)
        delay_until_new_player = 2.0
        a_bit_longer = 0.1
        player_capsule = TimeCapsule(delay_until_new_player, InvaderPlayer(), self.reserve)
        fleets.append(player_capsule)
        delay = delay_until_new_player + a_bit_longer
        maker_capsule = TimeCapsule(delay, PlayerMaker())
        fleets.append(maker_capsule)

Let’s make these more the same. I like the use of the delay temps, which explain what’s going on. First I want to arrange them better in the method that has them:

    def reserve_give_player_another_turn(self, fleets):
        delay_until_new_player = 2.0
        a_bit_longer = 0.1
        delay = delay_until_new_player + a_bit_longer
        fleets.remove(self)
        player_capsule = TimeCapsule(delay_until_new_player, InvaderPlayer(), self.reserve)
        fleets.append(player_capsule)
        maker_capsule = TimeCapsule(delay, PlayerMaker())
        fleets.append(maker_capsule)

Just a few quick “move line up” strokes. Now let’s give delay a better name:

    def reserve_give_player_another_turn(self, fleets):
        delay_until_new_player = 2.0
        a_bit_longer = 0.1
        delay_until_new_maker = delay_until_new_player + a_bit_longer
        fleets.remove(self)
        player_capsule = TimeCapsule(delay_until_new_player, InvaderPlayer(), self.reserve)
        fleets.append(player_capsule)
        maker_capsule = TimeCapsule(delay_until_new_maker, PlayerMaker())
        fleets.append(maker_capsule)

Now make the other method look the same:

    def reserve_absent_game_over(self, fleets):
        delay_until_new_player = 2.0
        a_bit_longer = 0.1
        delay_until_new_player = delay_until_new_player + a_bit_longer
        fleets.remove(self)
        fleets.append(InvadersGameOver())
        robot = RobotPlayer()
        capsule = TimeCapsule(delay_until_new_player, robot)
        fleets.append(capsule)
        maker = PlayerMaker()
        maker_capsule = TimeCapsule(delay_until_new_player, maker)
        fleets.append(maker_capsule)

    def reserve_give_player_another_turn(self, fleets):
        delay_until_new_player = 2.0
        a_bit_longer = 0.1
        delay_until_new_maker = delay_until_new_player + a_bit_longer
        fleets.remove(self)
        player_capsule = TimeCapsule(delay_until_new_player, InvaderPlayer(), self.reserve)
        fleets.append(player_capsule)
        maker_capsule = TimeCapsule(delay_until_new_maker, PlayerMaker())
        fleets.append(maker_capsule)

Extract the inline creations in the second method.

    def reserve_absent_game_over(self, fleets):
        delay_until_new_player = 2.0
        a_bit_longer = 0.1
        delay_until_new_player = delay_until_new_player + a_bit_longer
        fleets.remove(self)
        fleets.append(InvadersGameOver())
        robot = RobotPlayer()
        capsule = TimeCapsule(delay_until_new_player, robot)
        fleets.append(capsule)
        maker = PlayerMaker()
        maker_capsule = TimeCapsule(delay_until_new_player, maker)
        fleets.append(maker_capsule)

    def reserve_give_player_another_turn(self, fleets):
        delay_until_new_player = 2.0
        a_bit_longer = 0.1
        delay_until_new_maker = delay_until_new_player + a_bit_longer
        fleets.remove(self)
        new_player = InvaderPlayer()
        player_capsule = TimeCapsule(delay_until_new_player, new_player, self.reserve)
        fleets.append(player_capsule)
        new_maker = PlayerMaker()
        maker_capsule = TimeCapsule(delay_until_new_maker, new_maker)
        fleets.append(maker_capsule)

Rename robot in the first to new_player:

    def reserve_absent_game_over(self, fleets):
        delay_until_new_player = 2.0
        a_bit_longer = 0.1
        delay_until_new_player = delay_until_new_player + a_bit_longer
        fleets.remove(self)
        fleets.append(InvadersGameOver())
        new_player = RobotPlayer()
        capsule = TimeCapsule(delay_until_new_player, new_player)
        fleets.append(capsule)
        maker = PlayerMaker()
        maker_capsule = TimeCapsule(delay_until_new_player, maker)
        fleets.append(maker_capsule)

    def reserve_give_player_another_turn(self, fleets):
        delay_until_new_player = 2.0
        a_bit_longer = 0.1
        delay_until_new_maker = delay_until_new_player + a_bit_longer
        fleets.remove(self)
        new_player = InvaderPlayer()
        player_capsule = TimeCapsule(delay_until_new_player, new_player, self.reserve)
        fleets.append(player_capsule)
        new_maker = PlayerMaker()
        maker_capsule = TimeCapsule(delay_until_new_maker, new_maker)
        fleets.append(maker_capsule)

Move the differences to the top. They are the creation of the game over and the different kind of player.

    def reserve_absent_game_over(self, fleets):
        fleets.append(InvadersGameOver())
        new_player = RobotPlayer()
        delay_until_new_player = 2.0
        a_bit_longer = 0.1
        delay_until_new_player = delay_until_new_player + a_bit_longer
        fleets.remove(self)
        player_capsule = TimeCapsule(delay_until_new_player, new_player)
        fleets.append(player_capsule)
        maker = PlayerMaker()
        maker_capsule = TimeCapsule(delay_until_new_player, maker)
        fleets.append(maker_capsule)

    def reserve_give_player_another_turn(self, fleets):
        new_player = InvaderPlayer()
        delay_until_new_player = 2.0
        a_bit_longer = 0.1
        delay_until_new_maker = delay_until_new_player + a_bit_longer
        fleets.remove(self)
        player_capsule = TimeCapsule(delay_until_new_player, new_player, self.reserve)
        fleets.append(player_capsule)
        new_maker = PlayerMaker()
        maker_capsule = TimeCapsule(delay_until_new_maker, new_maker)

(I also renamed capsule to player_capsule)

I spin my wheels a bit wondering why PyCharm isn’t highlighting the duplication for me. I think they changed something. I’m probably supposed to turn on the AI assistant. Maybe later.

Anyway, it sure seems to me like we can extract a method here. Oh! I messed up a rename. Should have been delay_until_new_maker. The correct code is this:

    def reserve_absent_game_over(self, fleets):
        fleets.append(InvadersGameOver())
        new_player = RobotPlayer()
        delay_until_new_player = 2.0
        a_bit_longer = 0.1
        delay_until_new_maker = delay_until_new_player + a_bit_longer
        fleets.remove(self)
        player_capsule = TimeCapsule(delay_until_new_player, new_player)
        fleets.append(player_capsule)
        maker = PlayerMaker()
        maker_capsule = TimeCapsule(delay_until_new_maker, maker)
        fleets.append(maker_capsule)

    def reserve_give_player_another_turn(self, fleets):
        new_player = InvaderPlayer()
        delay_until_new_player = 2.0
        a_bit_longer = 0.1
        delay_until_new_maker = delay_until_new_player + a_bit_longer
        fleets.remove(self)
        player_capsule = TimeCapsule(delay_until_new_player, new_player, self.reserve)
        fleets.append(player_capsule)
        new_maker = PlayerMaker()
        maker_capsule = TimeCapsule(delay_until_new_maker, new_maker)
        fleets.append(maker_capsule)

Extract the method. PyCharm does not detect the other possibility. I finally notice the difference in the second one’s time capsule.

Undo, make the code more alike.

    def reserve_absent_game_over(self, fleets):
        fleets.append(InvadersGameOver())
        new_player = RobotPlayer()
        reserve_to_remove = None
        delay_until_new_player = 2.0
        a_bit_longer = 0.1
        delay_until_new_maker = delay_until_new_player + a_bit_longer
        fleets.remove(self)
        player_capsule = TimeCapsule(delay_until_new_player, new_player, reserve_to_remove)
        fleets.append(player_capsule)
        maker = PlayerMaker()
        maker_capsule = TimeCapsule(delay_until_new_maker, maker)
        fleets.append(maker_capsule)

    def reserve_give_player_another_turn(self, fleets):
        new_player = InvaderPlayer()
        reserve_to_remove = self.reserve
        delay_until_new_player = 2.0
        a_bit_longer = 0.1
        delay_until_new_maker = delay_until_new_player + a_bit_longer
        fleets.remove(self)
        player_capsule = TimeCapsule(delay_until_new_player, new_player, reserve_to_remove)
        fleets.append(player_capsule)
        new_maker = PlayerMaker()
        maker_capsule = TimeCapsule(delay_until_new_maker, new_maker)
        fleets.append(maker_capsule)

Ha! Now PyCharm notices 9 lines of duplication. My bad. Sorry, JetBrains, for doubting you.

Extract Method:

    def reserve_absent_game_over(self, fleets):
        fleets.append(InvadersGameOver())
        new_player = RobotPlayer()
        reserve_to_remove = None
        self.set_up_next_player(new_player, reserve_to_remove, fleets)

    def reserve_give_player_another_turn(self, fleets):
        new_player = InvaderPlayer()
        reserve_to_remove = self.reserve
        self.set_up_next_player(new_player, reserve_to_remove, fleets)

    def set_up_next_player(self, new_player, reserve_to_remove, fleets):
        delay_until_new_player = 2.0
        a_bit_longer = 0.1
        delay_until_new_maker = delay_until_new_player + a_bit_longer
        fleets.remove(self)
        player_capsule = TimeCapsule(delay_until_new_player, new_player, reserve_to_remove)
        fleets.append(player_capsule)
        maker = PlayerMaker()
        maker_capsule = TimeCapsule(delay_until_new_maker, maker)
        fleets.append(maker_capsule)

PyCharm did not suggest applying the extract in the other case. I really wonder why. It observed the duplication and generally when it sees a pattern like that, it offers to apply it. Well, no harm done, just had to do it myself, which was easy enough, but I’d rather PyCharm did it: it makes fewer mistakes than I do.

We are green and good. The plan was to see what would happen if we made those alike. I like the result. Commit: refactor out common elements in final player setup.

Let’s break here, that’s a lot of lines for you to read. I wrote this straight through at least the next article, in three hours.

Interim Summary

At this point the code is very slightly improved in that we have identified and emphasized some duplication and then removed it. I just want to give you a chance to rest.

See you in the next article!