UPDATE: Felix, the original author of Square Shooter, has made some comments in the comment section that are pretty insightful. I recommend you check them out.

This is a continuation from Part 1, where I go through the source code of Square Shooter, an Asteroids clone, and try to redesign the code to be more readable. (Part 3 is also up.)

All of the changes I make can be viewed on github. You can also download the original source code for Square Shooter.

Just to say this again, readability and software design have an element of subjectivity. The changes I make here aren't necessarily the one true way for this code to be written, and may even be worse (I'm sure you'll be able to check the comments section for people pointing out my errors.)

Note that any line numbers mentioned refer to the line numbers in the original source code.

Renaming some variables and re-spacing code

(These changes can be seen in the github history.)

Grepping to see where GameWorld gets instantiated, I see that the constructor is only called once from line 528 and placed into a global variable named model. Hmmm. I get how the GameWorld is a sort of model, but since the class name is GameWorld, let's rename the model variable to "world" by doing a find-and-replace.

Now let's look at the init_level() method. I'm going to change line 142 from a combination if statment and block to separate lines for the if statement and block:

if (level > self.max_level):

    self.max_level = level

I think this is a bit more readable. I used the combined approach for the if statement and blocks in the wrap_around() method because the blocks were almost identical to each other, having them on the same line (and lined up) makes it easier to compare each other:

def wrap_around(self):

    """Change the position of the bubble to toroidally "wrap around" if it goes off one edge of the map."""

    if self.pos.x < 0: self.pos.x += 1

    if self.pos.y < 0: self.pos.y += 1

    if self.pos.x > 1: self.pos.x -= 1

    if self.pos.y > 1: self.pos.y -= 1

In fact, I'll do the same thing to the Ship's update() method for those timer decrements (and update the comment, something I forgot to do last time):

# powerups degrade over time until it reaches 0.

if self.has_shield():        self._shield_timer       -= delta_t

if self.has_super_bullets(): self._super_bullet_timer -= delta_t

if self.has_freeze():        self._freeze_timer       -= delta_t

The death_timer member in GameWorld is the timer that is set after the player dies but before the level is restarted. This name is a bit ambiguous, so I'll rename it to afterdeath_timer. I'll do the same with finish_timer, which counts down after the last bubble has been shot but before the next level begins.

That looks pretty good. I'll just add some comments to init_level() and then check in my changes with the log message, "Formatting changes and comments to init_level()."

Added comments to the GameWorld class

(These changes can be seen in the github history.)

Now let's look at the GameWorld class's update() method (which is separate from the Ship class's update() method). This is a lot of code that has no comments to help summarize them. All the comments below are mine:

def update(self, delta_t):

    self.handle_collisions(delta_t)



    # expand the explosions and delete them once they get too big

    if len(self.explosions) > 0:

        if self.explosions[0].radius > 0.5:

            self.explosions.pop(0)

    for i in self.explosions:

        i.radius += delta_t



    # "age" the powerups on the map, and delete them if they get too old

    if len(self.powerups) > 0:

        if self.powerups[0].age > 9:

            self.powerups.pop(0)

    for i in self.powerups:

        i.age += delta_t



    # check if all the bubbles have been destroyed

    if len(self.bubbles) == 0:

        if self.afterfinish_timer > 0:

            # the afterfinish timer is still counting down

            self.afterfinish_timer -= delta_t;

        else:

            # the afterfinish timer is done, add a life and set up the next level

            self.level += 1

            self.lives += 1

            self.init_level(self.level)

            return

    elif not self.ship.has_freeze():

        # update all the bubbles

        for i in self.bubbles:

            i.update(delta_t)



    # update the bullet

    if self.bullet != None:

        bullet_wrapped = self.bullet.update(delta_t)

        if bullet_wrapped:

            # delete the bullet if it has hit the edge of the map

            self.bullet = None



    # update the ship

    if self.ship == None:

        if self.afterdeath_timer > 0:

            # player is dead and afterdeath timer is still counting down

            self.afterdeath_timer -= delta_t

        elif self.lives > 0:

            # create a new Ship for the next level

            self.ship = Ship()

            self.ship.add_shield() # add shields at the start of a life

        else:

            # player has run out of lives, level 0 will make the start screen display

            self.level = 0 # Game over

        return



    self.ship.update(delta_t)

Checking this in with log message, "Added comments to GameWorld.update()."

Refactoring the GameWorld class

(These changes can be seen in the github history.)

Now that it has some documentation, let's see how we can improve the GameWorld class. One thing that is odd is that the powerups and explosions are removed if they are too old, and then they are aged. This means that old explosions and powerups won't be removed until the next call to update(). Let's swap the order of that and age them first before doing the age check:

# expand the explosions and delete them once they get too big

for i in self.explosions:

    i.radius += delta_t

if len(self.explosions) > 0:

    if self.explosions[0].radius > 0.5:

        self.explosions.pop(0)



# "age" the powerups on the map, and delete them if they get too old

for i in self.powerups:

    i.age += delta_t

if len(self.powerups) > 0:

    if self.powerups[0].age > 9:

        self.powerups.pop(0)

Also, there seems to be a slight bug with this code. It only checks if the first explosion or power up in the explosions and powerups lists is too old. (You can tell by the self.explosions[0] and self.powerups[0] part.) We should add a loop to these if statements so that every explosion and powerup is checked. (The reason we don't see this bug in the game is because update() is called so frequently that the powerups and explosions are quickly removed in future update() calls.) Notice that we iterate from the end of the self.explosions and self.powerups list to the beginning, because we are modifying the items in the list as we iterate.

# expand the explosions and delete them once they get too big

for i in self.explosions:

    i.radius += delta_t

for i in range(len(self.explosions - 1, -1, -1)):

    if self.explosions[i].radius > 0.5:

        self.explosions.pop(i)



# "age" the powerups on the map, and delete them if they get too old

for i in self.powerups:

    i.age += delta_t

for i in range(len(self.powerups - 1, -1, -1)):

    if self.powerups[i].age > 9:

        self.powerups.pop(i)

Let's also replace the 0.5 and 9 magic numbers with global constants MAX_EXPLOSION_SIZE and MAX_POWERUP_AGE:

MAX_EXPLOSION_SIZE = 0.5 # set between 0.0 and 1.0

MAX_POWERUP_AGE = 9 # in seconds



...



        # expand the explosions and delete them once they get too big

        for i in self.explosions:

            i.radius += delta_t

        for i in range(len(self.explosions) - 1, -1, -1):

            if self.explosions[i].radius > MAX_EXPLOSION_SIZE:

                self.explosions.pop(i)



        # "age" the powerups on the map, and delete them if they get too old

        for i in self.powerups:

            i.age += delta_t

        for i in range(len(self.powerups) - 1, -1, -1):

            if self.powerups[i].age > MAX_POWERUP_AGE:

                self.powerups.pop(i)

It is more Pythonic to just use not len(self.bubbles) instead of len(self.bubbles) == 0 for conditions, since the integer value 0 evaluates to False anyway:

# check if all the bubbles have been destroyed

if not len(self.bubbles):

And the variable name i is usually used to mean "index". Since the for loop for updating the bubbles is iterating over Bubble objects instead of an integer index, let's rename i to bubble:

# update all the bubbles

for bubble in self.bubbles:

    bubble.update(delta_t)

Let's put the self.ship.update() call into an else block. Technically, if we call self.ship.update() unconditionally, then we're simulating delta_t seconds for the ship even though it was just created (on line 205). (This is such a small bug, though, that we haven't noticed it before.)

else:

    self.ship.update(delta_t)

I'll check in these changes with the comment "Improved the GameWorld.update() code." after testing my changes.

Refactoring init_level()

(These changes can be seen in the github history.)

Going back to the init_level() code, I realize that rather than use del on the items in the bubbles, explosions, and powerups lists, we can just set those members to blank lists and Python's garbage collector will delete the objects for us.

# clear out the bubbles, explosions, and power ups from the last level.

self.bubbles    = []

self.explosions = []

self.powerups   = []

Also, we can get rid of the for loop and simply use a list comprehension to create all the starting bubbles. This shows us that the self.bubbles = [] line wasn't need before:

# clear out the explosions, and power ups from the last level.

self.explosions = []

self.powerups   = []



# create a number of starting big bubbles as the level number.

self.bubbles = [Bubble("big") for i in range(level)]

Checking this in with the message, "Simplifying init_level() code."

Fixing some bugs I introduced

(These changes can be seen in the github history.)

I didn't test my changes too thoroughly, and introduced a couple of bugs. There are some times when the player has died and the self.ship member in GameWorld is set to None, but an attribute of self.ship is checked. I've added some self.ship != None checks to these if statements.

The render_ship() method also has a place where we need to do a check for the ship being set to None.

if self.world.ship != None and self.world.ship.has_super_bullets():

    pygame.draw.rect(self.screen, RED, bbox, 1)

I got ahead of myself by accident, I also checked in a change I made to handle_collisions(). Instead of calling b.collides_with(self.bullet) I changed it to self.bullet.collides_with(b), since it reads better as "the bullet collided with the bubble" rather than "the bubble collided with the bullet".

I've checked these in here and here and here.

Improving handle_collisions()

(These changes can be seen in the github history.)

I'm going to add a new comment that explains that non-super bullets go away when they hit a bubble:

self.bullet = None # delete the non-super bullet when it hits a bubble

Also, there's another place we can change len(self.bubbles) == 0 to just not len(self.bubbles):

if not len(self.bubbles):

   self.afterfinish_timer = 3

The code that follows the elif self.ship != None: statement can be simplified. This code checks that if a bubble has hit the ship and handles the player dying. Let's simplify lines 235 to 244:

# check if the bubble has hit the ship

if self.ship != None and b.collides_with(self.ship) and not self.ship.has_shield():

    self.spawn_explosion(self.ship)

    self.ship = None

    self.lives -= 1

    self.afterdeath_timer = 3;

    break

We can also get rid of the if self.ship == None: return line on line 246. The point of this line is to exit the function if ship is set to None, that way the code that checks if the ship and powerups have collided won't cause errors. But if we want to add code after the powerups collision check, then this function might prematurely exit.

Instead of returning, let's set the code to just skip the powerups collision checks:

if self.ship != None:

    for p in self.powerups[:]:

        if p.collides_with(self.ship):

            self.apply_powerup(p)

            self.powerups.remove(p)

That's good for now. Let's check this in with the log message, "Refactoring handle_collisions()"

Refactoring the rest of GameWorld

(These changes can be seen in the github history.)

The spawn_explosion() method looks okay. We'll leave it as is.

The code in mark_score() is a bunch of if-elif statements that change the same variable by different values. Let's put this info in a "kinds" dictionary like we did in the Bubble class:

def mark_score(self, bubble):

    #                   score

    kinds  = {'big':    1,

              'medium': 2,

              'small':  5}

    self.score += kinds[bubble.kind]

We'll do the same thing for the apply_powerup() method, but instead of mapping a bubble kind string value to a score integer value, we will use a dictionary to map a powerup kind string value to a function. (Python lets us store functions just like any other value.)

def apply_powerup(self, powerup):

    #                  function to call

    kinds = {'shield': self.ship.add_shield,

             'bullet': self.ship.add_super_bullets,

             'freeze': self.ship.add_freeze}

    kinds[powerup.kind]()

I thought about replacing this code:

if self.score > self.high_score:

    self.high_score = self.score

...with this code:

self.high_score = max(self.score, self.high_score)

...but decided against it. It's one less line, but it doesn't necessarily improve readability.

Checking these changes in with the log message, "Refactored mark_score() and apply_powerup()"

Fixing a regression

(These changes can be seen in the github history.)

In the process of fixing a bug, I've accidentally introduced a new bug. These kinds of bugs are known as regressions. In GameWorld's update() method, I have this code:

elif self.ship != None and not self.ship.has_freeze():

    # update all the bubbles

    for bubble in self.bubbles:

        bubble.update(delta_t)

This updates the position of all the bubbles, causing them to move around the game map. I added the self.ship != None check so that the not self.ship.has_freeze() wouldn't cause an exception due to the value of self.ship being None (and thus attempting to call None.has_freeze()).

(The second part of the condition, not self.ship.has_freeze(), is not evaluated if the first part, self.ship != None, is True because of short-circuit evaluation. In the expression False and blahblahblah, it doesn't matter if blahblahblah is True or False because the first part is False, so Python skips checking the second part.)

However, this means that when the ship doesn't exist, the bubble positions don't update. You can see this when the ship gets destroyed: all the bubbles freeze until the player's next life begins. The code should be changed to this:

elif self.ship == None or not self.ship.has_freeze():

    # update all the bubbles

    for bubble in self.bubbles:

        bubble.update(delta_t)

Because of short-circuiting, if self.ship is None, then the not self.ship.has_freeze() part of the condition won't be evaluated. (There's no reason to check: the expression True or blahblahblah will evaluate to True no matter if blahblahblah is True or False, so Python skips it.)

Now when we run the program and die, you can see the bubbles keep moving.

I'll check these changes in under the log message, "Fixed a regression where the bubbles freeze after the player dies."

Refactoring GameScreen

(These changes can be seen in the github history.)

The first thing I notice is that there is code that does exactly what our WINDOW_WIDTH and WINDOW_HEIGHT constants do:

self.width, self.height = screen.get_size()

We can just delete this line since we don't need it anymore, and replace the uses of self.height with WINDOW_HEIGHT.

self.hud_font =    pygame.font.SysFont(

    font_name, WINDOW_HEIGHT / 10)

self.msg_font = pygame.font.SysFont(

    font_name, WINDOW_HEIGHT / 20)

The code that bases the font size on the window height isn't ideal, because the font sizes are in points and the window size is in pixels, and they don't increase proportional. You can see this for yourself by increasing WINDOW_HEIGHT (and MAP_HEIGHT) and running the program.

Actually, it's a bit annoying that we have to change both WINDOW_HEIGHT and MAP_HEIGHT. Let's add code to the top of the file that sets MAP_WIDTH and MAP_HEIGHT based on the values of WINDOW_WIDTH and WINDOW_HEIGHT:

WINDOW_WIDTH = 640

WINDOW_HEIGHT = 480



MAP_WIDTH = WINDOW_WIDTH - 160

MAP_HEIGHT = WINDOW_HEIGHT

Also, the render_backround() method is misspelled. Let's fix that.

Checking this in with the log message, "Refactoring GameScreen's constructor."

Refactoring GameScreen.render()

(These changes can be seen in the github history.)

The first line in the render() method is a bit of syntactic sugar:

m = self.world

This is so that we only have to type m instead of self.world in this function. The m made sense before we replaced the "model" variable with "world". So we should change this m to w. But really, I think it's more readable just to use self.world instead of m. Let's get rid of this line and change all the m variables to self.world.

The same bit of syntactic sugar is in render_game_world(), so we'll replace m with self.world there too.

The render() code has this weird green-box drawing code:

if self.world.level == 0:

    self.render_title_screen()

    # Hide the [P]ause text.

    self.screen.fill(GREEN, (MAP_WIDTH + 20, 424, 140, 24))

else:

    self.render_game_world()

    # Hide the [P]lay text.

    self.screen.fill(GREEN, (MAP_WIDTH + 20, 400, 140, 24))

This code is here because the background layer pygame.Surface object has both [P]lay and [P]ause drawn on it, but it only makes sense to show one or the other while the game runs. So this code draws a green box to hide one of the pieces of text.

This is a less than graceful way to handle this. Felix probably wanted to skip rendering the background pygame.Surface object each time the game map was rendered and make the code run faster. But adding a call to self.render_background() in the render() method doesn't introduce any noticeable performance issues, so I'd say this is a case of premature optimization.

Let's add a call to self.render_background() in render(), and also modify the render_background() method to have a parameter that tells it whether or not to render [P]lay or [P]ause. That way we can skip this silly green-box drawing code. (Also, this means we can remove the self.render_background() line from __init__(), since now the background is rendered with the rest of the map.)

Also, I'm going to separate the combined if-and-block line on line 380 into two lines.

def render(self):

    self.screen.blit(self.bglayer, (0, 0))

    self.render_background(self.world.level)



    if self.world.level == 0:

        self.render_title_screen()

    else:

        self.render_game_world()

        if self.game_paused:

            self.render_pause_text()

(Ideally, we'd have some other variable set to track the state of whether or not we show the status screen or the game play screen. But I'll just leave in the convention of level 0 being the status screen for now.)

The render_background() method is altered to either display [P]lay or [P]ause:

def render_background(self, level):



...



    text = self.msg_font.render('[Q]uit', False, WHITE)

    self.bglayer.blit(text, (MAP_WIDTH + 20, 424))



    if level == 0:

        msg = '[P]lay'

    else:

        msg = '[P]ause'

    text = self.msg_font.render(msg, False, WHITE)

    self.bglayer.blit(text, (MAP_WIDTH + 20, 400))

Also, let's delete these two commented out lines, since even if we uncomment them they wouldn't work due to a lack of a self.fps variable:

#fps_text = self.msg_font.render(str(self.fps), False, GREEN)

#self.screen.blit(fps_text, (0, 0))

I'll check this in with the log message, "Refactored the GameScreen.render() method, and got rid of the green box drawing code."

Whoops.

(These changes can be seen in the github history.)

I accidentally checked in the code before deleting those commented out lines. I'll remove them in this check in.

Moving render_ship() and render_bullet()

(These changes can be seen in the github history.)

The render_title_screen() method looks fine. But the other render methods like render_ship(), render_bullet(), and render_powerup() would be better if they were in the Ship, Bullet, and Powerup classes respectively. I'll move them there and simply rename them all render() (just like how the update() name is used by the ObjectOnMap subclasses.) These render() methods will need a parameter for the pygame.Surface object to draw themselves on.

These lines in render_ship() can be removed once this method can be removed after the code is moved to the Ship class, since self.world.ship will just be self and ship.pos will simply be self.pos.

ship = self.world.ship

pos = ship.pos

Here's the code for the Ship.render() method, compare it to the old render_ship() code:

def render(self, surface):

    bbox = pygame.draw.circle(

        surface,

        SILVER,

        scale_and_round(self.pos.x, self.pos.y),

        int(round(self.radius * MAP_SIZE)))

    pygame.draw.circle(

        surface,

        BLACK,

        scale_and_round(self.pos.x, self.pos.y),

        int(round(self.radius * 0.5 * MAP_SIZE)),

        1)

    if self.has_shield():

        pygame.draw.rect(surface, SILVER, bbox, 1)

And here's the code to the new Bullet.render() method, compare it to the old render_bullet() code:

def render(self, surface):

    bbox = pygame.draw.circle(

        surface,

        RED,

        scale_and_round(self.pos.x, self.pos.y),

        int(round(self.radius * MAP_SIZE)))

    if self.shield:

        pygame.draw.rect(surface, RED, bbox, 1)

I'll check this in as "Moved render_bullet() and render_ship() to the Bullet and Ship classes."

Creating render() methods for Bubble and a new Explosion class

(These changes can be seen in the github history.)

Let's continue the trend of giving classes their own render() methods with the Bubble class:

def render(self, surface):

    pygame.draw.circle(

        surface,

        self.color,

        scale_and_round(self.pos.x, self.pos.y),

        int(round(self.radius * MAP_SIZE)),

        1)

Currently explosions are just ObjectOnMap objects, since we only needed them to have a radius and position. But we should give them their own class so they can have their own render() and update() methods. Currently this code is handled by the GameWorld and GameScreen classes instead of a unified place.

The new Explosion class looks like this:

class Explosion(ObjectOnMap):

    def __init__(self):

        super(Explosion, self).__init__(0) # explosions start at size 0



    def update(self, delta_t):

        self.radius += delta_t



    def render(self, surface):

        pygame.draw.circle(

            surface,

            RED,

            scale_and_round(self.pos.x, self.pos.y),

            int(round(self.radius * MAP_SIZE)),

            1)

I'll check in these changes with the log message, "Added an Explosion class, and created Bubble.render()"

Creatind a render() method for Powerup.

(These changes can be seen in the github history.)

The render() method for Powerup has a lot of code in it since it has to draw three different types of Powerup. We could split the Powerup class into three subclasses, but I think that's object-oriented overkill.

I've created a scaled_x and scaled_y variable inside render() to replace all the scale_and_round(self.pos.x, self.pos.y) calls. The utility of this is negligible. I almost made a scaled_r to replace the int(round(self.radius * MAP_SIZE)) expressions. Other than that, the code is mostly untouched.

I've also replaced the i in the for loop with "powerup" since it is not an index:

for powerup in self.world.powerups:

    powerup.render(self.screen)

"Created a render() method for Powerup."

And I made another check in: "Whoops. Left in a high powerup chance while testing."

At this point, our render_game_world() method in the GameScreen class looks very straightforward:

def render_game_world(self):

    self.screen.set_clip((0, 0, MAP_WIDTH, MAP_HEIGHT))



    if self.world.ship != None:

        self.world.ship.render(self.screen)

    if self.world.bullet != None:

        self.world.bullet.render(self.screen)



    for bubble in self.world.bubbles:

        bubble.render(self.screen)



    for explosion in self.world.explosions:

        explosion.render(self.screen)



    for powerup in self.world.powerups:

        powerup.render(self.screen)



    self.screen.set_clip(None)

The code in the render_pause_text() method looks fine, I'll just leave it like that.

Adding a debug switch for displaying FPS

(These changes can be seen in the github history.)

Whoops. Those two commented out lines actually did do something useful. The catch is it required you to uncomment a third line that was in the main game loop.

This code displays the current frames per second (FPS) in the upper left corner, which can be useful for measuring performance. Instead of commenting and uncommenting these lines whenever you want to see them, let's add a DISPLAY_FPS global variable that can be set to True or False, and then put an if DISPLAY_FPS: statement in front of these lines:

DISPLAY_FPS = True



...



if DISPLAY_FPS:

    fps_text = self.msg_font.render(str(self.fps), False, GREEN)

    self.screen.blit(fps_text, (0, 0))



...



if DISPLAY_FPS:

    renderer.fps = int(round(clock.get_fps()))

I'll check in this code with the log message, "Adding DISPLAY_FPS for debugging."

No need for a running variable

(These changes can be seen in the github history.)

The main game loop in the global scope at the bottom of the file continues handling events as long as the running variable is set to True. All of the code that needs to end the program do so by setting running to False. However, since there is no code that runs inside the loop if running is set to False, we can get rid of this variable entirely.

Instead, we'll just have the loop be a while True: infinite loop and the code that previously set running to False will just execute a break instead. That's one less variable we have to deal with.

Checked in with log message, "Get rid of the running variable."

End of Part 2

That's all for now. We're almost done refactoring the code. I think the GameScreen code should probably be merged into GameWorld, and I'd like to take a lot of the GameWorld-specific code out of the globally-scoped game loop. We'll cover that in Part 3, as well as making UX (user experience) design changes (instead of just the code design changes we've made in the first two Parts.)

Remember, you can see the full list of git check ins I've made for the code changes on the github page for this blog post and compare it to the original source code for Square Shooter.