Python Asteroids on GitHub

After all that happiness, I realized that free ships go to the wrong player in the two-player mode. Bummer. In fact: Dammit!

There is a Defect!!!

Free ships go to the wrong player! I just thought about that, tested it, and sure enough it’s true.

class ShipMaker(Flyer):
    def add_ship(self):
        self._ships_remaining[self._next_player] += 1
        player.play("extra_ship")

Dammit! I was doing so well, too. Tempted to just fix it and never tell you. But that is not how we do it.

We do have a test for free ships but it operates in one player mode:

    def test_free_ship_every_N_points(self):
        fleets, maker, keeper = self.set_up_free_ship_test()
        free = u.FREE_SHIP_SCORE
        keeper.interact_with_shipmaker(maker, fleets)
        assert keeper.score == 0
        keeper.interact_with_score(Score(100), fleets)
        assert maker.ships_remaining(0) == 0
        assert keeper.score == 100
        keeper.interact_with_score(Score(free), fleets)
        assert keeper.score == 100 + free
        assert maker.ships_remaining(0) == 1

    @staticmethod
    def set_up_free_ship_test():
        fleets = Fleets()
        fleets.append(keeper := ScoreKeeper())
        fleets.append(maker := ShipMaker(1))
        maker.testing_set_ships_remaining(0)
        return fleets, maker, keeper

We need another test:

    def test_two_player_free_ship_goes_to_right_ship(self):
        fleets = Fleets()
        fleets.append(keeper := ScoreKeeper())
        fleets.append(maker := ShipMaker(2))
        assert maker._next_player == 0
        maker.rez_available_ship(fleets)
        assert maker._next_player == 1
        maker.testing_set_ships_remaining(0)
        keeper.interact_with_signal(Signal(0), fleets)
        free = u.FREE_SHIP_SCORE
        keeper.interact_with_shipmaker(maker, fleets)
        assert keeper.score == 0
        keeper.interact_with_score(Score(100), fleets)
        assert maker.ships_remaining(0) == 0
        assert keeper.score == 100
        keeper.interact_with_score(Score(free), fleets)
        assert keeper.score == 100 + free
        assert maker.ships_remaining(0) == 1
        assert maker.ships_remaining(1) == u.SHIPS_PER_QUARTER

This is hard to understand, I think, but does the job, that is, it fails.

This fails, as expected, on the penultimate check, because we gave the free ship to player one not player zero. Fix the bug. I try the following, not for cuteness so much as because it should work and requires no thinking. I know the setting is wrong for adding so I swap it, add, and swap it back. Hackery but we need a quick fix.

    def add_ship(self):
        self.switch_to_other_player()
        self._ships_remaining[self._next_player] += 1
        self.switch_to_other_player()
        player.play("extra_ship")

That passes, so I am confident that we’re in the right place. Now let’s do something reasonable, expressing our actual intention:

    def add_ship(self):
        self._ships_remaining[self._current_player] += 1
        player.play("extra_ship")

I make _current_player a property, thus:

    @property
    def _current_player(self):
        return (self._next_player + 1) % len(self._ships_remaining)

That does pass. We have a defect and it is fixed now, so commit: fix defect where free ship went to wrong player.

Think about causes

I think that a deepish cause of this problem is the use of _next_player, which is at best weird.

Let’s see if we could make things more clear in rez to the benefit of all. What we’d like to have happen is for the game to hold _current_player as correct, aligned with whatever ScoreKeeper is doing. The thing is, we may have one player or we may have two. We always want to start with current player zero. We’d like to have just the one variable for current, plus the list.

Weird idea
Doesn’t pan out.

Wait. What if we always ran from the zeroth element of the list, and we reversed the list right before we rez? (Would we have to start the list reversed? I think so. Or pull from the end?)

Let’s try to make the list idea work. Nope!

I could show you my work, but it breaks too many tests and we’ll just roll it back.

We need to keep the ships remaining list in order: other objects are counting on it.

Try again, fail again
This time I have the right idea but the changes get away from me somehow. I recognize that and roll back again.

Instead of that weird implementation of current_player, what if we were to compute it directly? I have more success with that, but not quite as much as I’d like so let’s do it one more time:

Third time charm?
This time it works nicely.

Remove this:

    @property
    def _current_player(self):
        return (self._next_player + 1) % len(self._ships_remaining)

Replace with this:

    def __init__(self, number_of_players):
        self._ships_remaining = []
        for _ in range(0, number_of_players):
            self._ships_remaining.append(u.SHIPS_PER_QUARTER)
        self._next_player = 0
        self._current_player = 0
        self._timer = Timer(u.SHIP_EMERGENCE_TIME)
        self._game_over = False
        self._need_ship = True
        self._safe_to_emerge = False

    def switch_to_other_player(self):
        self._current_player = self._next_player
        self._next_player = (self._next_player + 1) % len(self._ships_remaining)

    def rez_available_ship(self, fleets):
        if self.ships_remaining(self._next_player) == 0:
            self.switch_to_other_player()
        self.switch_to_other_player()
        self.rez_ship_for_player(self._current_player, fleets)

This is sufficient to get us to green. Commit: keep track of current player and next player.

Now can we make it better?
Three tries got it working. Now, I would like to avoid having both _currrent_player and _next_player members.

Who’s looking at next? Try this:

class ShipMaker(Flyer):

    def __init__(self, number_of_players):
        self._ships_remaining = []
        for _ in range(0, number_of_players):
            self._ships_remaining.append(u.SHIPS_PER_QUARTER)
        self._current_player = number_of_players - 1  # trust me
        self._timer = Timer(u.SHIP_EMERGENCE_TIME)
        self._game_over = False
        self._need_ship = True
        self._safe_to_emerge = False

    def rez_available_ship(self, fleets):
        self.switch_to_other_player()
        if self.ships_remaining(self._current_player) == 0:
            self.switch_to_other_player()
        self.rez_ship_for_player(self._current_player, fleets)

    def switch_to_other_player(self):
        self._current_player = (self._current_player + 1) % len(self._ships_remaining)

There were some references to next in test, but they weren’t really germane to the tests in question, just there for debug purposes. I just pull them.

The only weird bitnow is the init for _current_player. In the case of the two player game, we have to make it appear that current player is player one, so that we’ll switch to player zero and start everything off right.

We’re green. Commit: remove _next_player member.

Summary

Well, dammit. A defect in our new feature. I would have said that free ships had been adequately checked, if you were to ask me, but I was mistaken. It was easy enough to write the new test, easy enough to make it pass. And subsequent refactoring has made the code better.

But embarrassing nonetheless. Everything was going so well.

Truth is, the objects still went in easily and I think we can see the good aspects of the decentralized scheme.

And in my defense, I never score free ships, I’m too terrible at the game. Why would I even think about the feature?

But the defect shows at least one not so good aspect of this design: if we do not check all the interactions, it might be too easy for things to go wrong. Of course, if we don’t check everything, things can always go wrong, and maybe, just maybe, we might have had a similar defect with any other scheme. The big mistake, of course, was not thinking of and writing the test. That’s on me.

Bad Ron, no biscuit. But on the other hand, good job finding and fixing the problem and good job fessing up, because I could have swept this right under the table. But that is not how we do it here. You get to see what really happens. Warts and all.

See you next time!