Python 66 - Refactoring Complete?
We’ll finish up our refactoring of the checking methods down into Fleets and lower. Quite a few odds and ends, none of them difficult. A noticeable improvement to Game.
I believe that today, I’m supposed to start by finishing up the removal of the old way of checking ship spawning, and removing the feature flag. The issue is exemplified by this:
def asteroids_tick(self, delta_time):
self.fleets.tick(delta_time)
if not ShipFleet.rez_from_fleet:
self.check_ship_spawn(self.ship, self.ships, delta_time)
self.control_game(self.ship, delta_time)
self.process_collisions()
self.draw_everything()
game_over = ShipFleet.game_over if ShipFleet.rez_from_fleet else self.game_over
if game_over: self.draw_game_over()
I don’t recall why I ran into trouble late yesterday, when I tried to remove the old code, so I guess I’ll just proceed carefully.
I think we are now running on the new code:
main:
if __name__ == "__main__":
keep_going = True
ShipFleet.rez_from_fleet = True
while keep_going:
asteroids_game = Game()
keep_going = asteroids_game.main_loop()
class ShipFleet(Fleet):
rez_from_fleet = True
ships_remaining = u.SHIPS_PER_QUARTER
game_over = False
def tick(self, delta_time, fleets):
ships = fleets.ships
if self.rez_from_fleet and len(ships) == 0:
self.ship_timer.tick(delta_time, fleets)
super().tick(delta_time, fleets)
return True
It seems to me that I can just remove references to that flag, based on it being true, and we should be good.
def tick(self, delta_time, fleets):
ships = fleets.ships
if len(ships) == 0:
self.ship_timer.tick(delta_time, fleets)
super().tick(delta_time, fleets)
return True
Change:
def draw_available_ships(self):
ship = Ship(Vector2(20, 100))
ship.angle = 90
ships_remaining = ShipFleet.ships_remaining if ShipFleet.rez_from_fleet else self.ships_remaining
for i in range(0, ships_remaining):
self.draw_available_ship(ship)
Thus:
def draw_available_ships(self):
ship = Ship(Vector2(20, 100))
ship.angle = 90
ships_remaining = ShipFleet.ships_remaining
for i in range(0, ships_remaining):
self.draw_available_ship(ship)
Change:
def asteroids_tick(self, delta_time):
self.fleets.tick(delta_time)
if not ShipFleet.rez_from_fleet:
self.check_ship_spawn(self.ship, self.ships, delta_time)
self.control_game(self.ship, delta_time)
self.process_collisions()
self.draw_everything()
game_over = ShipFleet.game_over if ShipFleet.rez_from_fleet else self.game_over
if game_over: self.draw_game_over()
Thus:
def asteroids_tick(self, delta_time):
self.fleets.tick(delta_time)
self.control_game(self.ship, delta_time)
self.process_collisions()
self.draw_everything()
game_over = ShipFleet.game_over
if game_over: self.draw_game_over()
The only references left are setting rez_from_fleet to True. I can test.
We are green and the game works. Remove all those assignments and the flag.
All seems good. Commit: ship spawning done in ShipFleet, feature flag removed.
More to Do
There remains cleanup to do, because there are tests that are relying on the old checking code in game. I didn’t want to just delete those. Let’s review what’s going on.
There are five uses of this method:
class Game:
def check_ship_spawn(self, ship, ships, delta_time):
if ships: return
if self.ships_remaining <= 0:
self.game_over = True
return
self.ship_timer.tick(delta_time, ship, ships)
The tests all check spawning and are redundant: we did new tests for what they do. I’ll remove those two tests and this method.
Clearly we’d like to get rid of this:
class Game:
def spawn_ship_when_ready(self, ship, ships):
if not self.safe_to_emerge(self.missiles, self.asteroids):
return False
ship.reset()
ships.append(ship)
self.ships_remaining -= 1
return True
def set_ship_timer(self, seconds):
self.ship_timer = Timer(seconds, self.spawn_ship_when_ready)
If I do, I think something will break. Only one way to find out. Nothing so far.
I see a method right above the set_ship_timer one:
def safe_to_emerge(self, missiles, asteroids):
if missiles: return False
for asteroid in asteroids:
if asteroid.position.distance_to(u.CENTER) < u.SAFE_EMERGENCE_DISTANCE:
return False
return True
It claims to have five usages. One of those is not referenced. Four are in two tests. I TDD’d all the safety features, so remove those two tests, then the method. We’re green. Commit: remove unused methods from game, corresponding redundant tests.
A Milestone!
Between about article #59 and here, #66, we’ve gone from this:
def asteroids_tick(self, delta_time):
self.check_saucer_spawn(self.saucer, self.saucers, delta_time)
self.check_ship_spawn(self.ship, self.ships, delta_time)
self.check_saucer_firing(delta_time, self. saucers, self.saucer_missiles, self.ships)
self.check_next_wave(delta_time)
self.control_ship(self.ship, delta_time)
self.move_everything(delta_time)
self.process_collisions()
self.draw_everything()
if self.game_over: self.draw_game_over()
To this:
def asteroids_tick(self, delta_time):
self.fleets.tick(delta_time)
self.control_game(self.ship, delta_time)
self.process_collisions()
self.draw_everything()
game_over = ShipFleet.game_over
if game_over: self.draw_game_over()
We can inline that last bit, Command+Option+N:
def asteroids_tick(self, delta_time):
self.fleets.tick(delta_time)
self.control_game(self.ship, delta_time)
self.process_collisions()
self.draw_everything()
if ShipFleet.game_over: self.draw_game_over()
I think there’s another spot like that. Right:
def draw_available_ships(self):
ship = Ship(Vector2(20, 100))
ship.angle = 90
ships_remaining = ShipFleet.ships_remaining
for i in range(0, ships_remaining):
self.draw_available_ship(ship)
Inline:
def draw_available_ships(self):
ship = Ship(Vector2(20, 100))
ship.angle = 90
for i in range(0, ShipFleet.ships_remaining):
self.draw_available_ship(ship)
Commit: inline temps formerly using feature flag.
Along the way we have removed surely at least a dozen other methods from game, there being typically between two and four methods supporting each one of those checks. And we’ve removed all the timers from game as well.
Game class is much improved, but it’s still 182 lines, which is a lot. I see that we have these properties:
@property
def asteroids(self):
return self.fleets.asteroids
@property
def missiles(self):
return self.fleets.missiles
@property
def saucers(self):
return self.fleets.saucers
@property
def saucer_missiles(self):
return self.fleets.saucer_missiles
@property
def ships(self):
return self.fleets.ships
Are we still making any use of these? Let’s find out.
# noinspection PyAttributeOutsideInit
def insert_quarter(self, number_of_ships):
self.asteroids.clear()
self.missiles.clear()
self.saucers.clear()
self.ships.clear()
self.game_over = False
self.score = 0
self.ships_remaining = number_of_ships
self.delta_time = 0
Does Fleets know clear? If not it will:
class Fleets:
def clear(self):
for fleet in self.fleets:
fleet.clear()
class Game:
# noinspection PyAttributeOutsideInit
def insert_quarter(self, number_of_ships):
self.fleets.clear()
self.game_over = False
self.score = 0
self.ships_remaining = number_of_ships # TODO remove
self.delta_time = 0
I just noticed the ships_remaining. That should go. But we’re doing clear.
We’re green. Commit: game uses Fleets.clear to clear fleets.
Arrgh it won’t let me commit with that comment. A bit strict, PyCharm? I commit anyway, it’ll keep reminding me. I’m after the properties.
I think there are no actual references to the asteroids one now. I’ll remove it and test. We’re good. Commit: remove fleet properties from Game, unused.
And I check and remove the ships_remaining thing. Unreferenced but initialized. Left over from when we counted ships in Game.
Commit: remove unused ships_remaining references.
Game class is down to 157 lines now, so those little changes were worthwhile. The methods of Game are these:
asteroids_tick
control_game
draw_available_ship
draw_available_ships
draw_everything
draw_game_over
draw_score
game_init
init_asteroids_game_values
init_fleets
init_game_over
init_general_game_values
init_pygame_and_display
init_score
insert_quarter
main_loop
process_collisions
render_score
A lot of initialization. No surprise there. We could very likely defer some of the drawing elsewhere. The available ships could be drawn in ShipFleet, which has that information. Since it knows the game over state, ShipFleet could probably draw the GAME OVER screen as well.
All that’s for some other day.
Summary
A noticeable improvement to Game, for sure.
Next time we’ll review the changes we made in the subordinate objects, just to see if there’s anything needing to be cleaned up.
The overall process started a bit ragged. I think my mistake was relying on the existing tests—which of course in retrospect I see it clearly now—were all making calls on Game methods that we were trying to remove, so they didn’t help much at all. Once I started TDDing the new implementations, everything went much more smoothly.
And, of course, I was probably learning a bit about how to do it, which surely helped, but I think it was the new tests, done very incrementally, that made the last couple of moves go so much better than the first ones.
I still have a number of sticky notes to review, and I definitely want to review all the new code down in Fleets, Fleet, and the flyers.
Speaking of Fleets and Fleet, it sure seemed that as soon as I made my own classes where formerly there had just been lists, things started to go better. My classes were able to do things that raw collections cannot do, and once I had the Fleet class, it seemed clear that we should make subclasses to do things just for particular kinds of flyers. That was a big part of getting capability where it needed to be.
From article 59 to 66, probably two working days counting all this article writing, and the program is much better.
We’ll review some details in upcoming articles, but I am well pleased.
See you next time!