Small "Grid Battle" console game

Question

This is my first attempt to make a game on my own. I just want to see if anyone has any comments. Am I doing things, at least, sort of alright? Is there a better way I could deal with the input?

import sys, platform, os
import time, random, math

class Entity(object):
    '''Basic entity object
        Has position and character'''
    def __init__(self, name, x, y, hp, char, state):
        self.name = name
        self.x = x
        self.y = y
        self.hp = hp
        self.char = char
        self.state = state

    def move(self, direction, board):
        '''To update entity position'''

        if direction == 'left' and board.is_valid_move(self, (self.x, self.y - 1)):
            self.y -= 1
        elif direction == 'right' and board.is_valid_move(self, (self.x, self.y + 1)):
            self.y += 1
        elif direction == 'up' and board.is_valid_move(self, (self.x - 1, self.y)):
            self.x -= 1
        elif direction == 'down' and board.is_valid_move(self, (self.x + 1, self.y)):
            self.x += 1

    def attack(self, target):
        target.hp -= 1

    def distance(self, target, pos=()):
        '''Get distance from self to target'''
        if not pos:
            x, y = self.x, self.y
        else:
            x, y, = pos
        return math.sqrt((x - target.x)**2 + (y - target.y)**2)

    def move_toward(self, target, board, pos=()):
        '''Moves entity towards target'''
        move_dict = {
        'right' : self.distance(target, (self.x, self.y + 1)),
        'left' : self.distance(target, (self.x, self.y - 1)),
        'up' : self.distance(target, (self.x - 1, self.y)),
        'down' : self.distance(target, (self.x + 1, self.y))
        }

        small_index = move_dict.values().index(min(move_dict.values()))
        small_key = move_dict.keys()[small_index]

        self.move(small_key, board)

    def change_state(self, state):
        '''Change Entity state. Only for death right now.'''
        self.state = state

    def update(self, board):
        '''Update Entity, check hp, xp, ect.'''
        if self.hp <= 0 and self.state == 'dead':
            time.sleep(1)
            pass
        elif self.hp <= 0 and self.state != 'dead':
            self.change_state('dead')
            self.char = '%'

class Board(object):
    '''Board class to create empty grid of cells'''
    def __init__(self, x_size, y_size, char='-'):
        self.x_size = x_size
        self.y_size = y_size
        self.char = char
        self.board = [[char for _ in range(x_size)] for _ in range(y_size)]

    def print_board(self, board, ents):
        '''To draw the board'''
        self.clear_screen()
        for ent in ents:
            self.print_char(ent, ent.char)
        for _ in self.board:
            print ' '.join(_)

    def print_char(self, ent, char='-'):
        '''Print a character to the board
            By default, prints empty cell'''
        self.board[ent.x][ent.y] = char

    def clear_screen(self):
        if platform.system() == 'Linux':
            os.system('clear')
        elif platform.system() == 'Windows':
            os.system('cls')
        else:
            print 'Clear screen is not working.'

    def is_vacant(self, x, y):
            if self.board[x][y] == '-':
                return True
            else:
                return False

    def is_valid_move(self, ent, future_pos):
        moves = []
        adj_cells = [(ent.x+1, ent.y),(ent.x-1, ent.y),
                    (ent.x, ent.y+1),(ent.x, ent.y-1)]
        for (x,y) in adj_cells:
            if not x >= self.x_size and not y >= self.y_size \
                    and not x <= self.x_size - self.x_size - 1 \
                    and not y <= self.y_size - self.y_size - 1 \
                    and self.is_vacant(x, y):
                moves.append((x,y))

        if future_pos in moves:
            return True
        else:
            return False

##
# Functions
##

def type_text(string, sec):
    for _ in string:
        if sec == 'random':
            sec = random.uniform(0.1, 0.2) if sec == 'random' else sec
        sys.stderr.write(_)
        time.sleep(sec)

def enemy_turn(enemy, player, board):

    distance = enemy.distance(player, (enemy.x, enemy.y))
    if distance == 1 and player.state != 'dead':
        enemy.attack(player)
    elif distance < 4 and player.state != 'dead':
        board.print_char(enemy)
        enemy.move_toward(player, board, (enemy.x, enemy.y))

    else:
        moves = ['up', 'down', 'left', 'right']
        move = random.choice(moves)  # Sweet. Oh yes, very sweet <3
        board.print_char(enemy)
        enemy.move(move, board)

def player_turn(player, board):
    player.update(board.board)

    if player.state == 'dead':
        print 'You are dead.'
        return

    m_actions = {
    'move' : ['move', 'walk', 'run', 'sprint'],
    'die' : ['die']
    } 

    s_actions = {
    'move' : ['up', 'down', 'left', 'right']
    }

    choice = raw_input('What do you want to do? :> ')
    parts = [x.strip() for x in choice.split(',')]
    if len(parts) < 2:
        if parts[0] in m_actions['move']:
            direction = raw_input('Which direction? :> ')
            parts.append(direction.strip())
            if parts[1] in s_actions['move']:
                board.print_char(player)
                player.move(parts[1], board)
        elif parts[0] in m_actions['die']:
            player.hp = 0
            board.print_char(player)
    elif len(parts) == 2:
        if parts[1] in s_actions['move']:
            board.print_char(player)
            player.move(parts[1], board)

def main():
    ## Title ##
    sys.stdout.write("\x1b]2;Grid_Battle V0.1\x07")

    # Initiate #
    board = Board(5, 5)
    player = Entity('Aet', 0, 0, 5, 'o', 'alive')
    enemy = Entity('Monster', 4, 4, 5, 'x', 'alive')
    objects = [player, enemy]

    board.clear_screen()
    type_text('Welcome', 'random')
    type_text('.', 3)
    board.print_board(board, objects)

    while True:
        if player.state == 'dead':
            print player.name, 'is dead.'
        print 'HP:', player.hp
        player_turn(player, board)
        board.print_board(board, objects)
        enemy_turn(enemy, player, board)
        board.print_board(board, objects)

if __name__ == '__main__':
    main()

Answer 1

What you have here looks good. It's clean and readable for the most part. Seems to work well, but I have some thoughts anyway.

In your Entity object, you have a state object for characters being alive or dead. Maybe you plan to extend this later, but right now it looks like it'd make more sense to have a value that's just self.alive = True. Then you can set it to False later, to indicate death. If you're doing this, there's no need to requite a parameter. You can just always set the initial value as True:

def __init__(self, name, x, y, hp, char):
    self.alive = True

If you do extend it, then I would firstly use constants as more reliable than strings:

class STATE:
    ALIVE = "alive"
    DEAD = "dead"

and I would still default it to being STATE.ALIVE, but you can use optional function paramaters to allow the user to supply it when they want, like this:

def __init__(self, name, x, y, hp, char, state=STATE.ALIVE):
    self.state = state

Why is your move described this way:

def move(self, direction, board):
    '''To update entity position'''

That indicates it's specific to an enemy, not a general Entity. Since the player is also an Entity this makes little sense. Instead, the Enemy class should inherit from Entity and add this Enemy specific function.

Also move doesn't in any way indicate if no movement occurs. Shouldn't you have an else case that somehow indicates movement was unnsuccesful? Eg. else: return False could then be used to check if movement actually occured. As it stands, if the enemy's move isn't valid it will just remain still.

In attack you hardcode 1 as the damage that target takes, but what if you want to add difficulty that will change this? You should make a strength attribute and use it here so that each individual Entity can have its own strength.

In distance you could shorten your if else with a ternary. A ternary's basically an expression that will resolve to one of two values based on a boolean evaluation. Here's how yours could look:

def distance(self, target, pos=()):
    '''Get distance from self to target'''

    x, y = pos if pos else (self.x, self.y)
    return math.sqrt((x - target.x)**2 + (y - target.y)**2)

If pos evaluates as True then it will use pos, if it's False then self.x, self.y are used.

move_toward is quite a confusing set up. I think you made it overly complicated by making it a dictionary. Instead, make a list of 2 value tuples. If you make the first value be the result of self.distance then you can just sort this list afterwards and the first (ie. lowest) value will be the one you move towards. Here's what I mean:

def move_toward(self, target, board, pos=()):
    '''Moves entity towards target'''
    possible_moves = (
        (self.distance(target, (self.x, self.y + 1)), 'right'),
        (self.distance(target, (self.x, self.y - 1)), 'left'),
        (self.distance(target, (self.x - 1, self.y)), 'up'),
        (self.distance(target, (self.x + 1, self.y)), 'down')
    )
    # Sort and take the first, ie. lowest, value.
    small_key = sorted(possible_moves)[0]

    self.move(small_key, board)

Whether you end up making state be a boolean, string or saved constant, there's little point in makign a function for this. It might be needed if you have complicated machinations in mind (eg. if an Entity can be poisoned but it's contextual and needs to be calculated). But at the moment it's just a simple attribute setting function. Python doesn't bother with those, let people set the value directly with player.state = "dead".

update doesn't need pass in your first if block, it doesn't do anything. Also, since you evaluate the same expression twice, consider nesting them instead:

def update(self, board):
    '''Update Entity, check hp, xp, ect.'''

    if self.hp <= 0:
        if self.state == 'dead':
            time.sleep(1)
        else:
            self.change_state('dead')
            self.char = '%'

In Board, using width and height would be easier to understand names than x_size and y_size. Also you can save some space in __init__ by making your inner list with multiplication:

    self.board = [[char] * x_size for _ in range(y_size)]

I'd also suggest having '-' be a class constant as you refer to it multiple times later.

You should rearrange how you print your board. For a start you could make a __str__ function, then you could just call print(Board()) and have it print properly to screen. To make that work, instead of printing in the function you'll need to build a string and return it. But that doesn't require much modification as I'll show below.

But also, you have the board characters being updated in their own separate function even though you're unlikely to ever need to do that separate to a print call. Your mileage may vary, but personally I'd put them together in one function.

There's no reason to have a board parameter, self.board will already access the characters you need. And you should have the player and enemy/ies already stored in Board in the same way. ie. self.entities = [player, enemy]. You could iniatilise this within __init__, just pass the player and enemy to Board and it can store a reference to them there, to call on for these updates.

With these changes, here's how I'd create your __str__ function:

def __str__(self):
    self.clear_screen()
    for entity in self.entities:
        self.board[entity.x][entity.y] = char
    return '\n'.join(' '.join(line) for line in self.board)

Both in is_vacant and at the end of is_valid_move you could just return the direct result of your boolean expressions:

def is_vacant(self, x, y):
    return self.board[x][y] == '-'

...

    return future_pos in moves

is_valid_move has a lot of conditions piled onto each other though. It's quite hard to read what's happening here. For a start x <= self.x_size - self.x_size - 1 is the same as x < 0, which is much more readable and comprehensible. It looks less like a formula now and more like a common sense limitation. Try to avoid using not if you can. Checking for truth is easier to understand than check for non falseness. Also, Python allows multiple comparisons at once, so you could use 0 <= x <= self.x_size which is even more readable. Now you could just rewrite it like this:

if (0 <= x <= self.x_size and 0 <= y <= self.y_size
    and self.is_vacant(x, y)):

Note that instead of using backslashes I used parentheses around the conditions, as that's the preferred style.

Avoid using for _ in when you actually do need the _ value. Underscore is used to denote a throwaway unnecessary variable, but you do need the value you're referring to as _ in cases like type_text. Also in type_text you accidentally double up on the if random check. Which is especially silly when you could put the ternary directly into the sleep call:

def type_text(string, sec):
    for _ in string:
        sys.stderr.write(_)
        time.sleep(random.uniform(0.1, 0.2) if sec == 'random' else sec)

Answer 2

Since Superbiasedman has already given detailed answer, I will just cover some high level data structures and design which might help you in future.

1. Namedtuple is a great data structure that could work here for X, Y coordinates.

2. Try to include constants instead of using "magic strings". Right, left, up, down, dead, alive are examples.

EDIT: Adding some of minor details that caught my eyes:

• Entity Class. I feel it would be better if there was also Player class and Enemy Class which inherit from Entity class. These classes could add the move method, or other special functions.

• Board Class. Generally, board class should be source of truth for everything that is on board. Which means that it knows the position of entities. For example if in future, right now you decide to extend multiple enemies, you would need to iterate over all entity objects. (This is not to say that entity objects should not keep track of their position... I am just saying that both should, board tracks everything, entity is just concerned with itself)

• Both classes are strongly coupled right now, with them passing and accepting objects of each other. Part of problem as I outlined above was design issue. For example, move method of Entity would not need to pass itself to Board class, if Board class already knows the location of characters. Instead of complete object, you would just require to pass coordinate tuple to validate the move.

• No else. I spotted this a couple of times. Anytime you write if-elif block without else, be very sure that you skip it. In your code function player_turn for example, you have no alternative for if user enters wrong direction. This is also evident couple of times more throughout code. Handling all branches is important throughout code, but definitely becomes one of top priority for any user input.

These design considerations along with superbiasedman technical review should help you.

If you need a specific input on something, please ask that, and I would update accordingly