2D console robot wars

Question

I'm working on a mini project to force me to learn OOP in Python and expand my range of tools. Essentially, I just want to code a basic console application where I will have a 2D 'terrain' where there will be inanimate objects (e.g. consumables, trees, rocks) and animate objects (either player controlled by keyboard input or AI-controlled).

Before I go too far with this, I'd like to have a code review, mostly on the application of OOP here and where it could lead me to future headaches the way I coded this.

Some more specific questions:

• Is it standard to have many functions with @property decorator ? It feels.. odd ?
• What are some good practices in exception handling in Python ? I thought of using exceptions to ensure that animate actions such as moving are valid (e.g. not moving out of bounds or moving over a blocking inanimate object such as a tree).
• Is it worth it to split my code into packages / modules at this point ? I have never really felt the need to do it so far because my code is usually < 150 lines.

Currently, this code creates a 10x10 terrain, assigns inanimate object to roughly 10% of free spaces, then places two animate objects on random free spaces and prints the resulting terrain.

# Robots

import random

class Position:
    def __init__(self, x, y):
        self._x = x
        self._y = y

    @property
    def x(self):
        return self._x

    @property
    def y(self):
        return self._y

class Inanimate:
    def __init__(self, walkable):
        self._walkable = walkable

    @property
    def walkable(self):
        return self._walkable

class AnimateManager:
    action_queue = []

    def get_animate_actions(self, terrain):
        for animate_elem in Animate.animate_list:
            action_queue.append((animate_elem, animate_elem.get_action(terrain)))

    def initialize_animates(self, terrain):
        free_placements = terrain.get_free_placements()
        if len(free_placements) < len(Animate.animate_list):
            raise Exception('NOT ENOUGH FREE PLACEMENTS')

        for animate_elem in Animate.animate_list:
            new_position = random.randint(0, len(free_placements))
            coordinates = free_placements[new_position]

            try:
                terrain.place_element(animate_elem, coordinates.x, coordinates.y)
                free_placements = free_placements[:max(new_position - 1, 0)] + free_placements[1 if new_position == 0 else new_position:]
            except Exception as e:
                print('Exception raised: ', e)

class Animate:
    animate_list = []
    new_animate_id = 0

    def __init__(self):
        self._name = 'BLAH'
        self._animate_id = Animate.get_new_animate_id()
        Animate.animate_list.append(self)

    def __del__(self):
        Animate.remove_animate(self._animate_id)

    @staticmethod
    def remove_animate(id):
        for i, x in enumerate(Animate.animate_list):
            if x.animate_id == id:
                Animate.animate_list = Animate.animate_list[:max(i-1, 0)] + Animate.animate_list[1 if i == 0 else i:]
                break

    @staticmethod
    def get_new_animate_id():
        Animate.new_animate_id += 1
        return Animate.new_animate_id - 1

    @property
    def animate_id(self):
        return self._animate_id

    def get_action(self, terrain):
        return 'HELLO ' + str(self.animate_id)

class Terrain:
    def __init__(self, width, height):
        self.generate_terrain(width, height)

    @property
    def width(self):
        return self._width

    @property
    def height(self):
        return self._height

    @property
    def terrain(self):
        return self._terrain

    def generate_terrain(self, width, height):
        inanimate_rate = 0.1
        self._width = width
        self._height = height
        self._terrain = [[] for x in range(self._height)]
        for i, line in enumerate(self._terrain):
            self._terrain[i] = [[Inanimate(False)] if ((random.randint(0, 100) + 1) < (100 * inanimate_rate)) else [] for x in range(self._width)]
        return self._terrain

    def place_element(self, element, location_x, location_y):
        if location_x >= self._width or location_y >= self._height:
            raise Exception('OUT OF BOUNDS')
            pass
        elif self._terrain[location_x][location_y] != []:
            raise Exception('OCCUPIED')
            pass
        else:
            self._terrain[location_x][location_y] = element

    def get_free_placements(self):
        free_placements = []
        for i in range(self._width):
            for j in range(self._height):
                if self._terrain[i][j] == []:
                    free_placements.append(Position(i, j))
        return free_placements

terrain = Terrain(10, 10)
a = Animate()
b = Animate()
a_mgr = AnimateManager()
a_mgr.initialize_animates(terrain)

print(terrain.terrain)

Answer 1

Nice to see some well-written code coming up for review, and someone taking the time to thing about a project before they dive in too deep!

Using @property to make 'read-only' classes is a good idea - it does make the class look 'bigger' but it makes the interface clear and understandable.

Note that if you have a property, e.g. width then it's a good idea to use it internally as well as externally for consistency, unless you have good reason not to. For example, when you do if location_x >= self._width: is fine now, but if the width property later does some validation, or modification of _width your code may start to behave unexpectedly.

Using exceptions for constraint handling is a good option too - though you don't need to pass after raising them. I'd also suggest creating your own specific exception types, e.g.:

class OccupiedException(Exception):
    """Square is already occupied."""

That way you don't ever do except Exception and catch things you didn't mean to.

Splitting is a good idea if the code gets too big, or if you want to only use a subset of a package somewhere (i.e. from x import y). pylint reckons 1000 lines is too many for a single file - but equally I wouldn't make 20 files, each with 5 lines in, as it makes following the code harder. At the point it seems to big, split it, but don't worry too much until it gets there.

A few other things:

for i, line in enumerate(self._terrain):
    self._terrain[i] = ...

You don't need to use enumerate here, since self._terrain[i] and line are the same thing. However also be aware that modifying a list while iterating over it is potentially dangerous - you are better off doing something like creating a new list as you iterate, and then doing an overwrite of the original when the loop is complete.

I've not dug down deeply into the logic behind the various methods, but I notice that Animate has 2 class atributes which are accessed and modified from within this class, and from AnimateManager:

animate_list = []
new_animate_id = 0

Just to flag up that this approach to keeping a global id register won't work if you want to do anything with threading later on - see https://stackoverflow.com/questions/1071935/i-need-a-python-class-that-keep-tracks-of-how-many-times-it-is-instantiated#1071939 for some ideas about handling this.

Minor tip, but you might want to consider using pprint instead of print for outputting the terrain while debugging, as it will give a better idea of what the grid looks like. Setting __repr__ on the classes will also help make it clearer - e.g. something like the following in the Animate class:

def __repr__(self):
    return "ANIMATE:{}".format(self._animate_id)

Let me know if any of that is unclear, or if you have any questions about anything I've suggested.

Answer 2

match has a very good rundown, and I would only like to add one thing. You have a Position class, which has two immutable numbers as its members. If you aren't going to need more complicated functionality (or even if you aren't sure, I'd go with YAGNI on this one), you can use a namedtuple:

from collections import namedtuple

Position = namedtuple('Position', ('x','y'))

p = Position(5, 3)

print(p.x) # 5
print(p.y) # 3
print(p) # Position(x=5, y=3)

q = Position(5, 3)
print(p == q) # True

p.x = 6 # AttributeError: can't set attribute

This reduces your Position class definition to a single line, reducing code clutter.