Python 175 - Better

Because I injected a defect into my two-player game, and because it could be better anyway, let’s make it better.

ShipMaker Responsibilities

Notice when there are no ships on screen;
Wait a discreet interval;
Wait until there is a safe space;
Create a ship for the current player;
Upon outside command, add a free ship for the current player;
Report how many ships a player has, for ScoreKeeper.

If I recall my many decades of study of this art, no one has ever said “An object should have six responsibilities”. I vaguely recall the notion that the number of responsibilities should be smaller. One.

Of course we could easily say “ShipMaker is responsible for, um, making Ships” and kind of slink out of the room, but when we have this much going on in an object, we should seriously consider whether it needs helpers.

Are there exceptions? Sure. Number classes can add, subtract, multiply and divide. They are responsible for “elementary arithmetic” or something like that. And for all I know, a good implementation of Integer might use helper classes. I’ve never tried to code up arithmetic.

Today, I plan to offload a few responsibilities of ShipMaker into a helper object. My plan is to implement an object (interface, really), perhaps called ShipProvider. Because I thought about this fairly extensively before getting up this morning, I am considering two implementors of ShipProvider, perhaps named SinglePlayerShipProvider and TwoPlayerShipProvider.

I think the protocol will include something like:

provide, which will return a list of objects to be added to the mix, typically a Ship and a corresponding Signal, but possibly a GameOver. The caller (ShipMaker) will just add whatever is provided. (We may prefer to have the provider do the adding. Remains to be seen.)
ships_remaining(player_identifier), which will return how many ships are still available for that player.
add_ship(player_identifier), which will add a ship. Since it is ScoreKeeper who sends that message when the score rolls over, it knows the player and should probably provide it. That would have avoided the defect we fixed yesterday.

I have been saying ‘player_identifier’ rather than player_number. I’m thinking that it might be better to use something other than an integer to identify the players. However, it seems to me that whatever we use comes down to an arbitrary agreed-upon value, whether integer, string, or UUID. Perhaps they should at least be named in the u constants.

I think that my plan will be to test-drive the development of the providers and then plug them in when I think they work.

Let’s begin by preparing the space. Since we can easily provide the player number on add ship, let’s do that. Presently we have:

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

class ScoreKeeper(Flyer):
    def interact_with_score(self, score, fleets):
        if self._scoring:
            self.score += score.score
            if self.score >= self._fence:
                self._ship_maker.add_ship()
                self._fence += u.FREE_SHIP_SCORE

I don’t think I need any new tests to change these:

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

class ScoreKeeper(Flyer):
    def interact_with_score(self, score, fleets):
        if self._scoring:
            self.score += score.score
            if self.score >= self._fence:
                self._ship_maker.add_ship(self._player_number)
                self._fence += u.FREE_SHIP_SCORE

Tests are green. Commit: ScoreKeeper provides player identifier to ShipMaker.add_ship.

With that protocol change in place, let’s TDD the Providers.

class TestShipProviders:
    def test_single_provider(self):
        fleets = Fleets()
        fi = FI(fleets)
        provider = SinglePlayerShipProvider(4)
        assert provider.ships_available(0) == 4

class SinglePlayerShipProvider:
    def __init__(self, number_of_ships):
        self._ships = number_of_ships

    def ships_available(self, _player):
        return self._ships

I got going so fast that I forgot to mention that I did that in two steps. Slow down. Hold your horses as my sainted daddy used to say. Commit: initial test and SinglePlayerShipProvider.

Now to test provide. I have decided to pass fleets in, which is why I showed it in the test.

Just to show you how TDD my way really goes:

class TestShipProviders:
    def test_single_provider(self):
        fleets = Fleets()
        fi = FI(fleets)
        provider = SinglePlayerShipProvider(4)
        assert provider.ships_available(0) == 4
        provider.provide(fleets)

PyCharm squiggles provide, which counts as a red bar. (And it auto-runs the tests, so I have a real red bar as well.) So I can let PyCharm provide provide, which it provides:

    def provide(self, fleets):
        pass

Test passes. Add assert:

class TestShipProviders:
    def test_single_provider(self):
        fleets = Fleets()
        fi = FI(fleets)
        provider = SinglePlayerShipProvider(4)
        assert provider.ships_available(0) == 4
        provider.provide(fleets)
        assert fi.ships

Test fails. Fix it:

    def provide(self, fleets):
        fleets.append(Ship(u.CENTER))

I got to put in the u.CENTER because PyCharm told me I needed a parameter. We should check it anyway, why not? Test is green.

class TestShipProviders:
    def test_single_provider(self):
        fleets = Fleets()
        fi = FI(fleets)
        provider = SinglePlayerShipProvider(4)
        assert provider.ships_available(0) == 4
        provider.provide(fleets)
        assert fi.ships
        assert fi.ships[0].position == u.CENTER

I suppose I could be committing this but since it’s not close to done, I don’t feel that would be quite the thing. I am probably wrong about that.

We should emit a Signal. Test for that.

class TestShipProviders:
    def test_single_provider(self):
        fleets = Fleets()
        fi = FI(fleets)
        provider = SinglePlayerShipProvider(4)
        assert provider.ships_available(0) == 4
        provider.provide(fleets)
        assert fi.ships
        assert fi.ships[0].position == u.CENTER
        assert fi.signals[0].signal == 0

Implement:

    def provide(self, fleets):
        fleets.append(Ship(u.CENTER))
        fleets.append(Signal(0))

Let’s deal with that player number right now.

    def provide(self, fleets):
        fleets.append(Ship(u.CENTER))
        fleets.append(Signal(u.PLAYER_ZERO))

u.py
PLAYER_ZERO = 0
PLAYER_ONE = 1

Commit u: u has PLAYER_ZERO and PLAYER_ONE defined.

Tests are green. We need to honor the count. I write out the whole story:

class TestShipProviders:
    def test_single_provider(self):
        fleets = Fleets()
        fi = FI(fleets)
        provider = SinglePlayerShipProvider(4)
        assert provider.ships_available(u.PLAYER_ZERO) == 4
        provider.provide(fleets)
        assert fi.ships
        assert fi.ships[0].position == u.CENTER
        assert fi.signals[0].signal == u.PLAYER_ZERO
        for i in range(0, 3):
            fleets.clear()
            assert provider.ships_available(u.PLAYER_ZERO) == 3 - i
            provider.provide(fleets)
            assert fi.ships[0].position == u.CENTER
            assert fi.signals[0].signal == u.PLAYER_ZERO
        assert provider.ships_available(u.PLAYER_ZERO) == 0
        fleets.clear()
        provider.provide(fleets)
        assert fi.game_over

Test fails on the assert 3 - i, because we never change the _ships member. Code:

    def provide(self, fleets):
        if self._ships:
            self._ships -= 1
            fleets.append(Ship(u.CENTER))
            fleets.append(Signal(u.PLAYER_ZERO))
        else:
            fleets.append(GameOver())

Is GameOver all we do in ShipMaker? I’d best look. Ah, not quite. ShipMaker wants to know that the game is over:

    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

It uses that information so that it doesn’t tick the timer and keep on creating more GameOver instances.

Oops: What we have here is a bit of a mismatch between my vision for ShipProvider, and what ShipMaker actually wants. Arguably, a ShipProvider shouldn’t be providing a GameOver. Let’s go back to the prior idea where it returns a list of things to add.

Redo the test. Mildly irritating but these things happen. The test is a bit simpler:

class TestShipProviders:
    def test_single_provider(self):
        provider = SinglePlayerShipProvider(4)
        assert provider.ships_available(u.PLAYER_ZERO) == 4
        for i in range(0, 4):
            assert provider.ships_available(u.PLAYER_ZERO) == 4 - i
            items = provider.provide()
            ship = next(s for s in items if isinstance(s, Ship))
            assert ship.position == u.CENTER
            signal = next(s for s in items if isinstance(s, Signal))
            assert signal.signal == u.PLAYER_ZERO
        assert provider.ships_available(u.PLAYER_ZERO) == 0
        assert not provider.provide()

Here’s the provider now:

class SinglePlayerShipProvider():
    def __init__(self, number_of_ships):
        self._ships = number_of_ships

    def ships_available(self, _player):
        return self._ships

    def provide(self):
        if self._ships:
            self._ships -= 1
            return [Ship(u.CENTER), Signal(u.PLAYER_ZERO)]
        else:
            return []

We need add_ship.

    def test_single_add(self):
        provider = SinglePlayerShipProvider(4)
        provider.add_ship(u.PLAYER_ZERO)
        assert provider.ships_available(u.PLAYER_ZERO) == 5

class SinglePlayerShipProvider:
    def add_ship(self, _player):
        self._ships += 1

We don’t care what happens if we’re called with any other player number. Not our problem. You call us, we add. I think this is ready to commit: SinglePlayerShipProvider ready to install.

Now for the two player version. But first I want to make an iced chai.

OK. Are you ready? Let’s write a test. We start simply:

    def test_two_player(self):
        provider = TwoPlayerShipProvider(2)
        assert provider.ships_available(u.PLAYER_ZERO) == 2
        assert provider.ships_available(u.PLAYER_ONE) == 2

A true fanatic would have just written the constructor call. Today I am not that true a fanatic. In general, I write as much as is clear in my mind, up until what seems like a small chunk of code to write. The result of this is that I feel that I am going a bit faster, until I take too big a bite and waste a lot of time figuring out what I did wrong. I am not a perfect being. This is what I do and when it bites me, I’ll tell you about it. What do I advise you to do? I advise you to make up your own mind, based on judgment in the light of experience: I’ve already got enough on mine.

We code:

class TwoPlayerShipProvider:
    def __init__(self, number_of_ships):
        self._ships = [number_of_ships, number_of_ships]

    def ships_available(self, player):
        return self._ships[player]

Now we want to see some alternation. Let’s do one ship for each player, longhand:

    def test_one_ship_for_each(self):
        provider = TwoPlayerShipProvider(1)
        items = provider.provide()
        ship = next(s for s in items if isinstance(s, Ship))
        assert ship.position == u.CENTER
        signal = next(s for s in items if isinstance(s, Signal))
        assert signal.signal == u.PLAYER_ZERO
        items = provider.provide()
        ship = next(s for s in items if isinstance(s, Ship))
        assert ship.position == u.CENTER
        signal = next(s for s in items if isinstance(s, Signal))
        assert signal.signal == u.PLAYER_ONE

We need provide. I wrote rather a lot of code for this:

class TwoPlayerShipProvider:
    def __init__(self, number_of_ships):
        self._current_player = 1
        self._ships = [number_of_ships, number_of_ships]

    def ships_available(self, player):
        return self._ships[player]

    def ship_for_player(self, player):
        return [Ship(u.CENTER), Signal(player)]

    def provide(self):
        self.switch_players()
        if self._ships[self._current_player]:
            return self.ship_for_player(self._current_player)
        else:
            self.switch_players()
            if self._ships[self._current_player]:
                return self.ship_for_player(self._current_player)
            else:
                return []
            
    def switch_players(self):
        self._current_player = (self._current_player + 1) % 2

With that much code just rolling off my fingers, we’d best be sure to test seriously.

Added in Post: I think you’ll see that the upcoming tests are pretty decent.

Test is good so far. Extend it a bit, with these two asserts:

        assert not provider.ships_available(u.PLAYER_ZERO)
        assert not provider.ships_available(u.PLAYER_ONE)

One or the other of them fails. I don’t care which because I know we’re not decrementing. Do the decrement:

    def ship_for_player(self, player):
        self._ships[player] -= 1
        return [Ship(u.CENTER), Signal(player)]

Green. Let’s commit this. It’s not done but there’s a lot going on. Commit: TwoPlayerShipProvider progress. This is perfectly safe: no one is using it, and it tests green.

We haven’t checked to be sure it returns empty on another call, and we haven’t dealt with add.

I just add the empty return check at the end of the test:

    def test_one_ship_for_each(self):
        provider = TwoPlayerShipProvider(1)
        items = provider.provide()
        ship = next(s for s in items if isinstance(s, Ship))
        assert ship.position == u.CENTER
        signal = next(s for s in items if isinstance(s, Signal))
        assert signal.signal == u.PLAYER_ZERO
        items = provider.provide()
        ship = next(s for s in items if isinstance(s, Ship))
        assert ship.position == u.CENTER
        signal = next(s for s in items if isinstance(s, Signal))
        assert signal.signal == u.PLAYER_ONE
        assert not provider.ships_available(u.PLAYER_ZERO)
        assert not provider.ships_available(u.PLAYER_ONE)
        assert not provider.provide()

Green. Commit: TwoPlayer checked to return [] when empty.

Now I’d really like to check add_ship and whether we alternate properly.

Here’s what I do first, but I have an idea for what’s next:

    def test_add_for_zero(self):
        provider = TwoPlayerShipProvider(1)
        provider.add_ship(u.PLAYER_ZERO)

This is enough to drive out the add_ship, and I decide to code it correctly, not to fake it:

    def add_ship(self, player):
        self._ships[player] += 1

Test passes so far. Here’s what I wanted to write:

    def test_add_for_zero(self):
        provider = TwoPlayerShipProvider(1)
        provider.add_ship(u.PLAYER_ZERO)
        results = []
        results.append(self.execute_provider(provider))  # 0
        results.append(self.execute_provider(provider))  # 1
        results.append(self.execute_provider(provider))  # 0
        assert results == [0, 1, 0]

    def execute_provider(self, provider):
        items = provider.provide()
        signal = next(s for s in items if isinstance(s, Signal))
        return signal.signal

The values of the Signals tell us what was done. That’s kind of a nifty test, if I do say so myself. We could check inside execute_provider to be sure we got a ship, though I am quite sure that we do. But for completeness:

    def execute_provider(self, provider):
        items = provider.provide()
        assert next(s for s in items if isinstance(s, Ship))
        signal = next(s for s in items if isinstance(s, Signal))
        return signal.signal

Still green. And we know that adding a ship to PLAYER_ZERO works. Now let’s do it the other way around:

    def test_add_for_zone(self):
        provider = TwoPlayerShipProvider(1)
        provider.add_ship(u.PLAYER_ONE)
        results = []
        results.append(self.execute_provider(provider))  # 0
        results.append(self.execute_provider(provider))  # 1
        results.append(self.execute_provider(provider))  # 0
        assert results == [0, 1, 1]

This works also. I mean to say: “WOOT! This works!”

Commit: TwoPlayerShipProvider passing all tests.

We should take a break. The article is long enough, and I’ve been working for two hours. I think this object is just about what we need, though I won’t be surprised to learn that it needs adjustment when we go to fit it in. That’ll be this afternoon, or tomorrow, depending on how the day goes.

Summary

I have imagined a new kind of object, ShipProvider, that comes in two delicious flavors, SinglePlayerShipProvider and TwoPlayerShipProvider. Both providers generate pairs of Ship and associated Signal until they run out of ships, then return an empty list. They handle add_ship(player) and ships_available(player).

Because I imagined the objects, there remains a chance that they aren’t quite what we need. Rather than building this thing speculatively, we might have refactored ShipMaker to extract this functionality from it. Thinking about it, we would probably have wound up with a single ShipProvider that handles the two cases, much as ShipMaker does now. My idea this morning was that two classes, one for Single and one for Two, would be cleaner: each version would do exactly what it should do and no more.

But there is a small risk that it won’t plug in. If it won’t, either we’ll adjust it, or we’ll do something different, in the light of what we now know about providing ships. The knowledge won’t be wasted in any case.

I expect it to work just fine, and yet I know that my expectations are not always fulfilled. We’ll see.

Join me next time, for the thrilling conclusion of

<echo>ShipProvision …</echo>