A simple animation in pygame

Question

I'm following a tutorial into pygame. I realized the example code was written for the sake of being easy to understand. I wondered if I could 'improve' the code, with a focus on keeping the exact same functionality. I tried, but I'm not really sure if this is an improvement.

This is the original code from the instructions:

import pygame, sys
from pygame.locals import *

pygame.init()

FPS = 30 # frames per second setting
fpsClock = pygame.time.Clock()

# set up the window
DISPLAYSURF = pygame.display.set_mode((400, 300), 0, 32)
pygame.display.set_caption('Animation')

WHITE = (255, 255, 255)
catImg = pygame.image.load('cat.png') # hosted at https://inventwithpython.com/cat.png
catx = 10
caty = 10
direction = 'right'

while True: # the main game loop
    DISPLAYSURF.fill(WHITE)

    if direction == 'right':
        catx += 5
        if catx == 280:
            direction = 'down'
    elif direction == 'down':
        caty += 5
        if caty == 220:
            direction = 'left'
    elif direction == 'left':
        catx -= 5
        if catx == 10:
            direction = 'up'
    elif direction == 'up':
        caty -= 5
        if caty == 10:
            direction = 'right'

    DISPLAYSURF.blit(catImg, (catx, caty))

    for event in pygame.event.get():
        if event.type == QUIT:
            pygame.quit()
            sys.exit()

    pygame.display.update()
    fpsClock.tick(FPS)

This is my 'improved' code. Two questions. In general, how can my version be improved and why is that improvement better then my code? And, how could the original be improved?

import pygame, sys
from pygame.locals import *

pygame.init()

FPS = 30
fps_clock = pygame.time.Clock()

window_size = (400,300)
DISPLAYSURF = pygame.display.set_mode(window_size,0,32)
pygame.display.set_caption("Animation test!")

WHITE = (255,255,255)
cat = pygame.image.load('assests/cat.png') # hosted at https://inventwithpython.com/cat.png
direction = 'right'
x_,y_ = 10,10


def listenToQuit():
    for event in pygame.event.get():
        if event.type == QUIT:
            pygame.quit()
            sys.exit()

def detectCollision(cat,x,y,window_size):
    return cat.get_size()[0] + x >= window_size[0] or \
        cat.get_size()[1] + y  >= window_size[1] or \
            x <= 0 or y <= 0

def getDirection(direction):
    directions = ['right','down','left','up']
    idx = directions.index(direction)
    idx = 0 if idx + 1 == len(directions) else idx + 1
    return directions[idx]

def getMovement(x,y,direction):
    if direction == 'right':
        return x+5,y
    elif direction == 'down':
        return x , y+5
    elif direction == 'left':
        return x - 5 , y
    elif direction == 'up':
        return x , y - 5

i = 0
while 1:
    DISPLAYSURF.fill(WHITE)

    x,y = getMovement(x_,y_,direction)
    while detectCollision(cat,x,y,window_size):  # if there would be a collision, change direction 
        direction = getDirection(direction)
        x,y = getMovement(x_,y_,direction) # re-calculate movement, now avoiding collision

    x_,y_ = x,y

    DISPLAYSURF.blit(cat,(x,y))
    listenToQuit()

    pygame.display.update()
    fps_clock.tick(FPS)

I specifically chose not to make the animated object a class. I will try to do so as my next exercise towards myself.

Answer 1

Yours is definitely an improvement, but there is still room for more improvement:

PEP8

The official Python style guide will tell you:

• two newlines between every function (i.e. before detectCollision, etc.)
• function and variable names in snake_case, i.e. detect_collision
• avoid line continuation: this

    return cat.get_size()[0] + x >= window_size[0] or \
        cat.get_size()[1] + y  >= window_size[1] or \
            x <= 0 or y <= 0

should become

return (
    cat.get_size()[0] + x >= window_size[0]
    or cat.get_size()[1] + y  >= window_size[1]
    or x <= 0 
    or y <= 0
)

• spaces after the commas in ['right','down','left','up']
• remove the spaces before the commas here:

        return x , y+5
    elif direction == 'left':
        return x - 5 , y
    elif direction == 'up':
        return x , y - 5

Main function

Move everything starting with i = 0 into a main function that is called with a name guard:

if __name__ == '__main__':
    main()

Constant arrangement

There's a big crazy mix of constants and variables in global scope. I consider these constants that should be grouped together:

FPS = 30  # frames per second setting
WINDOW_SIZE = (400,300)
WHITE = (255, 255, 255)

Nothing else in that region should be in global scope.

Enums

Represent direction as an enum.Enum with four entries, rather than a stringly-typed variable.

Hard exit

Don't exit from listen_to_quit - just break out of the loop.

Better yet - rearrange your code so that you don't need a forever loop:

while not should_quit():
    pygame.display.update()
    fps_clock.tick(FPS)

    DISPLAYSURF.fill(WHITE)

    x,y = getMovement(x_,y_,direction)
    while detectCollision(cat,x,y,window_size):  # if there would be a collision, change direction 
        direction = getDirection(direction)
        x,y = getMovement(x_,y_,direction) # re-calculate movement, now avoiding collision

    x_,y_ = x,y

    DISPLAYSURF.blit(cat,(x,y))