Skip to main content
added 2871 characters in body
Source Link
Thomas Weller
  • 1.7k
  • 14
  • 25

High CPU usage when game is over

When the game is over, it uses ~14% CPU time on my machine, which means that 1 core is 100% busy. Adding a pygame.time.delay(60) into the while is_over loop helps.

Static members

Your classes seem to define the properties twice, once in __init__ and once as static members. I don't see any usage of the static members, so these can be removed in Snake:

head: List[int, int]
color: Tuple[int, int, int]
body: List[List[int, int]]
direction: str
size: int

and in Food:

x: int
y: int
color: Tuple[int, int, int]
state: bool
position: Tuple[int, int]

Potential stack overflow in game over mode

Once the game is over, there's a loop handling the situation:

while is_over:

I expected the game to get out of this loop when a new round begins. However, that's not the case. Instead there is

if event.key == pygame.K_r:
    game()
    

This is a recursive call. It's unlikely that it will cause problems in this particular game, but in general, this may cause stack overflows.

It can be resolved by introducing another loop

while running:
    while is_over and running:
        ...

    # Initialization code here
    
    while running and not is_over:
        ...

Instead of calling game(), you can then set is_over = False.

Unused variable / unreachable code

The while running loop can be replaced by a while True, since there's no other assignment to running which would terminate the loop.

This also means that the code after while running will never be reached:

pygame.quit()
quit()

Changing the exit routine to running = False, you save some duplicate code and the code runs to the end. This is e.g. helpful if you later want to implement saving a highscore list etc. If you have many exit points during your program, it will be harder to implement something at the end of the game.

You can also omit quit(), because it is not helpful as the last statement of your code.

Smaller improvements

food.update() is only called with False as a parameter. It's never called with True. So this argument can be omitted and go hard-coded into the update() method. The code then looks like this:

while running:
    ...
    food_pos = food.spawn(board_width, board_height, block_size)
    if collision(snake, *food_pos):
        score += 1
        food.update()

This reads like the food is spawning in a new place with every frame. IMHO it reads better like this:

while running:
    ...
    food_pos = food.??? # whatever 
    if collision(snake, *food_pos):
        score += 1
        food.spawn(board_width, board_height, block_size)
        

Because that makes it clear that food only spaws whenever it collided with the snake aka. it was eaten.

High CPU usage when game is over

When the game is over, it uses ~14% CPU time on my machine, which means that 1 core is 100% busy. Adding a pygame.time.delay(60) into the while is_over loop helps.

Static members

Your classes seem to define the properties twice, once in __init__ and once as static members. I don't see any usage of the static members, so these can be removed in Snake:

head: List[int, int]
color: Tuple[int, int, int]
body: List[List[int, int]]
direction: str
size: int

and in Food:

x: int
y: int
color: Tuple[int, int, int]
state: bool
position: Tuple[int, int]

Potential stack overflow in game over mode

Once the game is over, there's a loop handling the situation:

while is_over:

I expected the game to get out of this loop when a new round begins. However, that's not the case. Instead there is

if event.key == pygame.K_r:
    game()
    

This is a recursive call. It's unlikely that it will cause problems in this particular game, but in general, this may cause stack overflows.

It can be resolved by introducing another loop

while running:
    while is_over and running:
        ...

    # Initialization code here
    
    while running and not is_over:
        ...

Instead of calling game(), you can then set is_over = False.

Unused variable / unreachable code

The while running loop can be replaced by a while True, since there's no other assignment to running which would terminate the loop.

This also means that the code after while running will never be reached:

pygame.quit()
quit()

Changing the exit routine to running = False, you save some duplicate code and the code runs to the end. This is e.g. helpful if you later want to implement saving a highscore list etc. If you have many exit points during your program, it will be harder to implement something at the end of the game.

You can also omit quit(), because it is not helpful as the last statement of your code.

Smaller improvements

food.update() is only called with False as a parameter. It's never called with True. So this argument can be omitted and go hard-coded into the update() method. The code then looks like this:

while running:
    ...
    food_pos = food.spawn(board_width, board_height, block_size)
    if collision(snake, *food_pos):
        score += 1
        food.update()

This reads like the food is spawning in a new place with every frame. IMHO it reads better like this:

while running:
    ...
    food_pos = food.??? # whatever 
    if collision(snake, *food_pos):
        score += 1
        food.spawn(board_width, board_height, block_size)
        

Because that makes it clear that food only spaws whenever it collided with the snake aka. it was eaten.

Source Link
Thomas Weller
  • 1.7k
  • 14
  • 25

Thanks for the game. I applied most changes suggested by @Dair and @vurmux as well as partially the suggestions from @Peter Cordes (some of them are not so simple). After that, there were still some things left:

Game logic

I'm not sure whether it was intentional, but the snake does not collide with itself. In usual snake games, if the snake bites into her tail, the game is over as well.

Swallowed keypresses

When playing the game, I noticed that I sometimes cannot perform a U-turn when I needed to. It seems that the first keypress got lost.

The reason is here:

pressed = pygame.key.get_pressed()
if pressed[pygame.K_RIGHT] or pressed[pygame.K_d]:
    ...

which should be changed to

if event.type == pygame.KEYDOWN:
    if event.key == pygame.K_RIGHT or event.key == pygame.K_d:
        snake.change_dir(Direction.RIGHT)
    elif event.key == pygame.K_LEFT or event.key == pygame.K_a:
        snake.change_dir(Direction.LEFT)
    elif event.key == pygame.K_UP or event.key == pygame.K_w:
        snake.change_dir(Direction.UP)
    elif event.key == pygame.K_DOWN or event.key == pygame.K_s:
        snake.change_dir(Direction.DOWN)

However, this brings us to an interesting other problem: doing 2 changes without moving the snake makes the snake run into the opposite direction immediately (without hitting itself as noted before).

Since pygame.event.get() consumes all events, it would be up to you, queuing the events yourself and processing them the next frame.

Classes in files

In addition I tried to apply another principle, which is to have each class in a separate file. Doing so is quite simple with an IDE like Pycharm, since it has the move-refactoring. Unfortunately, it doesn't work any more, because of this:

After the refactoring, Food has

from game import bg_width, block_size, bg_height

So one's left with a cycle of imports due to the use of global variables. Looking at the food class for possible resolutions, you could pass the necessary dependencies as parameters:

def spawn(self, bg_width:int, bg_height:int, block_size:int) -> Tuple[int, int]:

After the refactoring, Snake has

from game import block_size

and a similar solution can apply.

Naming

Assuming that bg is short for background, I really wonder whether that's the correct term here. The snake is moving on a board, not on a background I'd say. In the comment for that line, you call it screen width and height, which may accidentally be the case as well.

Considering a future version of the game where you add a nice background graphic and display the score below the board, neither background nor screen would match any more.

Code duplication

With the change before, I noticed that there's duplicate code in the Food class. Inside __init__, it basically spawns itself.

This duplication can be removed and adding

food = Food(BLUE)
food.spawn(bg_width, bg_height, block_size)

It can later be discussed whether or not Food needs to be spawned that early.

Snake direction change

Note: @Peter Cordes' vector approach is even more elegant. Perhaps the following might show you a refactoring you can apply in other cases as well when a vector does not fit.

After applying the enum suggestion, the direction check looks like this

def change_dir(self, direction: Direction) -> None:
    if self.direction != Direction.LEFT and direction == Direction.RIGHT:
        self.direction = Direction.RIGHT
    elif self.direction != Direction.RIGHT and direction == Direction.LEFT:
        self.direction = Direction.LEFT
    elif self.direction != Direction.DOWN and direction == Direction.UP:
        self.direction = Direction.UP
    elif self.direction != Direction.UP and direction == Direction.DOWN:
        self.direction = Direction.DOWN
        

Combining self.direction = Direction.RIGHT and direction == Direction.RIGHT, we can simplify

self.direction = direction

This applies to all 4 cases, so we end up with

def change_dir(self, direction: Direction) -> None:
    if self.direction != Direction.LEFT and direction == Direction.RIGHT:
        self.direction = direction
    elif self.direction != Direction.RIGHT and direction == Direction.LEFT:
        self.direction = direction
    elif self.direction != Direction.DOWN and direction == Direction.UP:
        self.direction = direction
    elif self.direction != Direction.UP and direction == Direction.DOWN:
        self.direction = direction

Now, we can argue that this is duplicate code and remove the duplication:

def change_dir(self, direction: Direction) -> None:
    if (self.direction != Direction.LEFT and direction == Direction.RIGHT) or \
            (self.direction != Direction.RIGHT and direction == Direction.LEFT) or \
            (self.direction != Direction.DOWN and direction == Direction.UP) or \
            (self.direction != Direction.UP and direction == Direction.DOWN):
        self.direction = direction
    

Personally, I'd even prefer

def change_dir(self, direction: Direction) -> None:
    if self.is_opposite_direction(direction, self.direction):
        return
    self.direction = direction