5
\$\begingroup\$

This is an iterative review.
Previous Iteration


My snake now moves and grows and successfully kills itself.

Eventually, the game board will handle state and things like the list of fatal coordinates and telling a snake whether it's dead, but for now that's just a function called fatal_board_positions paired with is_fatal_position.
Fatal coordinates are (X = 1-10, Y = 7-10)

So, how pythonic is my snake? And how can I make it better?


Module utilities.py

(N.B. there are other things in the utiltiies module, but they're not used here so I left them out)

import numpy as np

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


def unit_vector_from_cardinal(cardinal: str) -> np.array:
    if cardinal in DIRECTION_VECTORS:
        return np.array(DIRECTION_VECTORS[cardinal])
    else:
        raise ValueError("An invalid cardinal direction was provided")

Module Snake.py

(code laid out in the order presented here, but split up for convenience)


Header

import numpy as np
from utilities import unit_vector_from_cardinal

'''
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:
    The board is responsible for handling state
    The snake is therefore ignorant of state and should not decide *how* to move
    The board should decide whether the snake:
        Grows (food)
        Moves (non-food)
        Is Dead

'''

Static Methods

def get_initial_body_coordinates(x_pos: int, y_pos: int, facing: str, length: int) -> list:
    first_coordinate = np.array([x_pos, y_pos])
    unit_vector = unit_vector_from_cardinal(facing)
    return [
        first_coordinate
        - unit_vector * i
        for i in range(0, length)
        ]


def is_fatal_position(position, *args):
    # *args should be (lists of numpy arrays) of fatal coordinates
    for pos_list in args:
        if any((position == pos).all() for pos in pos_list):
            return True

    return False


def fatal_board_positions() -> list:
    # Will eventually be handled by game board
    return [
        np.array((x, y))
        for x in range(1, 10 + 1)
        for y in range(7, 10 + 1)
    ]

Class Snake

class Snake:
    # An ordered list of X,Y coordinates representing the position of body segments
    # Grow = add new coordinate in relevant direction
    # Contract = remove last coordinate
    # move = (grow then contract)

    body_coordinates = list()

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

    def get_non_head_coordinates(self) -> list:
        return self.body_coordinates[1:]

    def get_head_pos(self):
        return self.body_coordinates[0]

    def add_new_pos(self, new_pos):
        self.body_coordinates = [new_pos] + self.body_coordinates

    def remove_last_pos(self):
        del self.body_coordinates[-1]

    def new_body(self, x_pos: int, y_pos: int, facing: str, length: int):
        self.body_coordinates = get_initial_body_coordinates(x_pos, y_pos, facing, length)

    def grow(self, direction: str):
        # Add a new coordinate to the head of the body list
        direction_vector = unit_vector_from_cardinal(direction)
        current_head_pos = self.get_head_pos()
        new_pos = current_head_pos + direction_vector
        self.add_new_pos(new_pos)

    def contract(self):
        self.remove_last_pos()

    def move(self, direction: str):
        self.grow(direction)
        self.contract()

Tests

def test_move(target_snake, direction):
    # Move in direction
    # Check for death
    # Print body coordinates
    target_snake.move(direction)
    print ('Move ' + direction)
    print ('Is Dead?')
    print(is_fatal_position(target_snake.get_head_pos(), fatal_board_positions(), target_snake.get_non_head_coordinates()))
    for i in target_snake.body_coordinates:
        print (i)


def test_hit_north():
    # continue north until death at Y = 7
    print(('='*13))
    print('= Hit north =')
    print(('='*13))
    new_snake = Snake(5, 5, 'n', 3)
    for i in new_snake.body_coordinates:
        print(i)
    test_move(new_snake, 'n')
    test_move(new_snake, 'n') # Should print Dead = True


def test_hit_self():
    print(('='*12))
    print('= Hit self =')
    print(('='*12))
    new_snake = Snake(5, 5, 'n', 5)
    for i in new_snake.body_coordinates:
        print(i)
    test_move(new_snake, 'e')
    test_move(new_snake, 's')
    test_move(new_snake, 'w') # Should print Dead = True

if __name__ == '__main__':
    test_hit_north()
    test_hit_self()

Current Output

=============
= Hit north =
=============
[5 5]
[5 4]
[5 3]
Move n
Is Dead?
False
[5 6]
[5 5]
[5 4]
Move n
Is Dead?
True
[5 7]
[5 6]
[5 5]
============
= Hit self =
============
[5 5]
[5 4]
[5 3]
[5 2]
[5 1]
Move e
Is Dead?
False
[6 5]
[5 5]
[5 4]
[5 3]
[5 2]
Move s
Is Dead?
False
[6 4]
[6 5]
[5 5]
[5 4]
[5 3]
Move w
Is Dead?
True
[5 4]
[6 4]
[6 5]
[5 5]
[5 4]
\$\endgroup\$
  • \$\begingroup\$ Missing code is missing. We don't have your utilities class which I assume is now in a separate file. (Broken as is without it) \$\endgroup\$ – Thomas Ward Oct 10 '16 at 16:45
  • \$\begingroup\$ @ThomasWard Ah yes. Thanks for pointing that out. Adding now. \$\endgroup\$ – Kaz Oct 10 '16 at 16:57
4
\$\begingroup\$

Glad to see iteration #2 of your code, and that you've used some of my suggestions! :)

Got some more for you though.


Docstrings

While you have comments explaining what different functions seem to do (or they are already fairly clear to someone who understands what's going on with the code), you should consider using docstrings in your code.


Style: PEP8 Guidelines

Just some minor things here.

  • There should not be any spaces between print and the ( that goes with it.
  • In line comments at the end of a line should have two spaces between the end of the code line, and the start of the comment (the #)

Snake class: Multiple suggestions

Make body_positions list a protected item in the class, add a Getter that works in this case.

This is more or less to protect against modification of the Snake positions and to use a Getter that returns the list of positions, but don't directly access the protected variable inside the class. (We have other 'getter' and 'setter' items too, so I'll touch base on that in a few minutes)

class Snake:
    ...

    _body_coordinates = list()

    ...

    @property
    def body_coordinates(self) -> list:
        return self._body_coordinates

    ...

Note that I also add a property called body_coordinates which basically does what you were doing before. (Python won't yell for accessing the protected property, necessarily, but we should use Getters to get data from the class's items unless we are directly manipulating that class's properties with a function or code outside of the class, which doesn't seem to be being done here. Accessing protected properties can cause undefined behavior at times, so it's better to be careful here and use Getters / Setters / Modifiers instead.)

'Getter' functions as properties

These are getters:

def get_non_head_coordinates(self) -> list:
    return self.body_coordinates[1:]

def get_head_pos(self):
    return self.body_coordinates[0]

The getter functions are simply returning information. We can handle them as properties instead of standard functions (thereby it's not 'callable', so we save some parentheses later on, and refer to them without parentheses later). We can also change their name by setting them as properties.

class Snake:

    ...

    @property
    def non_head_coordinates(self) -> list:
        return self._body_coordinates[1:]

    @property
    def head_pos(self):
        return self._body_coordinates[0]

    ...

Anywhere you had .get_* for getting info from the Snake class, you can replace it with just .head_pos and .non_head_coordinates. No need for parentheses since you're not really needing to worry about calling anything here as we have nothing more to do but getting the value from an internal item to the Snake class.

grow: Unnecessary variable creation inside class

You don't need to create a new variable - current_head_pos will just use a property of the class with the above recommendations, and since the variable is only being used inside of the grow function, we don't need to have a variable here. Just use the property in place, so you'll end up with this:

def grow(self, direction: str):
    # Add a new coordinate to the head of the body list
    direction_vector = unit_vector_from_cardinal(direction)
    new_pos = self.head_pos + direction_vector
    self.add_new_pos(new_pos)

There may be more suggestions to come, though this is the first set of my recommendations.

\$\endgroup\$
  • \$\begingroup\$ There's other stuff in the utilities module. But it's not being used here so I figured I'd leave it out. \$\endgroup\$ – Kaz Oct 10 '16 at 20:04
  • \$\begingroup\$ @Kaz updated to reflect your comment but you may want to make a note about that in your post lest we assume that utilities contains only the code you posted. \$\endgroup\$ – Thomas Ward Oct 10 '16 at 20:58
4
\$\begingroup\$

utilities.py

As a first modification, you could consider using EAFP rather than LBYL:

import numpy as np

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


def unit_vector_from_cardinal(cardinal: str) -> np.array:
    try:
        return np.array(DIRECTION_VECTORS[cardinal])
    except KeyError:
        raise ValueError("An invalid cardinal direction was provided")

You could also let the KeyError bubble up, as it already contains enough information.

But I would instead use an Enum here:

import enum
import numpy as np

class Direction(enum.Enum):
    NORTH = (0, 1)
    SOUTH = (0, -1)
    EAST = (1, 0)
    WEST = (-1, 0)

    @property
    def unit_vector(self):
        # Using a method as np.array can't be stored as enum values
        return np.array(self.value)

So that your facing or direction parameters would be of type Direction instead of str. And you could use direction_vector = direction.unit_vector or unit_vector = facing.unit_vector.

You would also change all your various calls to use Direction.NORTH instead of 'n' and so on.

\$\endgroup\$
  • \$\begingroup\$ Turns out, it doesn't like trying to put a numpy array in an enum. \$\endgroup\$ – Kaz Oct 11 '16 at 12:41
  • \$\begingroup\$ @Kaz My bad, it worked with tuples so I went ahead and put np.array there. You may still need an unit_vector_from_cardinal \$\endgroup\$ – Mathias Ettinger Oct 11 '16 at 14:01
3
\$\begingroup\$

I'm sure this is subjective, but the way you're constructing your snake seems a bit circular to me. You essentially have:

External method calls Snake(pass in x,y,direction,length)
Snake calls internal method new_body
Snake.new_body calls global/static method get_initial_body_coordinates

This is hard coding knowledge about the external environment into the Snake class. This means that if in the future you decide to move/rename the get_initial_body_coordinates method you'll have to update the Snake class as well. I'd consider passing the information instead into the Snake's constructor. You could pass the initial snake coordinates directly, or my preference would be to pass the get_initial_body_coordinates function into the constructor.

\$\endgroup\$
  • \$\begingroup\$ I didn't notice this when I did my review (thank you, my workplace, for taking up too much of my time), but this is a very good suggestion. \$\endgroup\$ – Thomas Ward Oct 11 '16 at 14:12

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.