Python 51 - Code Review
There are things not to like. There always are. Let’s find some and improve them.
Last night, while we were chatting, I noticed the init method for the Game class:
def __init__(self, testing=False):
self.asteroids = []
self.asteroids_in_this_wave = None
self.delta_time = 0
self.elapsed_time = 0
self.game_over = False
self.game_over_pos = None
self.game_over_surface = None
self.help_lines = None
self.missiles = []
self.running = False
self.saucer = Saucer(Vector2(u.SCREEN_SIZE/4, u.SCREEN_SIZE/4))
self.saucers = []
self.saucer_timer = 0
self.saucer_zigzag_timer = 0
self.saucer_missiles = []
self.score = 0
self.score_font = None
self.ship = Ship(pygame.Vector2(u.SCREEN_SIZE / 2, u.SCREEN_SIZE / 2))
self.ships = []
self.ship_timer = 0
self.ships_remaining = 0
self.wave_timer = u.ASTEROID_TIMER_STOPPED
if not testing:
self.clock = pygame.time.Clock()
self.screen = pygame.display.set_mode((u.SCREEN_SIZE, u.SCREEN_SIZE))
pygame.init()
pygame.display.set_caption("Asteroids")
Twenty-seven lines, if I’m not mistaken. That seems like a lot. The group talked about it briefly. Bill suggested breaking out the things that “all games” need, like time, maybe score, and moving Asteroids-specific code elsewhere. That might have a desirable side effect, providing a starting point for a new game if we wanted to do one.
Clearly there’s some pygame screen stuff that could be moved over to some new class, including the code isolated under the testing flag, and some random items like game_over_pos and game_over_surface and score_font.
Let’s try re-ordering these lines, grouping them better. I think I’ll move those three variables mentioned above under the not testing if.
if not testing:
pygame.init()
pygame.display.set_caption("Asteroids")
self.clock = pygame.time.Clock()
self.game_over_pos = None
self.game_over_surface = None
self.help_lines = None
self.screen = pygame.display.set_mode((u.SCREEN_SIZE, u.SCREEN_SIZE))
self.score_font = None
self.define_game_over()
self.define_score()
Now I’ll try some additional grouping, first with comments saying what the groups might be.
def __init__(self, testing=False):
# General Game Values
self.delta_time = 0
self.elapsed_time = 0
self.game_over = False
self.running = False
self.score = 0
# Asteroids game values
self.asteroids_in_this_wave: int
self.saucer_timer = 0
self.saucer_zigzag_timer = 0
self.ship_timer = 0
self.ships_remaining = 0
self.wave_timer = u.ASTEROID_TIMER_STOPPED
# Space Objects and Collections
self.asteroids = []
self.missiles = []
self.saucer = Saucer(Vector2(u.SCREEN_SIZE/4, u.SCREEN_SIZE/4))
self.saucers = []
self.saucer_missiles = []
self.ship = Ship(pygame.Vector2(u.SCREEN_SIZE / 2, u.SCREEN_SIZE / 2))
self.ships = []
if not testing:
pygame.init()
pygame.display.set_caption("Asteroids")
self.clock = pygame.time.Clock()
self.game_over_pos = None
self.game_over_surface = None
self.help_lines = None
self.screen = pygame.display.set_mode((u.SCREEN_SIZE, u.SCREEN_SIZE))
self.score_font = None
self.define_game_over()
self.define_score()
That’s nearly better, I think.
We can certainly extract some methods here.
def __init__(self, testing=False):
self.init_general_game_values()
self.init_asteroids_game_values()
self.init_space_objects()
self.init_pygame_and_display(testing)
# noinspection PyAttributeOutsideInit
def init_general_game_values(self):
self.delta_time = 0
self.elapsed_time = 0
self.game_over = False
self.running = False
self.score = 0
# noinspection PyAttributeOutsideInit
def init_asteroids_game_values(self):
self.asteroids_in_this_wave: int
self.saucer_timer = 0
self.saucer_zigzag_timer = 0
self.ship_timer = 0
self.ships_remaining = 0
self.wave_timer = u.ASTEROID_TIMER_STOPPED
# noinspection PyAttributeOutsideInit
def init_space_objects(self):
self.asteroids = []
self.missiles = []
self.saucer = Saucer(Vector2(u.SCREEN_SIZE / 4, u.SCREEN_SIZE / 4))
self.saucers = []
self.saucer_missiles = []
self.ship = Ship(pygame.Vector2(u.SCREEN_SIZE / 2, u.SCREEN_SIZE / 2))
self.ships = []
# noinspection PyAttributeOutsideInit
def init_pygame_and_display(self, testing):
if testing: return
pygame.init()
pygame.display.set_caption("Asteroids")
self.clock = pygame.time.Clock()
self.game_over_pos = None
self.game_over_surface = None
self.help_lines = None
self.screen = pygame.display.set_mode((u.SCREEN_SIZE, u.SCREEN_SIZE))
self.score_font = None
self.define_game_over()
self.define_score()
The quaint noinspection comments suppress PyCharm’s warning that I’m initializing members outside the __init__ function, which I’d argue I am not.
This works. Commit: break init into logically-related chunks.
This isn’t a terribly significant change, but it does make what’s going on a bit more clear.
It looks to me as if we could push the game_over_pos and game_over_surface and help_lines down a bit.
def define_game_over(self):
big_font = pygame.font.SysFont("arial", 64)
small_font = pygame.font.SysFont("arial", 48)
self.game_over_surface = big_font.render("GAME OVER", True, "white")
self.game_over_pos = self.game_over_surface.get_rect(centerx=u.CENTER.x, centery=u.CENTER.y / 2)
pos_left = u.CENTER.x - 150
pos_top = self.game_over_pos.centery
self.help_lines = []
messages = ["d - turn left", "f - turn right", "j - accelerate", "k - fire missile", "q - insert quarter", ]
for message in messages:
pos_top += 60
text = small_font.render(message, True, "white")
text_rect = text.get_rect(topleft=(pos_left, pos_top))
pair = (text, text_rect)
self.help_lines.append(pair)
Let’s rename that to init..., like the others and let those be the first occurrence of those members.
And same with define_score:
# noinspection PyAttributeOutsideInit
def init_score(self):
self.score = 0
self.score_font = pygame.font.SysFont("arial", 48)
Commit: rename define_game_over and define_score to init…
Now when we look at the __init__:
def __init__(self, testing=False):
self.init_general_game_values()
self.init_asteroids_game_values()
self.init_space_objects()
self.init_pygame_and_display(testing)
We can begin to see how things might break out more and more nicely. For example, we could extract the middle two lines into something like init_for_asteroids. From there, and I’m just riffing now, we might extract an entire new class AsteroidsGame, and either defer operations to it from Game, or talk directly to it in some other fashion.
For example, let’s consider the main loop:
self.game_init()
while self.running:
for event in pygame.event.get():
if event.type == pygame.QUIT:
self.running = False
self.check_saucer_spawn(self.saucer, self.saucers, self.delta_time)
self.check_ship_spawn(self.ship, self.ships, self.delta_time)
self.check_next_wave(self.delta_time)
self.check_missile_timeout()
self.control_ship(self.ship, self.delta_time)
self.move_everything(self.delta_time)
self.process_collisions()
self.draw_everything()
if self.game_over: self.draw_game_over()
pygame.display.flip()
self.delta_time = self.clock.tick(60) / 1000
pygame.quit()
Let me extract a method here to show what I’m thinking:
self.game_init()
while self.running:
for event in pygame.event.get():
if event.type == pygame.QUIT:
self.running = False
self.asteroids_tick()
pygame.display.flip()
self.delta_time = self.clock.tick(60) / 1000
pygame.quit()
The stuff that’s left in there is general game code, with all the code for the specific game now in asteroids_tick. We have a lot of references back to our member variables, of course, in the newly extracted method:
def asteroids_tick(self):
self.check_saucer_spawn(self.saucer, self.saucers, self.delta_time)
self.check_ship_spawn(self.ship, self.ships, self.delta_time)
self.check_next_wave(self.delta_time)
self.check_missile_timeout()
self.control_ship(self.ship, self.delta_time)
self.move_everything(self.delta_time)
self.process_collisions()
self.draw_everything()
if self.game_over: self.draw_game_over()
Most of those are legitimately about asteroids … except for self.delta_time. Let’s pass that in as a parameter and use it:
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_next_wave(delta_time)
self.check_missile_timeout()
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()
I notice the check_missile_timeout. Why doesn’t it have delta_time as a parameter? I bet it’s referencing self.delta_time:
for missile in self.missiles.copy():
missile.update(self.missiles, self.delta_time)
for missile in self.saucer_missiles.copy():
missile.update(self.saucer_missiles, self.delta_time)
Change that signature to expect delta_time:
def check_missile_timeout(self, delta_time):
for missile in self.missiles.copy():
missile.update(self.missiles, delta_time)
for missile in self.saucer_missiles.copy():
missile.update(self.saucer_missiles, delta_time)
And caller:
self.check_missile_timeout(self.delta_time)
So that’s nice. Commit: pass delta_time in asteroids_tick rather than use self.delta_time.
Now I’ll check for references to the member delta_time, to see if I have them all.
The Command+B box shows me that all the accesses are at the game level, none at the asteroids level. We’re not far away from having an AsteroidsGame object if we want one.
Let’s reflect and sum up.
Reflecto-Summary
Just grouping the member variables by a general notion of function has led to a series of refactoring changes that could, if we continue, bring us to a separation of the “any game” logic from the “asteroids game” logic. The two resulting classes would each be simpler than the current Game class, with most of the current complexity moving over to AsteroidsGame. It would still be a bit simpler than things are now even if we didn’t see any further breakdown.
With the exception of changing signature, which took a little hand work, all the heavy lifting was done with machine refactoring, Extract Method. I think that I’ve figured out the Change Signature PyCharm refactoring to make that safer than it was this time. I’ll try that soon and report on what I’ve learned, but I think that the “Default Value” field in the refactoring tells PyCharm what to put into the callers so that they have a shot at working. I think I could have put self.delta_time and it would have fixed me right up. Next time, I’ll try that.
I’m not entirely happy about suppressing those warnings, and I certainly could leave them turned on: they just put weak squiggles under the variable references. But the fact of the suppression comment being right there serves to say that I think that I know what I’m doing, long experience notwithstanding. I’ll let it stand for now and see if I wind up regretting it.
I wonder whether the grouping of game variables vs space objects will lead to a useful separation of the AsteroidsGame into two or more parts, perhaps a sort of generic space game and a specific space game involving those collections of space objects. I imagine that we’ll find out, because while we do have some features yet to work on, refactoring this code will contain interesting lessons as well.
For me, this afternoon’s surprise was how valuable just grouping the variables turned out to be. It led me to extraction, and that has led me to a beginning of an idea for breaking things apart. Not that I didn’t have the general idea before, but the separation of the methods has given me a more confident feeling about the idea.
Confidence in an idea is valuable, because it gives that idea a better chance of being tried and being useful.
I look forward to finding out what I do next. I hope you’ll join me.