Python Asteroids on GitHub

Let’s review how we added a two-player mode to the game. Let’s look at how it was done. Frankly, I am pleased. EXCEPT: A Defect!!

The two-player game was a surprise to me: I hadn’t intended to do it. If I’ve ever done it before, I’ve forgotten. Certainly there was nothing built into the system to support it. My “theory” about the decentralized design we have now, with all the little “Flyer” objects, is that changes to the game would be simple, localized, and involve adding new objects to the mix.

Overall Plan
I did a whole couple of hours of planning, written up in Python 170, where I considered a few possibilities, converging on the notion of having two ScoreKeeper instances, one for each player, a Signal that would be put into the mix to turn one keeper on and the other off, and extending ShipMaker to have two ship counts, one for each player, and to alternate between them using Signal.

Oddly enough, that’s pretty much what happened.

ScoreKeeper
The two player game needs to display both players’ scores all the time, one on the left side of the screen, one on the right. The active player’s score needs to be indicated somehow.

We spiked moving the ScoreKeeper display to the other side of the screen, with some visual tuning to make it look right. We reversed the display of free ships to flow from right to left when the score is on the right, so that they’d extend outward toward the center, based on a new member variable, _player_number, zero or one.

We tested the visuals by adding an additional ScoreKeeper to the mix, using a local version of coin that was patched and not committed. Arguably this was a mistake: we could have created the two-player coin right then and committed its new versions as we went along. I think that I didn’t do that because my focus was on the game objects as they evolved, not on the final combination of objects for two-player mode.

Signal
Given that there would be two ScoreKeeper instances in the mix, which was now pretty clear, the Signal we needed was just a trivial object containing a single member variable, which would be zero or one depending on which ScoreKeeper should be active. Its member is called signal but perhaps should be active_player or even player_number. And the Signal should perhaps be renamed to show its relationship with ScoreKeeper. send interact_with_signal, as required, it doesn’t interact with any other Flyer, and on tick it removes itself, so it only exists for one cycle.
ShipMaker
We had to do a bit of refactoring with ShipMaker, because it had one member variable, ships_remaining that was referenced in about 14 places, mostly in tests, but also by ScoreKeeper, which now clearly needed to pass in its player number to get the right count. I think now that it would have been possible to keep the old property, perhaps renaming it to current_ships_remaining, with fewer changes to existing code. But it seemed better at the time to have all accesses to that information provide a player number, so in a series of simple refactoring steps, we did that.

Then we plugged in a table of two ship counts, and with a simple change to one method, got ShipMaker alternating from player zero to player one and back, sending the proper signal to alternate the score keeping. There is a small fake ShipMaker object, NoShips, used in testing, and that needed to have a couple of similar changes to keep those tests running.

At this point, we could see that if the two players ship counts were equal, all was well, but if one player were to gain extra ships, things would go awry. We proved this by patching in a different number of ships for the two players, and sure enough, the game ended too soon.

The implementation to handle that was clear enough: check both counts before ending the game, and if the player you’re trying to hand off to has no ships, the other player does, so run that one. We did a couple of Extract Method refactorings, added the new check, and it worked first time. We’ll review that code below, because it could use a little improvement.

Tests
I had been relying on the existing ShipMaker tests to catch any serious mistakes, but I did write a new test that checks that a player with extra ships gets all her turns before the game ends. We’ll discuss testing further, below.
ScoreKeeper
At this point, everything was working correctly, but there was no on-screen indication as to which player and score were active. So we made a quick change to ScoreKeeper.draw, to set the color of the non-scoring one to a pale gray, leaving the active one bright green.
Tests
Much of the testing was visual, to see what the scores looked like, and then to watch the ShipMaker alternate ships between the two players. Existing tests did ensure that I gave the new object, Signal, the necessary abstract methods.

However, once my ScoreKeeper experimenting was done, I wrote a test to check ScoreKeeper’s interaction with Signal. That checked that the necessary interact_with_signal was being sent, received, and processed correctly.

Coin
We implemented a new coin, “2”, which emits two ScoreKeeper instances rather than just one, and initializes ShipMaker with a table size of 2, so that it’ll operate in two player mode.
Ship It
And we shipped it.
Testing
I did write a few new tests aimed at the new features, but I freely grant that I was more in a code then test mode than test then code. This is well known to be one of my failings. I have two someone conflicting feelings about the state of testing for two player mode. On the one hand, I didn’t write many tests, so I feel a bit queasy about the situation. But on the other hand, I did test the key changes, and the old tests exercise most of the code as well and I can’t think of any tests that I’d like to add.

Perhaps I’d feel better had I been more test-driven.

But I wonder. The decentralized model generally requires a new object to do a new thing, and this particular feature required one new object, Signal, and changes to two object, ScoreKeeper and ShipMaker, and the two existing objects communicate using the new one.

The decentralized model doesn’t actually do anything until we have two or more objects ready to interact. The tests themselves tend to be a bit “high level”, typically requiring setting up a Fleets instance and putting a few objects in there to interact, and then inspecting the state of Fleets to be sure that the objects did what we intended.

Is this model inherently harder to test? It might well be, because it relies on two or more objects interacting, where a simpler design might allow us to exercise each object independently. In a more conventional model, the ShipMaker might have access to the ScoreKeepers, or vice versa and they could just send each other messages to make things happen.

Here, the ShipMaker doesn’t know there are ScoreKeepers. It just knows that it has to send a Signal when it changes players. And ScoreKeepers just know to listen for Signals and set themselves active or passive. All the critical game behavior is in the cooperation of objects, which know little or nothing about the other objects in the Game.

This seems to make testing more difficult. It might be easier with a more Mockist (London)approach than my usual Classical (Detroit) style. See Martin Fowler’s Mocks aren’t Stubs.

Overall, I’d prefer to have tested sooner, and possibly more, and yet I am quite confident in both the code and tests. I am in a good place.

One needed change

We create new ships with this logic:

    def create_ship(self, fleets):
        if not self._safe_to_emerge:
            return False
        if self.ships_remain():
            self.rez_available_ship(fleets)
        else:
            fleets.append(GameOver())
            self._game_over = True
        return True

    def ships_remain(self):
        return sum(self._ships_remaining) > 0

    def rez_available_ship(self, fleets):
        if self.ships_remaining(self._next_player) > 0:
            self._ships_remaining[self._next_player] -= 1
            fleets.append(Ship(u.CENTER))
            fleets.append(Signal(self._next_player))
            self._next_player = (self._next_player + 1) % len(self._ships_remaining)
        else:
            self._next_player = (self._next_player + 1) % len(self._ships_remaining)
            self.rez_available_ship(fleets)

The rez_available_ship method is recursive! It got that way because I wrote it that way. When I got to the else, I saw that what I needed to do was to do the code that was in the first part of the if and that if I were to recur, it would execute properly.

That is true, but strange, especially when we could instead do this:

    def rez_available_ship(self, fleets):
        if self.ships_remaining(self._next_player) == 0:
            self._next_player = (self._next_player + 1) % len(self._ships_remaining)
        self._ships_remaining[self._next_player] -= 1
        fleets.append(Ship(u.CENTER))
        fleets.append(Signal(self._next_player))
        self._next_player = (self._next_player + 1) % len(self._ships_remaining)

We enter this method knowing that someone has an available ship. Usually, both players have so we skip over the if and rez on behalf of _next_player. But if _next_player has run out of ships, the other player has not (because someone has ships), so we swap over to the other player and rez.

This passes all tests. Commit: refactor rez_available_ship.

Perhaps we can make it more clear. How about this:

    def rez_available_ship(self, fleets):
        if self.ships_remaining(self._next_player) == 0:
            self.switch_to_other_player()
        self._ships_remaining[self._next_player] -= 1
        fleets.append(Ship(u.CENTER))
        fleets.append(Signal(self._next_player))
        self.switch_to_other_player()

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

No, “switch” is too strong. It sounds like we’re going to do something active, not just set _next_player. Let’s extract another bit and see what we think:

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

    def rez_ship_for_player(self, player_number, fleets):
        self._ships_remaining[player_number] -= 1
        fleets.append(Ship(u.CENTER))
        fleets.append(Signal(player_number))

I like that better than before the second extraction. Still not sure about switch_to_other_player. What would be better? Is _next_player the wrong name? Should it be _current_player?

I don’t know. I think this is improved. Commit: extract method rez_ship_for_player.

Quit while you’re ahead, that’s my motto for just now.

Other Improvements?

Thinking about what my pals may say, the use of a simple 0/1 integer to signify player number may trouble them. They may think there should be some kind of enum, or something of that kind. Maybe the ships remaining info should be in a keyed dictionary or something rather than a simple list (Keyed by integer, by the way. A key is a key is a key?)

We could imagine something other than a list for that info, a smarter object that could somehow encapsulate the information about

Summary

The change to provide two-player mode went well:

  • The implementation was consistent with my theory of the decentralized approach, which is that generally,, we’ll add a new object, existing objects will be modified to interact with it (and/or vice versa) and we’ll be done. That’s pretty much what happened.

  • We did a series of about a dozen small steps, each one leaving the game working and shippable. All the operational code was released all the time, except for coin, which I was patching to do my work. That may not have been ideal: possibly implementing “2” and not documenting it would have been better.

  • Existing tests bore a reasonable amount of the weight, needing a bit of changing to adjust to the ShipMaker table, which was to be expected. It was probably possible to avoid most of those changes, but exercising the new table was, I think, better. (That is after-the-fact reasoning. In the moment, I just changed them.)

  • Initial design thinking, no more than a couple of hours of thinking and writing, led to a plan that held water. I don’t credit that so much to my own intelligence as to the fact that the objects available are reasonably well designed with well-separated functionality. (Of course, I do get credit for that …)

In short: the surprise request for two-player mode took just about a half day of actual work, and involved small changes to two existing objects and creation of a third quite trivial object. And however you cut it, that is a very good outcome!

See you next time!

Oh No! There’s a defect!