Public Group

Python code review for Snake

Recommended Posts

I wrote Snake in Python 3 using Pygame and was wondering if anyone can do a code review of it? If this is the appropriate fourm to post such a thing?

Some things to mention:

1. I realize I could have used a dict in the place of my Segment class, but I decided to go with the class because it looked more clean to me.

2. I used recursion heavily, though I could have used a list instead. I decided to do it recursively for practice and fun (I don't use recursion often).

3. I don't have doc strings for any of my functions.

4. I probably could have used my get_all_snake_segment_locations function to avoid recursion.

5. I set fps to 10 to limit the speed of the game. Is this a bad way to do such a thing?

6. I attached an input manager I created and unit tests for my game for completeness. Though, I'm only asking the actual game to be reviewed, if you want to look at those you can. Also, note the unit tests are not complete yet for several functions I changed.

7. I really appreciate anyone who takes the time to give me feedback of any kind. This fourm has been a huge help to me and I'm grateful for everyone's  insight!

import sys
import random
import itertools
import pygame
import inputmanager

class Segment:

def __init__(self, rect, direction=None, parent=None, child=None):
self.rect = rect
self.direction = direction
self.parent = parent
self.child = child

class Game:

def __init__(self):
pygame.init()
self.fps_clock = pygame.time.Clock()
self.fps = 10
self.window_size = (640, 480)
self.displaysurf = pygame.display.set_mode(self.window_size)
pygame.display.set_caption("Snake")
self.cell_size = (32, 32)
self.start_location = (320, 224)
self.up = "up"
self.down = "down"
self.left = "left"
self.right = "right"
self.black = (0, 0, 0)
self.green = (0, 255, 0)
self.red = (255, 0, 0)
self.direction = None
self.extender = None
self.cell_locations = set(
itertools.product(
range(0, self.window_size[0], self.cell_size[0]),
range(0, self.window_size[1], self.cell_size[1])
)
)

def main(self):
while True:
self.get_input()
self.update()
self.render()
self.fps_clock.tick(self.fps)

def get_input(self):
inputmanager.InputManager.get_events()
inputmanager.InputManager.check_for_quit_event()
inputmanager.InputManager.update_keyboard_key_state()
inputmanager.InputManager.get_keyboard_input()

def update(self):
self.handle_input()
if self.extender is None:
self.extend_snake()
if self.game_over():
self.refresh()

def handle_input(self):
if inputmanager.InputManager.quit:
self.terminate()
if (inputmanager.InputManager.keyboard[pygame.K_UP] == inputmanager.InputManager.pressed
and self.direction != self.down):
self.direction = self.up
elif (inputmanager.InputManager.keyboard[pygame.K_DOWN] == inputmanager.InputManager.pressed
and self.direction != self.up):
self.direction = self.down
elif (inputmanager.InputManager.keyboard[pygame.K_LEFT] == inputmanager.InputManager.pressed
and self.direction != self.right):
self.direction = self.left
elif (inputmanager.InputManager.keyboard[pygame.K_RIGHT] == inputmanager.InputManager.pressed
and self.direction != self.left):
self.direction = self.right

def terminate(self):
pygame.quit()
sys.exit()

def update_snake_direction(self, segment, parent_direction): ###TEST
if segment.child is not None:
self.update_snake_direction(segment.child, parent_direction)
if segment.parent is None:
segment.direction = parent_direction
else:
segment.direction = segment.parent.direction

def move_snake(self, segment):
self.move_segment(segment)
if segment.child is not None:
self.move_snake(segment.child)

def move_segment(self, segment):
if segment.direction == self.up:
segment.rect.move_ip(0, -self.cell_size[1])
elif segment.direction == self.down:
segment.rect.move_ip(0, self.cell_size[1])
elif segment.direction == self.left:
segment.rect.move_ip(-self.cell_size[0], 0)
elif segment.direction == self.right:
segment.rect.move_ip(self.cell_size[0], 0)

location = random.choice(list(self.cell_locations-snake_segments_locations))
self.extender = pygame.Rect(location, self.cell_size)

def get_all_snake_segment_locations(self, segment):
yield segment.rect.topleft
if segment.child is not None:
yield from self.get_all_snake_segment_locations(segment.child)

def extend_snake(self):
self.extender = None

if segment.child is None:
if segment.direction == self.up:
topleft = (segment.rect.x, segment.rect.y+self.cell_size[1])
elif segment.direction == self.down:
topleft = (segment.rect.x, segment.rect.y-self.cell_size[1])
elif segment.direction == self.left:
topleft = (segment.rect.x+self.cell_size[0], segment.rect.y)
elif segment.direction == self.right:
topleft = (segment.rect.x-self.cell_size[0], segment.rect.y)
segment.child = Segment(
pygame.Rect(topleft, self.cell_size),
segment.direction,
segment
)
else:

def game_over(self):
return any([
])

if segment.child is not None:
return True
else:
return False

return True
return False

def refresh(self):
self.direction = None
self.extender = None

def render(self):
self.displaysurf.fill(self.black)
if self.extender is not None:
self.draw_extender()
pygame.display.update()

def draw_snake(self, segment):
pygame.draw.rect(self.displaysurf, self.green, segment.rect)
if segment.child is not None:
self.draw_snake(segment.child)

def draw_extender(self):
pygame.draw.rect(self.displaysurf, self.red, self.extender)

if __name__ == "__main__":
game = Game()
game.main()

Edited by mrpeed

Share on other sites

Copied the files, and added comment, start with the input manager, as I refer back to that from the game comments. (see bottom of the post for the files).

16 hours ago, mrpeed said:

I realize I could have used a dict in the place of my Segment class

I agree, dicts look like cheap classes, but accessing fields looks messy. Also, there is no useful place to document what variables exist, and what they do.

16 hours ago, mrpeed said:

I decided to do it recursively for practice and fun

Ah, ok.

I agree that recursion is only useful if you have recursive data structures, so running into it is indeed dependent on the kind of problems that you usually solve. In this case, it's mostly just obfuscating intention, so better remove it.

16 hours ago, mrpeed said:

doc strings for any of my functions

Function names are pretty good, just a bit long-ish.

Documenting data and data flow is at least as important. Allowed values of all variables, and their meaning, and input/output relations of functions is crucial in avoiding problems.

As you seem to favor heavily fragmented function (a zillion functions of a few lines), some way to document the global picture would be useful too, as there is no single one page function that shows that.

I think it would be useful to try having less functions. You're almost using a function as a one line comment description above a block of lines now.

16 hours ago, mrpeed said:

I set fps to 10 to limit the speed of the game. Is this a bad way to do such a thing?

No idea, it does give you that authentic feeling doesn't it?

Some stuff to ponder is the problem of deciding which key the user pressed last. Right now the result depends on your order of processing, rather than me pressing keys in some order, I think (but didn't quite check).

You can of course also consider faster updating of the game screen, which makes things move more smooth. Maybe something for a next game?

16 hours ago, mrpeed said:

unit tests for my game for completeness. ...  if you want to look at those you can

I am a non-believer in unit test code for anything you can easily find out in the first use, so I'll pass on this one.

Edited by Alberth
point to files at the top

Share on other sites
7 hours ago, Alberth said:

Copied the files, and added comment, start with the input manager, as I refer back to that from the game comments. (see bottom of the post for the files).

Share on other sites

I did a second draft of the game.py file if you want to take a look. I plan to redo my input manager this week.

Some questions:

1. I use lots of functions because I find reading something like draw_segment() easier than reading pygame.draw.rect(self.displaysurf, GREEN, segment.rect), even if the function is one line. Also, I find that smaller functions are easy to write tests for which I often do. And if I need to make a change in the future, I would know exactly where a change needs to go. Do you think that approach is overkill for this program or in general?

2. Do you think putting all of my methods in the Game class for a game of this size is bad?

Again thanks for taking your time to look at my game! It has been super helpful.

#Removed refresh(), terminate(), extend_snake(), draw_extender(), draw_snake()
#Capitalized constants
#Modified some methods
#Now uses lists

import sys
import random
import itertools
import pygame
import inputmanager

FPS = 10
WINDOW_SIZE = (640, 480)
CELL_SIZE = (32, 32)
START_LOCATION = (320, 224)
UP = "up"
DOWN = "down"
LEFT = "left"
RIGHT = "right"
BLACK = (0, 0, 0)
GREEN = (0, 255, 0)
RED = (255, 0, 0)

class Segment:

def __init__(self, rect, direction=None):
self.rect = rect
self.direction = direction

class Game:

def __init__(self):
self.direction = None
self.extender = None
self.cell_locations = self.cell_locations = set(
itertools.product(
range(0, WINDOW_SIZE[0], CELL_SIZE[0]),
range(0, WINDOW_SIZE[1], CELL_SIZE[1])
)
)
self.snake = [Segment(pygame.Rect(START_LOCATION, CELL_SIZE))]

pygame.init()
self.fps_clock = pygame.time.Clock()
self.displaysurf = pygame.display.set_mode(WINDOW_SIZE)
pygame.display.set_caption("Snake")

def main(self):
while True:
self.get_input()
self.update()
self.render()
self.fps_clock.tick(FPS)

def get_input(self):
inputmanager.InputManager.get_events()
inputmanager.InputManager.check_for_quit_event()
inputmanager.InputManager.update_keyboard_key_state()
inputmanager.InputManager.get_keyboard_input()

def update(self):
self.handle_input()
self.update_snake_direction(self.direction)
self.move_snake()

if self.extender is None:
snake_segments_locations = set(segment.rect.topleft for segment in self.snake)
location = random.choice(list(self.cell_locations-snake_segments_locations)) ###Need to use set to subtract, and need to convert to list to use random.choice
self.extender = pygame.Rect(location, CELL_SIZE)

self.extender = None

if self.game_over():
self.snake = [Segment(pygame.Rect(START_LOCATION, CELL_SIZE))]
self.direction = None
self.extender = None

def handle_input(self):
if inputmanager.InputManager.quit:
pygame.quit()
sys.exit()

if (inputmanager.InputManager.keyboard[pygame.K_UP] == inputmanager.InputManager.pressed
and self.direction != DOWN):
self.direction = UP
elif (inputmanager.InputManager.keyboard[pygame.K_DOWN] == inputmanager.InputManager.pressed
and self.direction != UP):
self.direction = DOWN
elif (inputmanager.InputManager.keyboard[pygame.K_LEFT] == inputmanager.InputManager.pressed
and self.direction != RIGHT):
self.direction = LEFT
elif (inputmanager.InputManager.keyboard[pygame.K_RIGHT] == inputmanager.InputManager.pressed
and self.direction != LEFT):
self.direction = RIGHT

for index in reversed(range(len(self.snake))):
self.snake[index].direction = self.snake[index-1].direction

def move_snake(self):
for segment in self.snake:
self.move_segment(segment)

def move_segment(self, segment):
move_amount = {
UP: (0, -CELL_SIZE[1]),
DOWN: (0, CELL_SIZE[1]),
LEFT: (-CELL_SIZE[0], 0),
RIGHT: (CELL_SIZE[0], 0)
}.get(segment.direction, (0, 0))
segment.rect.move_ip(move_amount)

return self.snake[0].rect.colliderect(self.extender)

topleft = {
UP: (self.snake[-1].rect.x, self.snake[-1].rect.y+CELL_SIZE[1]),
DOWN: (self.snake[-1].rect.x, self.snake[-1].rect.y-CELL_SIZE[1]),
LEFT: (self.snake[-1].rect.x+CELL_SIZE[0], self.snake[-1].rect.y),
RIGHT: (self.snake[-1].rect.x-CELL_SIZE[0], self.snake[-1].rect.y)
}.get(self.snake[-1].direction, (0, 0))
self.snake.append(Segment(pygame.Rect(topleft, CELL_SIZE), self.snake[-1].direction))

def game_over(self):

return any([self.snake[0].rect.colliderect(segment) for segment in self.snake[1:]])

return (self.snake[0].rect.x < 0
or self.snake[0].rect.y < 0
or self.snake[0].rect.x >= WINDOW_SIZE[0]
or self.snake[0].rect.y >= WINDOW_SIZE[1])

def render(self):
self.displaysurf.fill(BLACK)
for segment in self.snake:
pygame.draw.rect(self.displaysurf, GREEN, segment.rect)
if self.extender is not None:
pygame.draw.rect(self.displaysurf, RED, self.extender)
pygame.display.update()

if __name__ == "__main__":
game = Game()
game.main()
Edited by mrpeed

Share on other sites
12 hours ago, mrpeed said:

. I use lots of functions because I find reading something like draw_segment() easier than reading pygame.draw.rect(self.displaysurf, GREEN, segment.rect), even if the function is one line.

Like I said, you're using functions as description of a single block of code, ie you could also do

# Draw segment
pygame.draw.rect(self.displaysurf, GREEN, segment.rect)

12 hours ago, mrpeed said:

Also, I find that smaller functions are easy to write tests for which I often do

If you need tests to write good code, how do you write good tests then?

12 hours ago, mrpeed said:

Do you think that approach is overkill for this program or in general?

I think in 5-10 years from now, when you read an old program of yourself, you'll find that having to jump a lot between functions before even understanding what is being done and before you understand how variables change, is very counter-productive.

Let's just wait 10 years to find out if I am right.

12 hours ago, mrpeed said:

2. Do you think putting all of my methods in the Game class for a game of this size is bad?

I think some of them belong to the input manager at least, but you didn't do that yet.

Other than that, there is no  "bad". If the code works, it's good, there are just many sutbly different variations of "good". The optimum is different for everybody, as well as for the goal and context of the code.

If this works for you, it's ok. However, by all means don't be afraid to experiment. Try a different approach some times. You might stumble on great discoveries and improve your coding.

The biggest omission to your code is documentation of the variables first, and documentation of functions second. It may feel like a waste of time (and at the time of writing it, it is), but you're saving yourself hours and hours of reverse-engineering existing code not to mention days of debugging, because you missed that a variable had a special value for some case. I started documenting lots of years ago, and found I got more productive, despite spending time writing additional text.

Share on other sites
On 10/22/2017 at 11:23 AM, Alberth said:

I agree, dicts look like cheap classes, but accessing fields looks messy. Also, there is no useful place to document what variables exist, and what they do.

Objects in Python are actually implemented like "dictionaries"

Edited by matt77hias

Share on other sites

I am aware of that, and of course that makes fully sense.

What I intended to say was that

something.field

looks a lot better and reads nicer than the forced stringy keys, as in

somedict["field"]

Constant keys don't quite fit with dictionaries

1. 1
2. 2
frob
15
3. 3
Rutin
12
4. 4
5. 5

• 13
• 12
• 58
• 14
• 15
• Forum Statistics

• Total Topics
632123
• Total Posts
3004235

×