Wow, this one is definitely going to need some improvement.

So, just for fun, I decided to make a program where the player moves across a 2-dimensional ASCII-art map. If the player types something such as down, the on screen representation moves down and "loads" a bit more of the map.

from console import clear

class Move(object):
    up_down = 1
    right_left = 1

class Actions(object):
    action = {
        "down": 1,
        "up": -1,
        "left": -1,
        "right": 1

def set_move_values(action):
    if action == "down":
        Move.up_down += Actions.action["down"]
    elif action == "up":
        Move.up_down += Actions.action["up"]
    elif action == "left":
        Move.right_left += Actions.action["left"]
    elif action == "right":
        Move.right_left += Actions.action["right"]

def render_map(x, y):
    for _ in range(y):
        print(''.join(['.' for _ in range(x + 4)]))
    print(''.join(['.' for _ in range(x + 2)])), '@',
    print(''.join(['.' for _ in range(1)]))
    for _ in range(1):
        print(''.join(['.' for _ in range(x + 4)]))

def run_program():
    while True:
        render_map(Move.right_left, Move.up_down)
        set_move_values(raw_input('> '))

if __name__ == "__main__":

All I'm really looking for is, what general stuff can I improve? Also, if possible, I'd like something like right 5 to work to. Note: This is Python 2.7, I just prefer Python3's print function better. Also, console is an ios module, as I wrote this script on ios.


A few comments:

  • You already did a good job with PEP8. The only error I get back is that it expects two blank lines between functions

  • Regarding PEP257, please add some docstrings and comments to make clear what's the code goal to the potential reader

  • If find the Move class confusing. The up_down and left_right class attributes seem coordinates. I'd prefer to have a class named, for example, Player with x and y instance attributes. Note the differenct between class and instance attributes if you want to have multiple players in the future

  • The translation between actions and movements isn't very effective because you need to hardcode all four cases and use up/down/left/right multiple times

  • If iteration is all what is needed, I recommend using xrange instead of range

  • If just one iteration is needed (range(1)), just drop the loop for better readability

  • Use the multiplication operator to get long strings of the same charater instead of loops ('.' * n)

The code I'd have written would be more or less as follows (more comments below, console library not used):

"""ASCII world game."""

class Player(object):

    """Player object with some actions (just move for now)."""

    def __init__(self):
        """Initialize coordinates."""
        self.x = 1
        self.y = 1

    def move(self, dx, dy):
        """Move player to some position.

        :param dx: How much to move in the x axis
        :type dx: int
        :param dy: Move much to move in the y axis
        :type dy: int

        # Ignore invalid movements
        if (self.x + dx) < 1 or (self.y + dy) < 1:

        self.x += dx
        self.y += dy

class GameMap(object):

    """GameMap that shows where the player is."""

    X_OFFSET = 4

    def render(self, player):
        """Render map and player position."""
        def render_empty_row():
            """Render row in which there is no player."""
            print('.' * (player.x + self.X_OFFSET))

        for _ in xrange(player.y):

        print('{} @ {}'
              .format('.' * (player.x - 1),
                      '.' * (self.X_OFFSET / 2)))

class Command(object):

    """Command reader from stdin."""

    # Map text command to player movement
    CMD_TO_MOVE = {
        'u': (0, -1),
        'up': (0, -1),
        'd': (0, 1),
        'down': (0, 1),
        'l': (-1, 0),
        'left': (-1, 0),
        'r': (1, 0),
        'right': (1, 0),

    # Default value for unknown command
    NO_MOVE = (0, 0)

    def read(self):
        """Read command (up, down, left, right) from stdin.

        The command is converted to a movement for the player using the same
        coordinate system.

        :returns: Player movement
        :rtype: tuple(int, int)

        command_str = raw_input('> ')
        return self.CMD_TO_MOVE.get(command_str, self.NO_MOVE)

class Game(object):

    """Game object to keep state and read command for next turn."""

    def __init__(self):
        """Get all the objects needed to run the game."""
        self.player = Player()
        self.game_map = GameMap()
        self.command = Command()

    def run(self):
        """Render map and read next command from stdin."""
        while True:

if __name__ == "__main__":
    game = Game()

What I tried to show:

  • Object oriented design: use the language of the problem space to define a class for each type of object in the problem. In general, when you think about the problem, if it's a name it should be a class, if it's a verb it should be a method

  • Use docstrings for every class and method (use sphinx format for arguments and return values if possible)

  • Make a difference about what is command (word) and a movement (delta in the coordinate system)

  • Use constants instead of harcoded values (X_OFFSET)

Note that not everything should be a class. The command reader could be a function, but I preferred to use class in this case for consistency.

I hope this helps.


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.