Python! (Snake)

Question

This is now an Iterative Review.
Next Iteration

---

Nowhere near a full game yet. Just a basic overview and initialisation of a Snake() class.

Thoughts?

---

Snake.py

import numpy
'''
Game Board:
 X by Y array of numbers
 Derived from Snake Object and base parameters (height, width, obstacles?)
Board Values:
 0 - Empty
 1 - Snake Head
 2 - Snake Body
Snake:
 Ordered list of X,Y coordinates for each body segment
Actions:
 Snake.Move:
 Append New Coordinate to list
 Move into Food:
 Do Nothing. List is now 1 segment longer.
 Move Into Non-Food:
 Remove last Coordinate From List
 Check for Dead Snake:
 Dead == New Coordinate is not unique in list.
'''
def unit_vector_from_cardinal(cardinal: str) -> numpy.array:
 if cardinal in ('n', 's', 'e', 'w'):
 if cardinal == 'n':
 unit_vector = [0, 1]
 elif cardinal == 's':
 unit_vector = [0, -1]
 elif cardinal == 'e':
 unit_vector = [1, 0]
 elif cardinal == 'w':
 unit_vector = [-1, 0]
 return numpy.array(unit_vector)
 else:
 raise ValueError
class Snake():
 # An ordered list of X,Y coordinates representing the position of body segments
 # Method to update said list depending on which direction it is now moving in
 # Method to delete the last coordinate in the list
 body_coordinates_list = list()
 def new_body(self, x_pos: int, y_pos:int, facing: int):
 self.body_coordinates_list = self.get_initial_coordinates_list(x_pos, y_pos, facing)
 def get_initial_coordinates_list(self, x_pos: int, y_pos: int, facing: str) -> list:
 first_coordinate = numpy.array([x_pos, y_pos])
 unit_vector = unit_vector_from_cardinal(facing)
 return [first_coordinate - unit_vector * i for i in range(0, 2 + 1)]
if __name__ == '__main__':
 new_snake = Snake()
 new_snake.new_body(5, 5, 'n')
 print(new_snake.body_coordinates_list[0])
 print(new_snake.body_coordinates_list[1])
 print(new_snake.body_coordinates_list[2])

Current Output

[5 5]
[5 4]
[5 3]

• Caridorc: I just wrote a full snake game, you may be interested in looking at it: codereview.stackexchange.com/questions/143470/… Also searching for [pygame] [snake-game] will yield interesting results.
• Thomas Ward: Just an FYI to the world, numpy doesn't exist on all Python 3 installations, so you may need to install it to make this code work.

Answer 1

I'm not really the best at reviewing code (I have my own failures in coding too), but I have a few things that I'd like to point out and share.

---

Consider setting up the new Snake object with an __init__ instead (unless you intend to reuse the same Snake object just to create a 'new' snake)

You may wish to just move the creation of the Snake body object to an __init__ for the class like so, rather than having to provide a new_body method:

class Snake:
 # An ordered list of X,Y coordinates representing the position of body segments
 # Method to update said list depending on which direction it is now moving in
 # Method to delete the last coordinate in the list
 body_coordinates_list = list()
 def __init__(self, x_pos: int, y_pos: int, facing: str):
 self.body_coordinates_list = self.get_initial_coordinates_list(x_pos, y_pos, str(facing))
 ...

You can then create the new snake with:

new_snake = Snake(5, 5, 'n')

---

Use a dictionary to handle the unit vectors and cardinal directions

Your current implementation of unit_vector_from_cardinal relies on a statically provided list for analysis, and then multiple if statements to determine what vector to provide based on that list.
I'd consider using a dictionary for the cardinal directions vector, like so:

DIRECTION_VECTORS = {
 'n': [0, 1],
 's': [0, -1],
 'e': [1, 0],
 'w': [-1, 0]
 }
def unit_vector_from_cardinal(cardinal: str) -> numpy.array:
 if cardinal in DIRECTION_VECTORS:
 return numpy.array(DIRECTION_VECTORS[cardinal])
 else:
 raise ValueError

This reduces the amount of logical checks, because you won't have to have a large set of if, elif, etc. checks, and prevents you from having to create a variable that is set based on the specific direction (one less variable to have to init in memory).

You could also use a dict that contains tuples instead of lists, and get the same result, as suggested in comments by Graipher, like so:

DIRECTION_VECTORS = {
 'n': (0, 1),
 's': (0, -1),
 'e': (1, 0),
 'w': (-1, 0)
 }

---

Unexpected data types passed to new_body, which in turn passes unexpected types to other functions and/or methods later on

(NOTE: If you choose to add an __init__ to the class, then this applies to that __init__)

Your new_body method should be looked at to make sure you're providing correct data types. Currently, it expects three integers when passed to it, but it seems you're using a string instead when called:

new_snake.new_body(5, 5, 'n')

Since you are expecting it to be a string in the third item (called facing in the function), consider changing the expected variable type for the third item to a str instead, which is what you're providing to it when calling it elsewhere in the code. We also need to do this to make sure we can work with strings rather than integers like you are attempting to do in unit_vector_from_cardinal when passing facing to it:

def new_body(self, x_pos: int, y_pos:int, facing: str):
 ...

---

get_initial_coordinates_list may be a static method

A static method only relies on the arguments passed to it. Your get_initial_coordinates_list function does not directly access any of the properties or variables assigned directly to the class itself (such as body_coordinates_list), and only uses the arguments and non-class-specific methods/functions, so we can actually make it a static method, and not have to provide a self argument (which means it doesn't need to have access to everything else in the class). At least, in its current form, it can be made into a static method:

@staticmethod
def get_initial_coordinates_list(x_pos: int, y_pos: int, facing: str) -> list:
 ...

---

Optional: Consider a better ValueError message

Looking at the code for unit_vector_from_cardinal, you could have a case where you trigger a ValueError. While this is okay, the error message is empty and potentially not super useful for debugging. Whether you decide to put a custom error message or not is up to you, though I would rather have a more informative reason for why the ValueError was raised, than have to be guessing:

raise ValueError("An invalid cardinal direction was provided.")

This way, when the program dies off, it can be a little more useful of an error message (for someone to debug).

---

Style: Empty parentheses on Snake() are redundant

This isn't really against PEP8 style, this just irks me a little. At some point you may wish to provide arguments to a new Snake() instance, but if not, you don't need the parentheses. (And even if you did wish to pass arguments when creating a new Snake, that should be handled in the __init__ function which doesn't exist here):

class Snake:
 ....

• Mathieu Guindon: We all make mistakes and have our own failures - that doesn't mean we can't make other people's code better! Very nice first answer, keep 'em coming!
• Thomas Ward: @Mat'sMug Thanks for the kind words! I've gotten a bunch of help before, here, so I'm hoping to be able to provide some suggestions every now and again :)
• Thomas Ward: For those who care, or don't know what I mean when I refer to PEP8, I mean this.
• Graipher: When you recommend a dict for the cardinal directions (which is a very good recommendation), if cardinal in DIRECTION_VECTORS is the same (and in python 2 even better) as if cardinal in DIRECTION_VECTORS.keys(). I would also use tuples instead of list in the dict itself (because they won't change and tuples are slightly more memory efficient).
• Thomas Ward: @Graipher I've incorporated your suggestion as a "you could also..." in that section of my review, after testing that revision and confirming it returns the same results. Thanks for the suggestion. I am picky enough to use .keys() as a personal preference, but you're right either works.