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.head_segment = Segment(pygame.Rect(self.start_location, self.cell_size))
		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()
		self.update_snake_direction(self.head_segment, self.direction)
		self.move_snake(self.head_segment)
		if self.extender is None:
			self.add_extender_to_board()
		if self.head_segment_collided_with_extender():
			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)
			
	def add_extender_to_board(self):
		snake_segments_locations = set(self.get_all_snake_segment_locations(self.head_segment))
		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 head_segment_collided_with_extender(self):
		return self.head_segment.rect.colliderect(self.extender)
	
	def extend_snake(self):
		self.extender = None
		self.add_segment_to_snake(self.head_segment)
	
	def add_segment_to_snake(self, segment):
		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:
			self.add_segment_to_snake(segment.child)
			
	def game_over(self):
		return any([
			self.head_segment_collided_with_self(self.head_segment), 
			self.head_segment_out_of_bounds()
		])
		
	def head_segment_collided_with_self(self, segment):
		if segment.child is not None:
			if self.head_segment.rect.colliderect(segment.child.rect):
				return True
			else:
				return self.head_segment_collided_with_self(segment.child)
		return False
			
	def head_segment_out_of_bounds(self):
		if (self.head_segment.rect.x < 0 
			or self.head_segment.rect.y < 0 
			or self.head_segment.rect.x >= self.window_size[0] 
			or self.head_segment.rect.y >= self.window_size[1]):
				return True
		return False
		
	def refresh(self):
		self.head_segment = Segment(pygame.Rect(self.start_location, self.cell_size))
		self.direction = None
		self.extender = None
			
	def render(self):
		self.displaysurf.fill(self.black)
		self.draw_snake(self.head_segment)
		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()

test_game.py

inputmanager.py

game.py

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.

game.py

inputmanager.py

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).

Thanks so much for the reply, I'm going to review your comments and I'll get back with any additional questions.

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
#Added spaces
#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)
			
		if self.head_segment_collided_with_extender():
			self.extender = None
			self.add_segment_to_snake()
			
		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
				
	def update_snake_direction(self, head_direction):
		for index in reversed(range(len(self.snake))):
			self.snake[index].direction = self.snake[index-1].direction
		self.snake[0].direction = head_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)
		
	def head_segment_collided_with_extender(self):
		return self.snake[0].rect.colliderect(self.extender)
	
	def add_segment_to_snake(self):
		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 (self.head_segment_collided_with_self() 
				or self.head_segment_out_of_bounds())
		
	def head_segment_collided_with_self(self):
		return any([self.snake[0].rect.colliderect(segment) for segment in self.snake[1:]])
			
	def head_segment_out_of_bounds(self):
		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()

@Alberth

game.py

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.

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