Python 107 - One More?
I think there’s just one more smart Fleet to remove and then we can greatly simplify Fleets and its interface. I wonder what other specific Asteroids notions may be lurking in the upper levels.
Yes, I’m back on my DELETED again, namely Asteroids. When it comes to refactoring, it seems to me that the great bulk of what needs to be done is independent of the application, the language, and the tools. Ideas are poorly expressed, or could be in a better place. There’s substantial duplication that could be removed. And right there I think you’ve covered 80 or 90 percent of all the code improvement existing code needs. So refactoring Asteroids is just as educational as any other program might be, at least to a first approximation.
Today, we’re going to move the logic from ShipFleet into a new object, ShipMaker. ShipFleet is short but slightly tricky:
class ShipFleet(Fleet):
ships_remaining = u.SHIPS_PER_QUARTER
game_over = False
def __init__(self, flyers):
super().__init__(flyers)
self.ship_timer = Timer(u.SHIP_EMERGENCE_TIME, self.spawn_ship_when_ready)
ShipFleet.ships_remaining = u.SHIPS_PER_QUARTER
ShipFleet.game_over = False
def spawn_ship_when_ready(self, fleets):
if not self.ships_remaining:
ShipFleet.game_over = True
return True
if fleets.safe_to_emerge():
self.append(Ship(u.CENTER))
ShipFleet.ships_remaining -= 1
return True
else:
return False
def tick(self, delta_time, fleets):
if not fleets.ships:
self.ship_timer.tick(delta_time, fleets)
super().tick(delta_time, fleets)
return True
The tricky bit? fleets.safe_to_emerge:
def safe_to_emerge(self):
if self.missiles:
return False
if self.saucer_missiles:
return False
return self.all_asteroids_are_away_from_center()
def all_asteroids_are_away_from_center(self):
for asteroid in self._asteroids:
if asteroid.position.distance_to(u.CENTER) < u.SAFE_EMERGENCE_DISTANCE:
return False
return True
According to our design plans, we don’t want Fleets to have any game logic, so that code will need to be moved somewhere. I’m supposing that it can be moved into ShipMaker.
I think we’d be wise, however, to first move the ShipFleet code, get that working and committed, and then work out how to deal with safe_to_emerge. As a rule, if a change involves more than one part, it seems to me best to do one part at a time, at least if there is a clean break between the two, and in this case, there definitely is, since some of the logic in in ShipFleet class and some in Fleets class.
Another possibly tricky bit: Should ShipMaker know that you get four ships per quarter, or should that information be stored somewhere else? For now, I think we’ll just let that be a constant, which I believe already exists.
We’ll make a ShipMaker test to get things going. I think I’ll just make a new class, TestShipMaker.
Remind me to reorganize my files, right now, as you know if you’ve looked at the GitHub repo, they’re all glommed together. It would surely be better to put the tests in a separate directory.
But not just now.
TDD ShipMaker
class TestShipMaker:
def test_exists(self):
ShipMaker()
This drives out the class, and PyCharm kindly reminds me to implement the required methods:
class ShipMaker(Flyer):
def interact_with(self, other, fleets):
pass
def draw(self, screen):
pass
def tick(self, delta_time, fleet, fleets):
pass
Green. Commit: Initial ShipMaker class.
Now a test that actually does something.
def test_creates_ship(self):
fleets = Fleets()
fi = FI(fleets)
fleets.add_flyer(ShipMaker())
interactor = Interactor(fleets)
interactor.perform_interactions()
assert not fi.ships
fleets.tick(u.SHIP_EMERGENCE_TIME)
assert fi.ships
This test passes, because we have a ShipFleet. So we change Fleets not to create one, changing this:
class Fleets:
def __init__(self, asteroids=(), missiles=(), saucers=(), saucer_missiles=(), ships=()):
self.fleets = dict(
asteroids=Fleet([]),
saucers=Fleet([]),
ships=ShipFleet([]),
flyers=Fleet([]))
Into this:
class Fleets:
def __init__(self, asteroids=(), missiles=(), saucers=(), saucer_missiles=(), ships=()):
self.fleets = dict(
asteroids=Fleet([]),
saucers=Fleet([]),
ships=Fleet([]),
flyers=Fleet([]))
Six tests fail. I was going to review them to see if I can reuse them, but I think instead I’ll work with the tests I’m creating now and then see about the others. Why? Because this test will be just like I want, and the others will surely require revision.
Our new test is failing, of course, so let’s make it work. We can build it in the style of SaucerMaker:
class ShipMaker(Flyer):
def __init__(self):
self._timer = Timer(u.SHIP_EMERGENCE_TIME, self.create_ship)
self._need_ship = True
We need to interact with ships so as to know if we need one. We need begin and interact_with_:
def begin_interactions(self, fleets):
self._need_ship = True
def interact_with_ship(self, ship, fleets):
self._need_ship = False
We need to check in tick and possibly tick our timer:
def tick(self, delta_time, fleet, fleets):
if self._need_ship:
self._timer.tick(delta_time, fleets)
And we need to create a ship. For now, unconditionally.
def create_ship(self, fleets):
fleets.add_ship(Ship())
The Ship requires a position. OK.
def create_ship(self, fleets):
fleets.add_ship(Ship(u.CENTER))
Our test passes. Commit: ShipMaker creates ships indefinitely.
Oh no, wait, that was a BAD idea. I just committed the game without ShipFleet set. Fix it back, commit: ensure Fleets creates ShipFleet.
Broke the game. Ouch. Fixed in under a minute. Still ouch.
Now change Fleets back and try to remember not to commit it until we’re ready.
Anyway, our test works. Let’s look at the failing ones, they might be useful.
Hey. What the heck am I doing committing with broken tests. I’ve gotten way ahead of myself here. Anyway the current version is almost good. Why almost? It does have my broken test in it but it is otherwise green.
I’m going to have to hold off on these commits, or else build some kind of feature flag. I’ll hold off, I think we’re nearly ready for a real commit.
This old test fails with ShipFleet turned off:
def test_ship_rez(self):
ShipFleet.rez_from_fleet = True
fleets = Fleets()
fi = FI(fleets)
assert not fi.ships
fleets.tick(0.1)
assert not fi.ships
fleets.tick(u.SHIP_EMERGENCE_TIME)
assert fi.ships
Add a ShipMaker.
def test_ship_rez(self):
ShipFleet.rez_from_fleet = True
fleets = Fleets()
fleets.add_flyer(ShipMaker())
fi = FI(fleets)
assert not fi.ships
fleets.tick(0.1)
assert not fi.ships
fleets.tick(u.SHIP_EMERGENCE_TIME)
assert fi.ships
It runs. The next one won’t but it will drive out some code for us:
def test_unsafe_because_missile(self):
ShipFleet.rez_from_fleet = True
ships = []
missile = Missile(u.CENTER, Vector2(0, 0), [0, 0, 0], [0, 0, 0])
fleets = Fleets()
fi = FI(fleets)
assert not fi.ships
fleets.tick(u.SHIP_EMERGENCE_TIME - 1)
assert not fi.ships
fleets.add_missile(missile)
fleets.tick(1)
assert not fi.ships
for missile in fi.missiles:
print("removing")
fleets.remove_missile(missile)
fleets.tick(0.001)
assert fi.ships
This test is not as good as it might be. Even the preceding one isn’t quite right.
I need to settle down here, I’m making mistakes.
OK, here’s my plan. I’ll check all the currently broken tests to be sure they are about the ship making, which they probably are. Then I’ll move them all wholesale into my new TestShipMaker class, edit them for proper style, and make them work.
I fix up this one:
def test_unsafe_because_missile(self):
missile = Missile(u.CENTER, Vector2(0, 0), [0, 0, 0], [0, 0, 0])
fleets = Fleets()
fleets.add_flyer(ShipMaker())
interactor = Interactor(fleets)
fi = FI(fleets)
interactor.perform_interactions()
fleets.tick(u.SHIP_EMERGENCE_TIME - 1)
assert not fi.ships
fleets.add_missile(missile)
interactor.perform_interactions()
fleets.tick(1)
assert not fi.ships
for missile in fi.missiles:
fleets.remove_missile(missile)
fleets.tick(0.001)
assert fi.ships
I think that’s good, though I am starting to wish that Fleets initiated interactions for me.
Now in ShipMaker:
class ShipMaker(Flyer):
def __init__(self):
self._timer = Timer(u.SHIP_EMERGENCE_TIME, self.create_ship)
self._need_ship = True
self._safe_to_emerge = False
def begin_interactions(self, fleets):
self._need_ship = True
self._safe_to_emerge = True
def interact_with_ship(self, ship, fleets):
self._need_ship = False
def interact_with_missile(self, missile, fleets):
self._safe_to_emerge = False
If there are any missiles, we don’t pop out. Make that happen here:
def create_ship(self, fleets):
if not self._safe_to_emerge:
return False
fleets.add_ship(Ship(u.CENTER))
return True
Recall that if the Timer action returns False, the timer does not reset.
I expected my test to run. It did not. Oh, I forgot the interactions.
def test_unsafe_because_missile(self):
missile = Missile(u.CENTER, Vector2(0, 0), [0, 0, 0], [0, 0, 0])
fleets = Fleets()
fleets.add_flyer(ShipMaker())
interactor = Interactor(fleets)
fi = FI(fleets)
interactor.perform_interactions()
fleets.tick(u.SHIP_EMERGENCE_TIME - 1)
assert not fi.ships
fleets.add_missile(missile)
interactor.perform_interactions()
fleets.tick(1)
assert not fi.ships
for missile in fi.missiles:
fleets.remove_missile(missile)
interactor.perform_interactions()
fleets.tick(0.001)
assert fi.ships
Suddenly six tests are failing. It was four. Somehow I got the create_ship method tabbed in an extra level. Now my test runs as intended, and we’re down to three broken.
I don’t think this test is necessary:
def test_unsafe_because_saucer_missile(self):
A missile is a missile is a missile. Remove that test. Here’s the next one:
def test_unsafe_because_asteroid(self):
fleets = Fleets()
fleets.add_flyer(ShipMaker())
interactor = Interactor(fleets)
fi = FI(fleets)
asteroid = Asteroid()
asteroid.move_to(u.CENTER + Vector2(u.SAFE_EMERGENCE_DISTANCE - 0.1, 0))
asteroid._location.velocity = Vector2(0, 0)
fleets.add_asteroid(asteroid)
assert not fi.ships
interactor.perform_interactions()
fleets.tick(u.SHIP_EMERGENCE_TIME)
assert not fi.ships
for asteroid in fi.asteroids:
fleets.remove_asteroid(asteroid)
interactor.perform_interactions()
fleets.tick(0.001)
assert fi.ships
This test is not robust enough. The rule is that we don’t rez the ship if asteroids are too close, but the test doesn’t test distance, it only tests presence. We’ll enhance it shortly. No, enhance it now.
def test_unsafe_because_asteroid(self):
fleets = Fleets()
fleets.add_flyer(ShipMaker())
interactor = Interactor(fleets)
fi = FI(fleets)
asteroid = Asteroid()
asteroid.move_to(u.CENTER + Vector2(u.SAFE_EMERGENCE_DISTANCE - 0.1, 0))
asteroid._location.velocity = Vector2(0, 0)
fleets.add_asteroid(asteroid)
assert not fi.ships
interactor.perform_interactions()
fleets.tick(u.SHIP_EMERGENCE_TIME)
assert not fi.ships
asteroid.move_to(u.CENTER + Vector2(u.SAFE_EMERGENCE_DISTANCE + 0.1, 0))
interactor.perform_interactions()
fleets.tick(0.001)
assert fi.ships
I just moved it out of the way instead of removing it.
OK, let’s implement.
def interact_with_asteroid(self, asteroid, fleets):
if asteroid.position.distance_to(u.CENTER) < u.SAFE_EMERGENCE_DISTANCE:
self._safe_to_emerge = False
Our test is green. One left, what is it?
def test_can_run_out_of_ships(self):
fleets = Fleets()
fleets.add_flyer(ShipMaker())
fi = FI(fleets)
ShipFleet.ships_remaining = 2
fleets.tick(u.SHIP_EMERGENCE_TIME)
assert fi.ships
assert fleets.ships.ships_remaining == 1
for ship in fi.ships:
fleets.remove_ship(ship)
fleets.tick(u.SHIP_EMERGENCE_TIME)
assert fi.ships
assert fleets.ships.ships_remaining == 0
assert not fleets.ships.game_over
for ship in fi.ships:
fleets.remove_ship(ship)
fleets.tick(u.SHIP_EMERGENCE_TIME)
assert not fi.ships
assert fleets.ships.game_over
First I’ll fix this up with interactor stuff.
def test_can_run_out_of_ships(self):
fleets = Fleets()
fleets.add_flyer(ShipMaker(2))
interactor = Interactor(fleets)
fi = FI(fleets)
interactor.perform_interactions()
fleets.tick(u.SHIP_EMERGENCE_TIME)
assert fi.ships
# assert fleets.ships.ships_remaining == 1
for ship in fi.ships:
fleets.remove_ship(ship)
interactor.perform_interactions()
fleets.tick(u.SHIP_EMERGENCE_TIME)
assert fi.ships
# assert fleets.ships.ships_remaining == 0
assert not fleets.ships.game_over
for ship in fi.ships:
fleets.remove_ship(ship)
interactor.perform_interactions()
fleets.tick(u.SHIP_EMERGENCE_TIME)
assert not fi.ships
assert fleets.ships.game_over
How does game_over work? It’s a flag, currently in ShipsFleet and it is checked variously including here:
def asteroids_tick(self, delta_time):
self.fleets.tick(delta_time)
self.control_game()
self.process_interactions()
self.draw_everything()
if ShipFleet.game_over:
self.draw_game_over()
For now we’ll have the rule be that Fleets gets the flag.
I have to make ShipMaker understand the input parameter, which I intend to be number of ships.
class ShipMaker(Flyer):
def __init__(self, number_of_ships = u.SHIPS_PER_QUARTER):
self._timer = Timer(u.SHIP_EMERGENCE_TIME, self.create_ship)
self._need_ship = True
self._safe_to_emerge = False
self.number_of_ships = number_of_ships
No, that’s not right. Well, maybe:
class Game:
def draw_everything(self):
screen = self.screen
screen.fill("midnightblue")
self.fleets.draw(screen)
self.draw_score()
self.draw_available_ships()
def draw_available_ships(self):
for i in range(0, ShipFleet.ships_remaining):
self.draw_available_ship(self.available_ship, i)
Note that this is a class variable. Let’s make it a class Variable in Fleets.
I move ships_remaining and game_over to Fleets and change the test:
def test_can_run_out_of_ships(self):
fleets = Fleets()
fleets.ships_remaining = 2
fleets.add_flyer(ShipMaker())
interactor = Interactor(fleets)
fi = FI(fleets)
interactor.perform_interactions()
fleets.tick(u.SHIP_EMERGENCE_TIME)
assert fi.ships
assert fleets.ships_remaining == 1
for ship in fi.ships:
fleets.remove_ship(ship)
interactor.perform_interactions()
fleets.tick(u.SHIP_EMERGENCE_TIME)
assert fi.ships
assert fleets.ships_remaining == 0
assert not fleets.game_over
for ship in fi.ships:
fleets.remove_ship(ship)
interactor.perform_interactions()
fleets.tick(u.SHIP_EMERGENCE_TIME)
assert not fi.ships
assert fleets.game_over
The test runs green, but I lack confidence. I’ll try the game and then think about why I’m not confident. Well the game doesn’t create a ShipMaker so that was useful.
class Game:
def __init__(self, testing=False):
self.delta_time = 0
self.init_pygame_and_display(testing)
self.fleets = Fleets()
self.fleets.add_scorekeeper(ScoreKeeper(testing))
self.fleets.add_wavemaker(WaveMaker())
self.fleets.add_flyer(SaucerMaker())
self.fleets.add_flyer(ShipMaker())
self.running = not testing
Try game again. Ships rez. You only get four. Game over never comes up. Available ship display does not count down.
I’ll remove the game_over var from ShipsFleet which should give me lots of nice errors.
In fact we could remove Ships_Fleet, but I’m still running sans commit and I hesitate.
No, this will work. But still, let’s remove just the variables:
class ShipFleet(Fleet):
ships_remaining = u.SHIPS_PER_QUARTER
game_over = False
I’ll look for references and fix them to point to Fleets or fleets.
As I do this, I realize that what I really need to do is revert. But I’m so close … so close. I’ll try to debug my way to success.
I find the available ships issue:
def draw_available_ships(self):
for i in range(0, self.fleets.ships_remaining):
self.draw_available_ship(self.available_ship, i)
Now the picture counts down correctly. The game over still doesn’t show up. Ah, that’s here:
def asteroids_tick(self, delta_time):
self.fleets.tick(delta_time)
self.control_game()
self.process_interactions()
self.draw_everything()
if ShipFleet.game_over:
self.draw_game_over()
Needs to be:
def asteroids_tick(self, delta_time):
self.fleets.tick(delta_time)
self.control_game()
self.process_interactions()
self.draw_everything()
if self.fleets.game_over:
self.draw_game_over()
Try game to be sure. Available shows correctly and game over comes up. Inserting a quarter works after game over.
I think we’re good. Let’s remove ShipsFleet. I find two imports and all other references are internal to the ShipsFleet. Deleted. Tests are green.
Am I confident? Well, yes, having played a game and seen it work. And I am confident that ShipMaker works correctly, because its tests are rather good. Where I am not confident in my tests is in the connection of the game to the ships available and game over. Presently those are shared variables in the Fleets instance. Those are correctly maintained: my tests show me that. But what the tests do not show is whether the game knows those values, and presently it needs to.
I think we’ll commit this and then reflect about what to do about this confidence issue.
Commit: Game now uses ShipMaker to rez ships.
Reflection: Confidence
To be sure that game, fleets, and the Flyers are cooperating on game over and ships available, could write a couple of easy tests. That would work for now, but we should think more deeply.
I was so close to giving up and trying a revert. But there were only two trivial changes needed at that point. I should have been confident but I was not. Why is that?
The core issue, I think, is that the design itself is a bit rough in the area of available ships and game over. The main loop in the whole program just continually recreates a Game and runs it:
if __name__ == "__main__":
keep_going = True
while keep_going:
asteroids_game = Game()
keep_going = asteroids_game.main_loop()
In Game, there’s this:
def control_game(self):
keys = pygame.key.get_pressed()
if keys[pygame.K_q]:
# noinspection PyAttributeOutsideInit
self.keep_going = True
self.running = False
Setting running to False triggers Game to drop out of its loop:
# noinspection PyAttributeOutsideInit
def main_loop(self):
self.keep_going = False
while self.running:
for event in pygame.event.get():
if event.type == pygame.QUIT:
self.running = False
self.keep_going = False
self.asteroids_tick(self.delta_time)
pygame.display.flip()
self.delta_time = self.clock.tick(60) / 1000
pygame.quit()
return self.keep_going
That juggling of running and keep_going is weird, and it’s hard to see how you’d ever exit the loop with keep_going set to false. What even is the QUIT event? Does it trigger when you close the window? Do we even care?
Point is, the interface between the game and fleets, and the actual objects that know what’s going on, is a bunch of shared values, running, keep_going, game_over, and ships_available, which interact oddly.
Since the way those things work is odd, sort of action at a distance rather than handling things locally or returning a result, like a civilized person, we jam values into objects that don’t really deserve to have that done to them.
It’s messy, I don’t fully understand the interactions, and so when those things started to go wrong, I lost confidence. Fortunately, this time, finding and fixing the problems was easy, and reverting would probably not have paid off. But the area does need cleaning up as we go forward.
Did you notice?
Did you notice that although I thought I’d have to do something special about the safe_to_emerge logic, it just dropped quite nicely into ShipMaker, as three different ways to set the _safe_to_emerge flag?
Did you also notice that somehow I failed to test for the saucer being present and therefore forgot to implement it? Neither did I, until I wrote the sentence above.
def test_unsafe_because_saucer(self):
fleets = Fleets()
fleets.add_flyer(ShipMaker())
interactor = Interactor(fleets)
fi = FI(fleets)
interactor.perform_interactions()
fleets.tick(u.SHIP_EMERGENCE_TIME - 1)
assert not fi.ships
fleets.add_saucer(Saucer())
interactor.perform_interactions()
fleets.tick(1)
assert not fi.ships
for missile in fi.missiles:
fleets.remove_missile(missile)
interactor.perform_interactions()
fleets.tick(0.001)
assert not fi.ships
for saucer in fi.saucers:
fleets.remove_saucer(saucer)
interactor.perform_interactions()
fleets.tick(0.001)
assert fi.ships
We have to remove the missiles because the saucer will fire at least one. But that’s not sufficient, so we remove the saucer and then we get a ship. Voila!
And all we need in the code is:
class ShipMaker(Flyer):
def interact_with_saucer(self, saucer, fleets):
self._safe_to_emerge = False
Commit: Saucer now makes it unsafe to emerge.
Ragged, though. Not my best day. I’m not sure why. Bear very close to biting me. Arguably did nip me a couple of times. I’ll try again. Remember Beckett:
Ever tried. Ever failed. No Matter. Try again. Fail again. Fail better.
The Future
Where do we want to be when we’re “done”?
We want the Game, and the Fleets instance, not to know anything about the game at all. We want to be able to create “any game” by just creating new Flyer instances that collaborate to be Asteroids, or SpaceWar, or Space Invaders, or whatever.
How might that work? Here are some ideas:
- There is a GameOver object that displays the GAME OVER screen, with instructions to insert a Quarter.
- There is a Quarter object that contains the starting load for the game. In Asteroids, that’s a ScoreKeeper, WaveMaker, SaucerMaker, and ShipMaker. When the Quarter runs, it adds that load and removes itself.
- ScoreKeeper should probably know how many ships are available.
- ShipMaker should interact with ScoreKeeper to find out if ships can be made. If so, it tells ScoreKeeper to decrement and rezzes a Ship. (Maybe we do that with a callback.) If there are no ships, it rezzes a GameOver instead.)
- Fleets becomes a simple repository for whatever objects are alive. It probably needs a new method to clear itself for when we insert a quarter. I think right now we create a new Fleets on a quarter, essentially by recreating the whole game.
If those things were done—plus whatever I may have not thought of—the game object would just keep running, displaying whatever displayed, having no idea how things were going. It might just be displaying GAME OVER, or there might be an amazing game going on. Game object wouldn’t know or care, nor would Fleets.
I should mention another crock. When we “insert quarter”, we reinstantiate an entirely new Game and everything on down. The good news is that this pretty much ensures that we start clean, with no leftover ships and all the right counts. The bad news is that it tells us that I was afraid to do anything less drastic.
As we go forward we should do better. This is a lot like rebooting every time you want to write a new document. It works, but is indicative of some kind of a problem.
A Smaller Problem
In my tests, there’s this sequence:
fleets.perform_interactions()
fleets.tick(delta_time)
It occurs many times, and if I forget the perform, the test won’t work right. That suggests that I need at least a function in the tests, and possibly the performing of interactions should be pushed down from game into fleets.
Just a thing I noticed. I’m too tired to fix it, but I see it and will probably see it again, hopefully when I’m not so done.
The Good News
The good news is that we have all the special Fleet instances removed from Fleets class, and we can now begin to simplify that class. It presently holds four Fleet instances, asteroids, ships, saucers, and flyers, where flyers is intended to be the only one we wind up with. With a bit of work, I think we can make Fleets a lot simpler.
I think we’ll want a GameOver object to handle display of GameOver. We’ll just dump it into the mix instead of a Ship when we run out of Ships and it’ll display the screen.
There will be more. (That’s bad news for those who are tired of Asteroids.)
The Larger Point
The point, other than just the sheer fun of crafting in code, is that we can rather freely change our designs, even quite substantially, with a series of small changes. It’s true that this morning I made a misstep and found myself in a situation where I couldn’t commit for about an hour, but on most teams’ scale that is but a moment. I wish I had done better. Possibly a feature flag would have done the job, letting me test the ShipMaker to completion and the flicking the switch. But I didn’t think of it, and then just backed away from quick commits.
The tests were solid and helpful, and I even brought along the old ones. I think they helped, in that they showed what had to work, and hurt, in that editing them to be correct probably took a few minutes longer than rewriting them would have.
That was a mistake. Not the largest one I’ve made in my life, but I could have done better. But the result is still good and the feature is released.
A larger mistake was that I became aware that I was a bit ragged but I kept on keeping on. I’m honestly not sure what I “should” have done, but possibilities include:
- Walk away and come back later;
- Revert and do again, smaller steps;
- Make a check-list of things to do;
- Intentionally pause to think more often.
I’ll try to fail better next time. See you then!