3
\$\begingroup\$

I would really like to hear out some suggestions as well as general response to the code I've written for 4 players snake game. It's my first time using pygame module, as well as trying to design a simple game, in general. I've encountered few bugs which I've managed to fix, but I would like to improve my code and refactor it to make it better. My biggest concern is about "apples" and random generation in general. For sure I repeat myself too much in my code, in few places.

import pygame
from random import randint

WALLS = True
PASSABLE_WALLS = True
GAME_SPEED = 120  # suggsted 120; lower value - faster game play, higher value - slower game play


def is_game_over():
    global game_end
    game_end = not snakes


class Snake:
    def __init__(self, color, head_x, head_y, movement_keys):
        self.segments = [[head_x, head_y]]
        self.prev_pos = None
        self.UP = movement_keys[0]
        self.RIGHT = movement_keys[1]
        self.DOWN = movement_keys[2]
        self.LEFT = movement_keys[3]
        self.direction = None
        self.x = 0
        self.y = 0
        self.COLOR = color
        self.score = 0
        self.moved = True

    def draw_snake(self):
        for segment in self.segments:
            self.draw_segment(segment)

    def draw_segment(self, segment):
        pygame.draw.rect(game_display, self.COLOR, [segment[0], segment[1], TILE_SIZE, TILE_SIZE])

    def eat_apple(self, apple):
        self.score += 1
        apples.remove(apple)
        self.segments.append(self.prev_pos)

    def change_direction(self, direction):
        if self.change_direction_helper(direction):
            self.direction = direction
            self.x = DIRECTIONS[direction][0]
            self.y = DIRECTIONS[direction][1]
            self.moved = False

    def change_direction_helper(self, direction):
        return self.moved and (self.direction is None or
                               (self.direction != direction and DIRECTIONS_REVERSE[self.direction] != direction))

    def move(self):
        # debug
        # if self.direction:
        #     print(self.segments[0], WIDTH, HEIGHT)
        self.prev_pos = [self.segments[-1][0], self.segments[-1][1]]
        previous = self.segments[0][:]
        self.segments[0][0] += self.x
        self.segments[0][1] += self.y
        if PASSABLE_WALLS:
            # if walls are enabled, there's bug that you can hide in wall -- FIXED
            if WALLS:
                if self.segments[0][0] > WIDTH - TILE_SIZE * 2:
                    self.segments[0][0] = 0 + TILE_SIZE

                elif self.segments[0][0] < 0 + TILE_SIZE:
                    self.segments[0][0] = WIDTH - TILE_SIZE * 2

                elif self.segments[0][1] > HEIGHT - TILE_SIZE * 2:
                    self.segments[0][1] = 0 + TILE_SIZE

                elif self.segments[0][1] < 0 + TILE_SIZE:
                    self.segments[0][1] = HEIGHT - TILE_SIZE * 2

            else:
                if self.segments[0][0] > WIDTH - TILE_SIZE:
                    self.segments[0][0] = 0

                elif self.segments[0][0] < 0:
                    self.segments[0][0] = WIDTH - TILE_SIZE

                elif self.segments[0][1] > HEIGHT - TILE_SIZE:
                    self.segments[0][1] = 0

                elif self.segments[0][1] < 0:
                    self.segments[0][1] = HEIGHT - TILE_SIZE

        self.is_collision()
        for i in range(1, len(self.segments)):
            self.segments[i], previous = previous, self.segments[i][:]
        self.moved = True
        # print(apples)

    def is_collision(self):
        # snakes are checked 1 by 1, if both collide, ONLY earlier snake from list will get destroyed
        if not PASSABLE_WALLS:
            if not 0 <= self.segments[0][0] <= WIDTH or not 0 <= self.segments[0][1] <= HEIGHT:
                self.destroy_snake()
        for snake in snakes:
            for segment in snake.segments:
                if segment == self.segments[0] and segment is not self.segments[0]:
                    self.destroy_snake()
                    break

    def destroy_snake(self):
        snakes.remove(self)
        print('DESTROYED', self.COLOR, self.score)
        is_game_over()


def create_apple(app):
    if WALLS:
        # apple can appear in walls -- FIXED
        apple_x = randint(1, WIDTH // TILE_SIZE - 2) * TILE_SIZE
        apple_y = randint(1, HEIGHT // TILE_SIZE - 2) * TILE_SIZE
        app.append([apple_x, apple_y])
    else:
        apple_x = randint(0, WIDTH // TILE_SIZE) * TILE_SIZE
        apple_y = randint(0, HEIGHT // TILE_SIZE) * TILE_SIZE
        app.append([apple_x, apple_y])


def draw_apples():
    for apple in apples:
        pygame.draw.rect(game_display, RED, [apple[0], apple[1], TILE_SIZE, TILE_SIZE])


def draw_walls():
    color = BLACK
    pygame.draw.rect(game_display, color, [0, 0, WIDTH, TILE_SIZE])
    pygame.draw.rect(game_display, color, [0, HEIGHT - TILE_SIZE, WIDTH, TILE_SIZE])
    pygame.draw.rect(game_display, color, [0, 0, TILE_SIZE, HEIGHT])
    pygame.draw.rect(game_display, color, [WIDTH - TILE_SIZE, 0, TILE_SIZE, HEIGHT])

pygame.init()

WIDTH = 800
HEIGHT = 600
game_display = pygame.display.set_mode((WIDTH, HEIGHT))

game_end = False

WHITE = (255, 255, 255)
BLACK = (0, 0, 0)
RED = (255, 0, 0)
GREEN = (0, 255, 0)
BLUE = (0, 0, 255)
YELLOW = (255, 255, 0)
GRAY = (200, 200, 200)
BACKGROUND_COLOR = GRAY
TILE_SIZE = 10
TOTAL_APPLES = 10
DIRECTION_LEFT = "LEFT"
DIRECTION_RIGHT = "RIGHT"
DIRECTION_UP = "UP"
DIRECTION_DOWN = "DOWN"
DIRECTIONS_REVERSE = {DIRECTION_LEFT: DIRECTION_RIGHT,
                      DIRECTION_RIGHT: DIRECTION_LEFT,
                      DIRECTION_UP: DIRECTION_DOWN,
                      DIRECTION_DOWN: DIRECTION_UP}
DIRECTIONS = {DIRECTION_LEFT: (-TILE_SIZE, 0),
              DIRECTION_RIGHT: (TILE_SIZE, 0),
              DIRECTION_UP: (0, -TILE_SIZE),
              DIRECTION_DOWN: (0, TILE_SIZE)}
apples = []
for _ in range(TOTAL_APPLES):
    create_apple(apples)
snakes = [Snake(YELLOW, 400, 400, [pygame.K_UP, pygame.K_RIGHT, pygame.K_DOWN, pygame.K_LEFT]),
          Snake(GREEN, 200, 200, [pygame.K_w, pygame.K_d, pygame.K_s, pygame.K_a]),
          Snake(BLUE, 300, 300, [pygame.K_i, pygame.K_l, pygame.K_k, pygame.K_j]),
          Snake(BLACK, 500, 500, [pygame.K_KP8, pygame.K_KP6, pygame.K_KP2, pygame.K_KP4])]


def main():
    global move_ticker
    global game_end

    for event in pygame.event.get():
        if event.type == pygame.QUIT:
            game_end = True

        if event.type == pygame.KEYDOWN:
            # very rapid change can destroy snake -- FIXED with self.moved in change_direction & move
            for snake in snakes:
                if event.key == snake.LEFT:
                    snake.change_direction(DIRECTION_LEFT)
                elif event.key == snake.RIGHT:
                    snake.change_direction(DIRECTION_RIGHT)
                elif event.key == snake.UP:
                    snake.change_direction(DIRECTION_UP)
                elif event.key == snake.DOWN:
                    snake.change_direction(DIRECTION_DOWN)

    game_display.fill(BACKGROUND_COLOR)
    draw_apples()
    if move_ticker == 0:
        move_ticker = GAME_SPEED
        for snake in snakes:
            snake.move()

    for snake in snakes:
        snake.draw_snake()

    for apple in apples:
        for snake in snakes:
            if snake.segments[0] == apple:
                snake.eat_apple(apple)
                create_apple(apples)

    if WALLS:
        draw_walls()

    pygame.display.update()
    if move_ticker > 0:
        move_ticker -= 1

move_ticker = 0
while not game_end:
    main()
quit()
\$\endgroup\$
3
\$\begingroup\$

is_x functions should not modify anything

As a widely accepted convention, if a function starts with is_ it just returns an information, without changing the state of the world.

As such your is_collision is surprising:

def is_collision(self):
    ....
            self.destroy_snake()

Just change it to return a boolean and destroy the snake from outside.

Use tuple unpacking

It is unnecessary to use a = x[0]; b = x[1]; #..., using , you can assign multiple values at once and simplify:

self.UP, self.RIGHT, self.DOWN, self.LEFT = movement_keys

and:

self.x, self.y = DIRECTIONS[direction]

Avoid changing the world if reasonable

In a game you must change the world a lot, but each change is a possible breaking point, so I suggest minimizing these.

I would change create_apple to return the newly create apple:

def create_apple():
    if WALLS:
        apple_x = randint(1, WIDTH // TILE_SIZE - 2)
        apple_y = randint(1, HEIGHT // TILE_SIZE - 2) 
    else:
        apple_x = randint(0, WIDTH // TILE_SIZE)
        apple_y = randint(0, HEIGHT // TILE_SIZE)
    return [apple_x * TILE_SIZE, apple_y * TILE_SIZE]

This also simplifies the code later because allows the use of a list comprehension:

apples = [create_apple() for _ in range(TOTAL_APPLES)]

Prefer dictionaries over long if elif chains

            if event.key == snake.LEFT:
                snake.change_direction(DIRECTION_LEFT)
            elif event.key == snake.RIGHT:
                snake.change_direction(DIRECTION_RIGHT)
            elif event.key == snake.UP:
                snake.change_direction(DIRECTION_UP)
            elif event.key == snake.DOWN:
                snake.change_direction(DIRECTION_DOWN)

Can be written as:

KEY_TO_DIRECTION = {
     snake.LEFT : DIRECTION_LEFT,
     ...
}

and:

snake.change_direction(KEY_TO_DIRECTION[event.key])

This makes the logic clearer and complexity is handled better in data-structures rather than conditionals.

Reducing repetition

You can use loops to reduce repetition:

def draw_walls():
    color = BLACK
    pygame.draw.rect(game_display, color, [0, 0, WIDTH, TILE_SIZE])
    pygame.draw.rect(game_display, color, [0, HEIGHT - TILE_SIZE, WIDTH, TILE_SIZE])
    pygame.draw.rect(game_display, color, [0, 0, TILE_SIZE, HEIGHT])
    pygame.draw.rect(game_display, color, [WIDTH - TILE_SIZE, 0, TILE_SIZE, HEIGHT])

pygame.draw.rect(game_display, color, is repeated four times, instead you could write:

def draw_walls():
    color = BLACK
    for rect in ([0, 0, WIDTH, TILE_SIZE],
                 [0, HEIGHT - TILE_SIZE, WIDTH, TILE_SIZE],
                 [0, 0, TILE_SIZE, HEIGHT],
                 [WIDTH - TILE_SIZE, 0, TILE_SIZE, HEIGHT]):
        pygame.draw.rect(game_display, color, rect)

This has the advantage of making it obvious that between this calls only the rect changes.

Write additional functions to explain your code at a higher abstraction

The following b0lock of code:

            if self.segments[0][0] > WIDTH - TILE_SIZE:
                self.segments[0][0] = 0

            elif self.segments[0][0] < 0:
                self.segments[0][0] = WIDTH - TILE_SIZE

            elif self.segments[0][1] > HEIGHT - TILE_SIZE:
                self.segments[0][1] = 0

            elif self.segments[0][1] < 0:
                self.segments[0][1] = HEIGHT - TILE_SIZE

Handles the wrapping around the edges of the snake, but the reader has to guess/deduce it, instead you could write a function:

def wrap_around(x, y, width=WIDTH, height=HEIGHT, tile_size=TILE_SIZE):
    # implement

And write:

self.segments[0][0], self.segments[0][1] = wrap_around(self.segments[0][0], self.segments[0][1])

A separate pure function can also be documented and/or tested on its own to further improve the quality of the code.

Naming: helper

Naming a function helper does not help the reader, he can already see it is a helper by seeing that it is used inside a function only once.

Given that change_direction_helper verifies whether it is possible to change direction, I would rename it can_change_direction

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.