Python Asteroids+Invaders on GitHub

Continued: 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.

This article continues the previous one, still trying to improve PlayerMaker. Here’s the current code:

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

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

Still Not Happy

We do see more clearly what happens, I think, but it seems to me that it could be far more clear. Let me see if I can express it better in something like a table.

Case: Player Exists
Add: nothing
Remove: nothing
Case: No Player, Reserve Exists
Add: new human player (2 seconds)
Add: new player maker (2.1 seconds)
Remove: self
remove: reserve (2 seconds)
Case: No Player, No Reserve
Add: new robot player (2 seconds)
Add: new player maker (2.1 seconds)
Add: GameOver
remove: self

Why can’t the code say that?

Observation
Note that I ask the question but do not come up with an answer today. Perhaps later?

I’m thinking about having a table that might say to add nothing. Can our code handle adding None? Let’s glance at TimeCapsule for a moment.

class TimeCapsule(InvadersFlyer):
    def __init__(self, time, added_flyer, removed_flyer=None):
        self.to_add = added_flyer
        self.to_remove = removed_flyer
        self.time = time

    def tick(self, delta_time, fleets):
        self.time -= delta_time
        if self.time <= 0:
            fleets.remove(self)
            fleets.append(self.to_add)
            if self.to_remove:
                fleets.remove(self.to_remove)

We check to be sure we don’t remove a None. Why don’t we check for adding a None? What does Fleets do with None?

Note
We’ll find out below that Fleets doesn’t care if you remove None, but will go ahead and add a None. We fix that. We could come back and remove the check for to_remove here, but we do not. Can’t catch them all.
class Fleets:
    def append(self, flyer):
        self.flyers.append(flyer)

    def remove(self, flyer):
        # more pythonic?
        try:
            self.flyers.remove(flyer)
        except ValueError:
            pass

I want to know more about this.

    def test_append_none(self):
        fleets = Fleets()
        fleets.append(None)
        assert not fleets.all_objects

Right. all_objects becomes a collection containing a None, because None is a legitimate if not very interesting object. We’ll fix that.

Test remove while we’re at it.

    def test_remove_none(self):
        fleets = Fleets()
        fleets.remove(None)
        assert not fleets.all_objects

That works fine, to no one’s surprise. I want Fleets to ignore None.: it’s not a legitimate Flyer. This is speculation but I’ll accept that.

    def append(self, flyer):
        if flyer:
            self.flyers.append(flyer)

Tests pass. First update to the framework? Might be. Commit: Fleets ignores attempt to add a None.

Now about the table idea.

Note
I keep thinking about a table and doing something else. The code wants to be improved, and does not yet want to be tabularized. Neither the code nor I even know what “tabularized” means.

I think I know where I want to go. I’ll hold on to the idea loosely, but I will hold on to it. I want to change our PlayerMaker so that it sets up some tables containing the things to be added and removed. Then it deals with interactions, recording results. Then based on what it sees, it adds and removes the things.

No, that won’t do. We don’t want to create all those time capsules and player makers and whatnot instances on every cycle.

I’m going to make some changes, informal refactoring. I hope to keep it, am willing to roll back, depending on what happens. Let’s review the playermaker tests:

    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

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

    def test_rezzes_time_capsule(self):
        maker = PlayerMaker()
        maker.begin_interactions(None)
        maker.interact_with_reserveplayer(ReservePlayer(0), None)
        maker.end_interactions(fleets := FakeFleets())
        assert fleets.appends
        capsule = fleets.appends[0]
        assert isinstance(capsule.to_add, InvaderPlayer)
        assert isinstance(capsule.to_remove, ReservePlayer)

    def test_game_over(self):
        maker = PlayerMaker()
        maker.begin_interactions(None)
        maker.end_interactions(fleets := Fleets())
        assert fleets.flyers
        assert any(isinstance(flyer, InvadersGameOver) for flyer in fleets.flyers)

I want to have a flag, saying whether I have seen a player. Let’s just save the player.: it’s a perfectly good flag.

    def test_notices_player(self):
        maker = PlayerMaker()
        maker.begin_interactions(None)
        assert maker.player_found == None
        final = maker.pluggable_final_action
        invader_player = InvaderPlayer()
        maker.interact_with_invaderplayer(invader_player, None)
        assert maker.player_found == invader_player
        assert maker.pluggable_final_action is not final

Fails for want of player_found. Fix:

class PlayerMaker...
    def begin_interactions(self, _fleets):
        self.reserve = ReservePlayer(-999)
        self.player_found = None
        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.player_found = player
        self.pluggable_final_action = self.final_do_nothing

Same test for RobotPlayer:

    def test_notices_robot_player(self):
        maker = PlayerMaker()
        maker.begin_interactions(None)
        assert maker.player_found is None
        robot_player = RobotPlayer()
        maker.interact_with_robotplayer(robot_player, None)
        assert maker.player_found == robot_player

I don’t check the final action because I am planning to replace it.

This fails as planned. Fix:

    def interact_with_robotplayer(self, robot, fleets):
        self.player_found = robot
        self.pluggable_reserve_action = self.final_do_nothing
        self.pluggable_final_action = self.final_do_nothing

Green. Shall we commit? Sure, why not. Commit: PlayerMaker holds on to InvaderPlayer or RobotPlayer if seen.

Note
We don’t use the fact that our player_found is a player or robot. A boolean would work as well. Didn’t see that. Maybe next time.

Take a Break?

This might be a good time to take a break and come back later for the thrilling not quite a conclusion. I’ve decided not to split the article again.

Now I want to refactor end_interactions, which is now:

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

I change it to this:

    def end_interactions(self, fleets):
        if not self.player_found:
            self.final_deal_with_missing_player(fleets)

This no longer refers to the final pluggable actions If there is a player or robot, nothing happens, otherwise we deal with a missing player. Remove the pluggable, expecting some tests to fail. One does, needing the changes shown as lines commented out:

    def test_notices_player(self):
        maker = PlayerMaker()
        maker.begin_interactions(None)
        assert maker.player_found is None
        # final = maker.pluggable_final_action
        invader_player = InvaderPlayer()
        maker.interact_with_invaderplayer(invader_player, None)
        assert maker.player_found == invader_player
        # assert maker.pluggable_final_action is not final

I am not sure that I have enough tests. I’m sure we’re good for now tho. Commit: refactoring to remove pluggables.

Now the other pluggable has two modes. In the final_deal_with_missing_player, we just do this:

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

That leads to one of two places:

    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)

Now we always have a reserve, because we start with number -999, so that our sorting logic works:

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

Let me just change final_deal_with_missing_player to what it does without all the plugging stuff. It just runs one of two methods.

Was:

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

Becomes:

    def final_deal_with_missing_player(self, fleets):
        if self.reserve.reserve_number < 0:
            self.reserve_absent_game_over(fleets)
        else:
            self.reserve_give_player_another_turn(fleets)

Tests are all green. I am semi-confident. Test in game. Works perfectly. Commit: no longer using pluggables.

Now I have cleaning to do.

class PlayerMaker(InvadersFlyer):
    def __init__(self):
        self.reserve = ReservePlayer(-999)
        self.player_found = None

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

    def begin_interactions(self, _fleets):
        self.reserve = ReservePlayer(-999)
        self.player_found = None

    def interact_with_invaderplayer(self, player, _fleets):
        self.player_found = player

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

    def interact_with_robotplayer(self, robot, fleets):
        self.player_found = robot

    def end_interactions(self, fleets):
        if not self.player_found:
            self.deal_with_missing_player(fleets)

    def deal_with_missing_player(self, fleets):
        if self.reserve.reserve_number < 0:
            self.no_reserve_game_over(fleets)
        else:
            self.reserve_give_player_another_turn(fleets)

    def no_reserve_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)

One more thing, maybe two. That code about reserve_number isn’t as clear as it might be. Let’s have ReservePlayer help us. We’ll give it the ability to create an “invalid” ReservePlayer and to answer whether a ReservePlayer is valid.

class ReservePlayer(Spritely, InvadersFlyer):
    @classmethod
    def invalid(cls):
        return cls(-666)

    def __init__(self, reserve_number=0):
        self._sprite = Sprite.player()
        x = u.INVADER_PLAYER_LEFT + reserve_number*(5*self.rect.width//4)
        self.position = Vector2(x, u.RESERVE_PLAYER_Y)
        self.reserve_number = reserve_number

    @property
    def is_valid(self):
        return self.reserve_number >= 0

And in the PlayerMaker:

class PlayerMaker(InvadersFlyer):
    def __init__(self):
        self.reserve = ReservePlayer.invalid()
        self.player_found = None

    def begin_interactions(self, _fleets):
        self.reserve = ReservePlayer.invalid()
        self.player_found = None

    def deal_with_missing_player(self, fleets):
        if not self.reserve.is_valid:
            self.no_reserve_game_over(fleets)
        else:
            self.reserve_give_player_another_turn(fleets)

Invert that if, get rid of the not.

    def deal_with_missing_player(self, fleets):
        if self.reserve.is_valid:
            self.reserve_give_player_another_turn(fleets)
        else:
            self.no_reserve_game_over(fleets)

And I think we’re done here. Let’s take one more look and sum up:

class PlayerMaker(InvadersFlyer):
    def __init__(self):
        self.reserve = ReservePlayer.invalid()
        self.player_found = None

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

    def begin_interactions(self, _fleets):
        self.reserve = ReservePlayer.invalid()
        self.player_found = None

    def interact_with_invaderplayer(self, player, _fleets):
        self.player_found = player

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

    def interact_with_robotplayer(self, robot, fleets):
        self.player_found = robot

    def end_interactions(self, fleets):
        if not self.player_found:
            self.deal_with_missing_player(fleets)

    def deal_with_missing_player(self, fleets):
        if self.reserve.is_valid:
            self.reserve_give_player_another_turn(fleets)
        else:
            self.no_reserve_game_over(fleets)

    def no_reserve_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)

Still don’t have that nice tabular look. I’d like to have that. But we’ve removed the pluggable stuff and I think the if statements are pretty clear. What about inlining that deal_with and flattening it?

Surprisingly PyCharm screws it up!

    def end_interactions(self, fleets):
        if not self.player_found:
            if self.reserve.is_valid:
        self.reserve_give_player_another_turn(fleets)

    else:
    self.no_reserve_game_over(fleets)

Nasty. I’ll report it. Undo, do it by hand.

    def end_interactions(self, fleets):
        if self.player_found:
            pass
        elif self.reserve.is_valid:
            self.reserve_give_player_another_turn(fleets)
        else:
            self.no_reserve_game_over(fleets)

Is that better? I’m not convinced. Put it back.

    def end_interactions(self, fleets):
        if not self.player_found:
            self.deal_with_missing_player(fleets)

    def deal_with_missing_player(self, fleets):
        if self.reserve.is_valid:
            self.reserve_give_player_another_turn(fleets)
        else:
            self.no_reserve_game_over(fleets)

Summary

I promised to hold on to the idea of the tables, and I did. I just haven’t done it: the code changes that I discovered have not taken me there yet. They have, however, in my current opinion (see related remarks), led to a more clear implementation of the PlayerMaker. We’ve reduced the code from almost 70 lines to 51, and removed two rather mysterious pluggable method variables.

I do think that I’d like to have something that presents the three results more explicitly. That’s not for today. We have improvement, that’s what we look for.

Also, reviewing the notes I made in finalizing these two articles, I see that I’ve left some little tag ends that could be better. Stuff happens: we can’t remember every idea we get, nor can we write fast enough to write them all down. The things missed are harmless: it’s just that things could be improved. But things can always be improved. What matters is that they are improving … and this is better.

As feelings go, I felt confident throughout, and survived a few interruptions as well. A good session.

See you next time!