7
\$\begingroup\$

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()
\$\endgroup\$
  • 4
    \$\begingroup\$ Welcome to Code Review! I have rolled back the last edit. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Vogel612 Nov 12 '15 at 22:41
  • 2
    \$\begingroup\$ Please, do not edit the received recommendations into your original question. The site regulars are only trying to help here, and I'd rather not have to lock your post. \$\endgroup\$ – Mathieu Guindon Nov 12 '15 at 22:58
  • \$\begingroup\$ I didn't edit the code. All I did was post a link to the edited code, which it said I could do!! \$\endgroup\$ – acollection_ Nov 12 '15 at 23:20
5
\$\begingroup\$

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)
\$\endgroup\$
  • \$\begingroup\$ Thank you! There were a few things I couldn't figure out. One, the distance method. I couldn't get the ternary to work. I keep getting a TypeError: TypeError: unsupported operand type(s) for -: 'tuple' and 'int'. And two, the is_vacant method. You said I should return the direct results but I wasn't sure what you meant. The code has been updated, if you want to look over it. Thank you, again. \$\endgroup\$ – acollection_ Nov 12 '15 at 22:43
  • \$\begingroup\$ Since apparently I can't add a link to the new code in my question body, here is the edited code: pastebin.com/EzxCbaB6 \$\endgroup\$ – acollection_ Nov 12 '15 at 23:23
  • \$\begingroup\$ @acollection_ Ah, that's my bad about the ternary! You need to wrap it in brackets, otherwise it gets read as x, y = (pos if pos else self.x), self.y, so y is always set to self.y and x is either pos or self.x. I'll fix it in my post. With is_vacant I meant you can return future_pos in moves. Instead of having to use something like if future_pos in moves: return True else: return False. \$\endgroup\$ – SuperBiasedMan Nov 12 '15 at 23:39
  • \$\begingroup\$ Thank you! Another thing. The UI (Like, player health) used to be drawn on the bottom. But with the new str method, I can't seem to do that? Any ideas on how to get the UI to draw after the board has been drawn? \$\endgroup\$ – acollection_ Nov 12 '15 at 23:41
  • \$\begingroup\$ It works like a charm now! \$\endgroup\$ – acollection_ Nov 12 '15 at 23:49
4
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ Please do add those "other minor concerns", as it will help the OP getting evaluate different options and arrive at a better solution as soon as possible. It also allows the community to aid in approving/discussing those enhancments through the voting system. \$\endgroup\$ – holroy Nov 10 '15 at 18:15
  • \$\begingroup\$ Thank you for your feedback, and going into further detail! First off, I'm not sure how to use named tuples to my advantage here. I read about them but I'm still unaware of what advantages I will have over using normal tuples. Second, I have constants now, and I love it! It looks much better. I've made separate classes for the player and enemies but I'm not the best at inheritance so there hasn't been much of a change. Third, the board class. I understand that there's a lot of interplay with both the entity and board classes but I'm not sure how to fix that. \$\endgroup\$ – acollection_ Nov 12 '15 at 22:46
  • \$\begingroup\$ Ran out of room. Any more input on how I can separate those two classes a little more? That's the main thing I'm curious about. I'd love to have cleaner code. And last, I put elses on every if that I could. There was actually a bug when I put one. It was in the enemies move_toward function. It wasn't filtering out the bad moves. That took forever to find, haha. Thank you, again! \$\endgroup\$ – acollection_ Nov 12 '15 at 22:48
  • \$\begingroup\$ The edited code is in Superbiasedman's answer comments. \$\endgroup\$ – acollection_ Nov 12 '15 at 23:24
  • \$\begingroup\$ Okay, first of all the standard way of posting improved code is posting it in new post: Titled "Follow up: <rest of title>", include link to previous question in body, that way I can comment and post in detail on that too :) Quick answers though: you are right you are still not making bets use of inheritance, I would post detailed example in new post of yours.... As for decoupling classes, see my next comment. Leave namedtuples for now, they won't offer much improvement in this use-case, and maybe none at all when you are done with all other refactoring :) \$\endgroup\$ – kushj Nov 13 '15 at 3:34

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.