PEP 8

PEP 8 recommends that constants be written as "all capital letters with underscores separating words.". So something like:

yellow = (255, 255, 0)

Should be:

YELLOW = (255, 255, 0)

or possibly an enum (more on this later)

Inconsistent quotations

Usually a project will stick with either "" or '' unless you have particular reason not to.¹ But for instance:

pygame.display.set_caption("Snake")

font = pygame.font.SysFont('Times New Roman', 20)

text_colour = pygame.Color('White')

violates the uniformity unnecessarily.

Enums

To quote the docs on enum:

An enumeration is a set of symbolic names (members) bound to unique, constant values. Within an enumeration, the members can be compared by identity, and the enumeration itself can be iterated over.

So for instance:

left = "LEFT"

right = "RIGHT"

up = "UP"

down = "DOWN"

becomes:

from enum import Enum

class Direction(Enum):

    LEFT = "LEFT"

    RIGHT = "RIGHT"

    DOWN = "DOWN"

    UP = "UP"

See here for more reasons why you should port your stringy code to enums. In addition, there are probably some integer arithmetic tricks you could use to eliminate a lot of the if-elif-else chains. (You will need to do some further refactoring to get this to run without error.)

Unnecessary Comments

Consider:

# initialize food and snake

food = Food(blue)

snake = Snake(green)



# initialize loop logic

running = True

is_over = False



# initialize score

score = 0



# game loop

while running:

I, personally, would omit all the comments. I could see where a case can be made the game loop comments in the case that someone is trying to understand the code without knowledge of game loops, but I would argue the concept of a game loop is so ubiquitous that those comments get in the way.

If you had to write comments I would instead write:

# Initialize entities/data for new game

food = Food(blue)

snake = Snake(green)

score = 0



# Init/start game loop

running = True

is_over = False

while running:

    ...

Even then, I'm not quite satisfied with it, but the moral it to not add redundant comments. However, there may be some debate on this.

The best example is probably:

    # move snake

    snake.move()

Really think about what this comment contributes.

Unnecessary assignments

I'm not sure why you wrote:

    # check for collision with wall:

    collision_with_wall = wall_collision(snake)

    if collision_with_wall:

        is_over = True

When:

    if wall_collision(snake):

        is_over = True

suffices.

Add a main function

You should consider adding a main function to your project. This is usually done through the introduction of:

if __name__ == "__main__":

    game()

This allows for me to import your project and not have it automatically execute the game function at the bottom. Increasing re-usability.

^{1 For instance, It may be the project defaults to "", but you need to print out " Hi " with the quotes, so print('" Hi "') may be chosen.}

In addition to another answer I will add these places to improve (I will not repeat all things from that answer, my answer is just an addition):

1. Code inconsistency

You have several places in your code where your code is inconsistent

def collision():

...

        return 1

    return 0





def wall_collision():

...

        return True

    return False

Both functions are checking collisions but one returns integer (0/1) and another - boolean (True/False).

self.head = [int(10*block_size), int(5*block_size)]

self.body = [self.head, [9*block_size, 5*block_size]]

For head you convert X*block_size to int (it is unnecessary anyway). For body you don't do it.

I recommend you to always check your code for possible inconsistency.

2. Names inconsistency

I moved it to an another point because it is not about how your code works - it is about how another programmers will read your code.

Look at this two lines:

def change_dir(self, direc: str) -> None:

        if self.direction != left and direc == right:

You use three different namings for direction entity:

• dir


• direct


• direction

It is far better to use consistent variable names. If you are using different direction variables, get them names with only one word for directions. Here is the example:

• snake_direction


• pokemon_direction


• some_strange_direction

or:

• enemy_dir


• arrow_dir


• some_another_strange_dir

3. Small code improvements

Sometimes you look at your code and think: "Hmmm, it is working but looks not good". Often in these cases you can slightly reorganize your code to make it a bit better :)

for event in pygame.event.get():

    pressed_key = pygame.key.get_pressed()

    if event.type == pygame.QUIT:

        pygame.quit()

        sys.exit()

    if pressed_key[pygame.K_q]:

        pygame.quit()

        sys.exit()

can be shortened to:

for event in pygame.event.get():

    pressed_key = pygame.key.get_pressed()

    if event.type == pygame.QUIT or pressed_key[pygame.K_q]:

        pygame.quit()

        sys.exit()

This line of code is correct too but it is hard to read:

if (s.head[0] < 0) or (s.head[0] > bg_width-block_size) or (s.head[1] < 0)

            or (s.head[1] > bg_height-block_size):

But it will be far better to read if you will change it to:

if (s.head[0] < 0 or

        s.head[0] > bg_width-block_size or

        s.head[1] < 0 or

        s.head[1] > bg_height-block_size):

or even this (sometimes I use it for really long if's):

is_wall_collision = any([

    s.head[0] < 0,

    s.head[0] > bg_width-block_size,

    s.head[1] < 0,

    s.head[1] > bg_height-block_size

])

if is_wall_collision:

    size: int

...



    self.size = 2

You never read size, and Python Lists already know their own length in case you did need it.

You're just duplicating functionality that List already gives you by manually keeping size in sync with the list length.

Separate game logic from screen rendering details

You keep snake coordinates scaled by pixels. This seems to complicate your code everywhere with block_size instead of 1; I suspect it would be easier to keep snake coordinates in units of blocks, and only scale to pixels for drawing purposes.

e.g. then your outer-wall check could be something like s.head[0] >= bg_width instead of
s.head[0] > bg_width-block_size.

1 is special for integers because it's the difference between > and >=

I like your add_to_tail idea of putting multiple snake segments in the same place initially, so move on future turns actually makes the snake longer on screen. That high-level design might be worthy of a comment.

The other option would be a "future growth" counter that you check (and decrement) every move to decide whether to remove the last tail block. (That's what I was expecting because I hadn't thought of your idea). Your way is more efficient and will still work if you want food to cause multiple segments of growth. You can still do them all when the food is eaten.

Keeping a separate head: I'm not sure this is helping you. Functions that want to look at the first segment can do head = snake_.body[0].

I don't understand why move needs to do this:

       if self.body[0] != self.head:

            self.head = self.body[0]

when it looks to me like inserting the modified head as a new first element will already create that condition. So either this is redundant (and confusing), or else it's extra work that you wouldn't need to do if you didn't redundantly keep the head coordinates in 2 places. (Front of the List, and in its own object.)

    direction: str

This should be an enum or integer, not a string. String compares are fairly cheap, but not as cheap as integers. There's no reason that a direction needs to be an arbitrary string when it can take only 1 of 4 possible values. A string isn't making your code any easier to read.

You probably still want the if/elif chain to turn a direction into an x or y offset, but another option for an integer would be a lookup table of [-1, 0] , [1, 0] etc. so you look up the x and y offsets for the given direction and just add them to the head's x and y coordinates without any conditionals.

Or direction could actually be an XY vector, removing the lookup step.

    head[0] += direction[0]

    head[1] += direction[1]

But that might complicate change_dir. Or give you a different implementation: to check for reversing direction: if new_dir + old_dir == [0,0] then you tried to double back. Otherwise you can set dir = new_dir.

I like your design for having the keyboard-input just call change_dir to check the game-logic of that input. That works very cleanly, and would still work if left was defined as [-1, 0] instead of "LEFT".

Good going on your first game. I completely agree with most of the points made by others. Here are a few more nitpicks. This answer is a bit long-winded, but take it as a complement. :-)

Unpack the Coordinates

This is what lines 195-197 currently look like:

        for item in snake.get_body():

            pygame.draw.rect(window, snake.color, [item[0], item[1], block_size,

                                                   block_size])

Instead of relying on indexing, we can directly unpack item into its respective xy-coordinates. This makes for good brevity and readability.

        for x, y in snake.get_body():

            pygame.draw.rect(window, snake.color, [x, y, block_size, block_size])

The same can be done for lines 208-214.

# Before:

        food_pos = food.spawn()

        collision_ = collision(snake, *food_pos)   # 🤔

        if collision_ == 1:

            score += 1

            food.update(False)

        pygame.draw.rect(window, food.color, [food_pos[0], food_pos[1],  # 🤔

                                              block_size, block_size])





# After:

        food_x, food_y = food.spawn()

        # the rest is left as an exercise for the reader

On wall_collision(Snake)

Here's what the wall_collision function currently looks like (lines 137-141):

def wall_collision(s: Snake) -> bool:

    if (s.head[0] < 0) or (s.head[0] > bg_width-block_size) or (s.head[1] < 0)

            or (s.head[1] > bg_height-block_size):

        return True

    return False

1. Being a predicate (i.e. a function which returns a boolean), the function can be better named. You can always find a way to name a predicate such that it is prefixed with is or has. Instead of wall_collision, I'd go with is_colliding_with_wall or collides_with_wall to communicate the intention better.


2. It could be simplified. vurmux's answer suggests a few possible ways. However, there exists an easier (and more readable) way of writing it in one line:

    return not ((0 < s.head[0] < bg_width-block_size) and (0 < s.head[1] < bg_height-block_size))

^{I intentionally added the extraneous parentheses to group comparisons.}

This is made possible by Python's comparison "chaining" along with De Morgan's Laws. For fun, you can try to prove that the two snippets are equivalent.

Points and Coordinates

It took a while for me to figure out what the following line meant:

self.head = [int(10*block_size), int(5*block_size)]

I'm guessing this is a point/coordinate? Looks like it.

Lists are so prevalent in Python, that they can hold a multitude of meanings. What's more, they are variable-length constructs, which communicates something else: you might be changing the length of the object in the future. This might mean appending, erasing, or inserting into the list.

self.body uses a list appropriately, since it will be appended to and popped from in various places. On the other hand, not once does self.head use list.append, list.pop, or list.insert and this raises a question: does it need to be a list at all? It is better made a tuple, which is a fixed-length construct and immediately communicates to the reader that we won't be modifying the length of self.head.

I commend the usage of tuples in Food.spawn() for this very reason. Still, tuples can carry a multitude of different meanings. We can do better by using collections.namedtuple and creating a record-type Point for the purpose of representing xy-coordinates. This can greatly reduce ambiguity and improve readability.

from collections import namedtuple

Point = namedtuple('Point', ['x', 'y'])  # we now have a Point type  



class Snake:

    ...

    def __init__(self, color: Tuple[int, int, int]) -> None:

        self.head = Point(10*block_size, 5*block_size)  # foolproof: self.head is a point      

        self.body = [self.head, Point(...)]

    ...

    def add_to_tail(self) -> None:

        self.body.append(Point(self.body[-1][0], self.body[-1][1]))

        self.size += 1

The only pain with namedtuple is that we can't explicitly do assignments, which can be solved by making a new object:

def move(self) -> None:

    if self.direction == right:



# Before:

        self.head.x += block_size   # AttributeError: can't set attribute   



# After:

        self.head = Point(self.head.x + block_size, self.head.y)   # preferable

# or:

        self.head = self.head._replace(x=self.head.x + block_size) # looks hacky 

You can also use Point as a replacement for Food.position, which isn't being used.

class Food:

    ...

    position: Point



    def __init__(self, color: Tuple[int, int, int]) -> None:

        self.position = Point(x=random.randint(0, bg_width//block_size - 1)*block_size

                              y=random.randint(0, bg_width//block_size - 1)*block_size)



    def spawn(self) -> Point:  # more explicit

        if self.state:

            return self.position

        else:

            self.state = True

            self.position = Point(x=random.randint(0, bg_width//block_size - 1)*block_size

                                  y=random.randint(0, bg_width//block_size - 1)*block_size)    

            return self.position

I think this is especially useful when used together with Cordes's suggestion in his answer, in his Separate game logic from screen rendering details section.

Another good thing about namedtuple is that we can still do unpacking.

        for x, y in snake.get_body():    # still works

            pygame.draw.rect(window, snake.color, [x, y, block_size, block_size])

Refactor Random-Point Generation

There are two places where the coordinates of the food are updated with random integers, generated via a special formula. Based on the DRY principle, I suggest refactoring the generation of random points to a single function get_random_point().

def get_random_point():

    return Point(x=random.randint(0, bg_width//block_size - 1)*block_size

                 y=random.randint(0, bg_width//block_size - 1)*block_size)    

Then you can conveniently do self.position = get_random_point() without having to painfully, repetitively type out the above.

Refactoring Food

Currently, Food updates its position only when spawn() is called and if state is false. Seems a bit long-winded. Even the name update seems to be untruthful as it makes a delayed update.

I suggest updating the position immediately when update() is called.

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

        return self.position



    def update(self):

        self.position = get_random_point()

Note that Food.state and Food.spawn(self) are redundant now and can be removed. That should be three cheers (less lines of code, yes?).

On collision(Snake, int, int)

Logically, this section should come first, but I saved it for the last.

1. The name collision is ambiguous. Can we improve it? Sure! Note that logically speaking, the function returns a boolean, so it's a predicate. Can we prefix it with is? Certainly! I think is_snake_on_food isn't too bad. You could also go with is_food_reached or is_snake_colliding_with_food.


2. 
   

   Having changed how positions are stored in the Food class, we can pass in the food's position directly. Thus the signature of the function can be reduced to:

   
   
   

   def is_snake_on_food(snake: Snake, food_target: Point):
   
   

   
   
   

   This also saves us from needing unpack *food_pos in line 209.

   
   
   

   # Before:
   
   collision_ = collision(snake, *food_pos)
   
   
   
   # After:
   
   collision_ = is_snake_on_food(snake, food_pos)
   
   

   
   


3. 
   

   There is no need to create pygame.Rect just to compare snake_.head and food_target. Currently, lines 128-131 are

   
   
   

       snake_rect = pygame.Rect(*snake_.head, block_size, block_size)
   
       food_rect = pygame.Rect(food_target_x, food_target_y, block_size,
   
                               block_size)
   
       if snake_rect == food_rect:
   
   

   
   
   

   Instead, the coordinates can be compared directly:

   
   
   

       if (*snake_.head,) == (food_target_x, food_target_y):
   
   

   
   
   

   Having passed in food_target as a Point, we can simplify this to

   
   
   

       if snake_.head == food_target:
   
   

   
   

Hope this helps!

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.

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.

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.

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]

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.

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.

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