Python 176
OK, let’s see if this baby will fit where we intended it to fit.
The moving fingers having writ the ShipProvider classes, let’s see if they can move on to using them in ShipMaker. Let’s review the terrain. The central code is this:
class ShipMaker(Flyer):
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 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 rez_ship_for_player(self, player_number, fleets):
self._ships_remaining[player_number] -= 1
fleets.append(Ship(u.CENTER))
fleets.append(Signal(player_number))
def switch_to_other_player(self):
self._current_player = (self._current_player + 1) % len(self._ships_remaining)
We also have related code:
class ShipMaker(Flyer):
def ships_remaining(self, player_number):
return self._ships_remaining[player_number]
def testing_set_ships_remaining(self, ships):
self._ships_remaining[0] = ships
def add_ship(self, player_identifier):
self._ships_remaining[player_identifier] += 1
player.play("extra_ship")
Let’s just go for it. I don’t see anything truly simple to do here.
- Foreshadowing
- This works out OK, but smaller steps would have been nicer.
Change the init:
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
That becomes this:
def __init__(self, number_of_players):
if number_of_players == 1:
self._provider = SinglePlayerShipProvider(u.SHIPS_PER_QUARTER)
else:
self._provider = TwoPlayerShipProvider(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
That change, removing the ships_remaining member, squiggles all the places that need changing:
def ships_remaining(self, player_number):
return self._provider.ships_available(player_number)
We see our first mis-fit: should rename those two members to have the same name. Not right now, though.
- Foreshadowing
- We finally do it, way down in the final bits. Easy enough of course.
def testing_set_ships_remaining(self, ships):
self._ships_remaining[0] = ships
I had forgotten that entirely. Better do that one. Let’s add a test for it, and make it private in both places:
def test_tests_can_set(self):
provider = SinglePlayerShipProvider(4)
provider.testing_set_ships_remaining(2)
assert provider.ships_available(0) == 2
And:
class SinglePlayerShipProvider:
def testing_set_ships_remaining(self, count):
self._ships = count
PyCharm wanted me to add the method to the TwoPlayer version as well, so I did. But I’m sitting on ten broken tests so need to get done here. Or roll back and think of a simpler path. Let’s continue on: if it goes as well as I expect, we’ll be done and if not we’ll have the learning for the next try.
class ShipProvider(Flyer):
def add_ship(self, player_identifier):
self._provider.add_ship(player_identifier)
player.play("extra_ship")
Still just finding the squiggles and making the obvious changes.
For the actual creation, I think I want to make the change here:
def create_ship(self, fleets):
if not self._safe_to_emerge:
return False
to_append = self._provider.provide()
if to_append:
for flyer in to_append:
fleets.append(flyer)
else:
fleets.append(GameOver())
self._game_over = True
return True
Broken tests are down to two. I think I would like to remove these methods that are not used in ShipMaker, but they may be tested:
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 rez_ship_for_player(self, player_number, fleets):
self._ships_remaining[player_number] -= 1
fleets.append(Ship(u.CENTER))
fleets.append(Signal(player_number))
def switch_to_other_player(self):
self._current_player = (self._current_player + 1) % len(self._ships_remaining)
def ships_remain(self):
return sum(self._ships_remaining) > 0
Let me just remove them and follow my nose. I am not pleased with how any changes this is requiring. We might still roll back.
- Foreshadowing
- We do not. Everything unwinds nicely.
Still only two tests failing. They are, first this:
def test_two_player_free_ship_goes_to_right_ship(self):
fleets = Fleets()
fleets.append(keeper := ScoreKeeper())
fleets.append(maker := ShipMaker(2))
maker.rez_available_ship(fleets)
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
Do we really need this test? I doubt it but I’d like to keep it. Let’s see what the other test is:
def test_unequal_ship_count(self):
fleets = Fleets()
fi = FI(fleets)
maker = ShipMaker(2)
maker._ships_remaining = [1, 3]
self.make_ship_for_player(0, fi, fleets, maker)
self.make_ship_for_player(1, fi, fleets, maker)
self.make_ship_for_player(1, fi, fleets, maker)
self.make_ship_for_player(1, fi, fleets, maker)
assert not maker.ships_remain()
@staticmethod
def make_ship_for_player(player, fi, fleets, maker):
assert maker.ships_remain()
maker.rez_available_ship(fleets)
signal = fi.signals[0]
assert signal.signal == player
fleets.remove(signal)
That’s unhappy because we’re calling ships_remain. We can skip those calls and use the rest.
What about rez_available_ship? I think we could extract from this method and make a usable rez method:
def create_ship(self, fleets):
if not self._safe_to_emerge:
return False
to_append = self._provider.provide()
if to_append:
for flyer in to_append:
fleets.append(flyer)
else:
fleets.append(GameOver())
self._game_over = True
return True
It won’t be quite the same but I think it might be good enough if we do this:
def create_ship(self, fleets):
if not self._safe_to_emerge:
return False
self.rez_available_ship(fleets)
return True
def rez_available_ship(self, fleets):
to_append = self._provider.provide()
if to_append:
for flyer in to_append:
fleets.append(flyer)
else:
fleets.append(GameOver())
self._game_over = True
One of the two passes. Both would have made me happier. Still borked:
def test_unequal_ship_count(self):
fleets = Fleets()
fi = FI(fleets)
maker = ShipMaker(2)
maker._ships_remaining = [1, 3]
self.make_ship_for_player(0, fi, fleets, maker)
self.make_ship_for_player(1, fi, fleets, maker)
self.make_ship_for_player(1, fi, fleets, maker)
self.make_ship_for_player(1, fi, fleets, maker)
assert not maker.ships_remain()
@staticmethod
def make_ship_for_player(player, fi, fleets, maker):
assert maker.ships_remain()
maker.rez_available_ship(fleets)
signal = fi.signals[0]
assert signal.signal == player
fleets.remove(signal)
Ah. I can’t set the ship count quite like that. Let’s change the setup:
def test_unequal_ship_count(self):
fleets = Fleets()
fi = FI(fleets)
maker = ShipMaker(2)
maker._provider._ships = [1, 3]
self.make_ship_for_player(0, fi, fleets, maker)
self.make_ship_for_player(1, fi, fleets, maker)
self.make_ship_for_player(1, fi, fleets, maker)
self.make_ship_for_player(1, fi, fleets, maker)
And we are green. I almost like this. Let me try the game just for extra confidence. Works as advertised. Let’s commit, reflect, and clean up. Commit: ShipMaker uses new ShipProvider objects to, well, provide ships.
Reflection
That went nearly as well as one could have hoped. Basically I just plugged in the new objects and modified all the references to the former member variable _ships_remaining. Most of the tests started running immediately: I never even looked to see what they were.
The two that continued to break were using that somewhat internal method, rez_available_ship. We reproduced a good-enough version of that and used it. That fixed one test and the other uses a very tiny hammer to fudge the number of available ships for testing:
maker._provider._ships = [1, 3]
Not one private access, but two! Fantastic. Let’s improve this. We do have this method on ShipMaker:
def testing_set_ships_remaining(self, ships):
self._provider.testing_set_ships_remaining(ships)
Let’s extend that to accept a list and pass it on down. First change the above test to use our new method:
def test_unequal_ship_count(self):
fleets = Fleets()
fi = FI(fleets)
maker = ShipMaker(2)
maker._provider._ships = [1, 3]
maker.testing_set_ships_remaining([1, 3])
self.make_ship_for_player(0, fi, fleets, maker)
self.make_ship_for_player(1, fi, fleets, maker)
self.make_ship_for_player(1, fi, fleets, maker)
self.make_ship_for_player(1, fi, fleets, maker)
Then find usages and change them to pass a list:
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
def test_two_player_free_ship_goes_to_right_ship(self):
fleets = Fleets()
fleets.append(keeper := ScoreKeeper())
fleets.append(maker := ShipMaker(2))
maker.rez_available_ship(fleets)
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
Change the method to expect a list and pass it on:
class ShipMaker(Flyer):
def testing_set_ships_remaining(self, counts):
self._provider.testing_set_ships_remaining(counts)
class SinglePlayerShipProvider:
def testing_set_ships_remaining(self, counts):
self._ships = counts[0]
class TwoPlayerShipProvider:
def testing_set_ships_remaining(self, counts):
self._ships = counts
I am surprised that this doesn’t make the tests run. This one needed two parameters:
def test_two_player_free_ship_goes_to_right_ship(self):
fleets = Fleets()
fleets.append(keeper := ScoreKeeper())
fleets.append(maker := ShipMaker(2))
maker.rez_available_ship(fleets)
maker.testing_set_ships_remaining([0, 4])
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
I think that test is clearly useless now, but we shouldn’t delete tests at this critical time. The other failure is this, where I had just missed changing the set to use a list:
def test_tests_can_set(self):
provider = SinglePlayerShipProvider(4)
provider.testing_set_ships_remaining([2])
assert provider.ships_available(0) == 2
Green. Commit: Tests no longer quite so invasive. Back to reflection:
Reflection (2)
With that change in place, we still have that testing method. One hates to leave testing methods in the production code. We could get rid of it by monkey-patching it in, or by providing a very invasive test method like the one we just fixed. Or we could bless the ability to set the ship counts, use it in the Providers, and then use it cruelly in the tests. But even to do that, we’d generally have to work through the ShipMaker, so we’d have to change it as well.
OK, what about using add_ship somehow? What about implementing remove_ship?
What about getting over it? I think we’ll get over it.
Code Review
What can we say about the code? I think ShipMaker is clearly better, with its _provider doing all the lifting. What about the Providers themselves?
Well, first, there is no abstract class, and there should be.
class ShipProvider(ABC):
@abstractmethod
def add_ship(self, _player: int):
pass
@abstractmethod
def provide(self) -> list[Ship|Signal]:
pass
@abstractmethod
def ships_available(self, _player:int) -> int:
pass
@abstractmethod
def testing_set_ships_remaining(self, counts: list[int]):
pass
I went entire pig here and specified type hints. Can’t hurt, might help.
Green. Commit: Added abstract class ShipProvider.
I think there are some methods in TwoPlayer that could be marked private:
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 _ship_for_player(self, player):
self._ships[player] -= 1
return [Ship(u.CENTER), Signal(player)]
def _switch_players(self):
self._current_player = (self._current_player + 1) % 2
Again, can’t hurt, might help. I think I’d do well to mark things that way more often.
What about those u.PLAYER_ZERO and u.PLAYER_ONE values? Are they used everywhere they should be?
Not really. What should we really do with them, especially in TwoPlayerShipProvider?
Suppose that those values are just tokens of some kind. Then this is a bit suspect:
class TwoPlayerShipProvider:
def __init__(self, number_of_ships):
self._current_player = 1
self._ships = [number_of_ships, number_of_ships]
Should _ships be named _ship_counts, and should it perhaps be a dictionary from the player token to count?
Let’s try that, see if we like it:
class TwoPlayerShipProvider:
def __init__(self, number_of_ships):
self._current_player = u.PLAYER_ONE
self._ships = {u.PLAYER_ZERO: number_of_ships, u.PLAYER_ONE: number_of_ships}
We can’t quite tolerate this now:
def _switch_players(self):
self._current_player = (self._current_player + 1) % 2
We don’t know, or are at least trying to forget that the player tokens aren’t numbers, they’re just tokens. How about this:
def _switch_players(self):
self._current_player = u.PLAYER_ONE if self._current_player == u.PLAYER_ZERO else u.PLAYER_ZERO
As an old—and I really mean it—C programmer, I like the ternary. YMMV.
Let’s do a bit of renaming:
class TwoPlayerShipProvider:
def __init__(self, number_of_ships):
self._current_player_token = u.PLAYER_ONE
self._ships = {u.PLAYER_ZERO: number_of_ships, u.PLAYER_ONE: number_of_ships}
def add_ship(self, player_token):
self._ships[player_token] += 1
def ships_available(self, player_token):
return self._ships[player_token]
def provide(self):
self._switch_players()
if self._ships[self._current_player_token]:
return self._ship_for_player(self._current_player_token)
else:
self._switch_players()
if self._ships[self._current_player_token]:
return self._ship_for_player(self._current_player_token)
else:
return []
def _ship_for_player(self, player_token):
self._ships[player_token] -= 1
return [Ship(u.CENTER), Signal(player_token)]
def _switch_players(self):
self._current_player_token = u.PLAYER_ONE if self._current_player_token == u.PLAYER_ZERO else u.PLAYER_ZERO
def testing_set_ships_remaining(self, counts):
self._ships[u.PLAYER_ZERO] = counts[0]
self._ships[u.PLAYER_ONE] = counts[1]
I think that’s better. Why? Because now we are (nearly?) unconcerned about the type of u.PLAYER_ZERO and u.PLAYER_ONE. They could be strings or UUIDs or something sensible.
We don’t need to do the same trick in SinglePlayerShipProvider but what we could do is rename it to PlayerZeroShipProvider to make it clear that that’s what it is.
- Possibly Bad Idea
-
Hm, that makes me wonder … would it make sense for TwoPlayerShipProvider to use two SinglePlayerShipProviders, one for each player token?
-
It seems to me that it certainly could use SinglePlayer. But should it?1 I honestly do not know. Might want to try it and see if it is somehow better to use the existing abstraction rather than the new one. Not today, Satan, not today.
We are green as, um, things that are green. Commit: ShipProviders are independent of type of u.PLAYER_ZERO and u.PLAYER_ONE. Within Reason.
I think we’ll call this a wrap.
Summary
The new ShipProvider classes installed quite easily. Tests broke while the changes were underway, then generally healed, except for two that were using internal ShipMaker methods that no longer existed. Those tests were readily converted to use the new scheme.
We then provided an abstract class to act as an interface for the two concrete Provider classes, and then refactored a bit so that u.PLAYER_ZERO and u.PLAYER_ONE are no longer known to be integers inside Providers, but are treated as literal tokens.
This method seems to need renaming somewhere:
class ShipMaker(Flyer):
def ships_remaining(self, player_number):
return self._provider.ships_available(player_number)
I’ll just do that right now. Commit: rename method.
I suspect that there are tests with literal zero or one that are not agnostic about the type of those u variables. I’ll make a note to look for them and adjust them, and will save you the boredom unless it isn’t boring after all.
Overall, my new objects went into place nicely, though I had to make all the changes before everything worked. That went easily, as you can see above, but there were around a half-dozen things that needed to change. Possibly there was a better way. Possibly we could have extracted a combined object first, and then plugged our new thing into it. Possibly we could have refactored from what we had all the way to what we have now, or possibly even to something better.
That’s even more speculative than creating the two little objects was. Those objects came with some very nice tests, so I’m perfectly happy with the result. But I freely grant that the code was red longer than we’d have liked. Ideally, we make one small change and get back to green. We fell short of that ideal. And even so, we have a fine result.
Could it be better? Of course. There’s always room for better.
See you next time!