Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I have written this small game for the "learn python the hard way" book. I ask for some opinions about it and how to improve it.

I know about pygame and others game engines but all the game runs in the terminal. The text file orc.txt, cyclops.txt, etc are ASCII drawings

from PIL import Image
import numpy as np
import msvcrt
from time import sleep
from random import choice, randint

previous = 15 #start position tile

def createmap():
    The map is sketched in bmp, I make a array
    to use it like a tileset
    global bmp
    bmp ='sketch.bmp')
    bmp_data = bmp.getdata()
    bmp_array = np.array(bmp_data)
    global map
    map = bmp_array.reshape(bmp.size)

def array_to_text_map():
    """ Tileset
    # = Wall
    . = sand
    + = door
    e = enemy
    D = dragon
    $ = gold
    @ = the player

    numbers as due to the 16 color palette used
    in the sketch
    text_map = open('map.txt', 'w')
    for x in map:
        for y in x:
            if y == 0:
            elif y == 3:
            elif y == 8:
            elif y == 9:
            elif y == 10:
            elif y == 12:
            elif y == 13:
            elif y == 15:
                text_map.write(' ')

def print_text_map():
    text_map = open('map.txt')
    for x in range(bmp.size[1]):
        print(text_map.readline(), end='')

def move_player_by_key():
    wkey = msvcrt.getch
    if wkey() == b"\xe0":
        key2 = wkey()
        if key2 == b"H":
        elif key2 == b"M":
        elif key2 == b"K":
        elif key2 == b"P":

def position():
    ver = np.where(map == 13)[0][0]
    hor = np.where(map == 13)[1][0]
    return ver, hor

def move_in_matrix(direction):
    ver, hor = position()

        if direction == 'up':
            analize(ver-1 , hor)
        elif direction == 'right':
            analize(ver , hor+1)
        elif direction == 'left':
            analize(ver , hor-1)
        elif direction == 'down':
            analize(ver+1 , hor)
    except IndexError:
        console.append('Not that way')

def analize(verm, horm):
    """Take decision for each tile
    verm and horm = next position
    ver and hor = actual position
    previous = keeps tile number for later
    global previous
    ver, hor = position()

    def restore(ver, hor, previous):
        """Restore color of the leaved tile"""
        map[ver, hor] = previous

    if map[verm, horm] == 0:

    elif map[verm, horm] == 3:
        restore(ver, hor, previous)
        previous = map[verm, horm]
        map[verm, horm] = 13

    elif map[verm, horm] == 8:
        print('Open the door (yes/no)?', end='')
        answer = input(' ')
        if door(answer):
            restore(verm, horm, 15)
            console.append('The door was opened')
            console.append(doors_dict[(verm, horm)]) #show the description
                                                     #attached to the door

    elif map[verm, horm] == 9:
        print('Do you want to fight against the monster (yes/no)?', end='')
        answer = input(' ')
        result = fight(answer)
        if result and randint(0,1) == 1:
            restore(verm, horm, 12)
        elif result:
            restore(verm, horm, 15)

    elif map[verm, horm] == 10:
        print('the beast seems asleep, you want to wake her?(yes/no)?', end='')
        answer = input(' ')
        result = fight(answer, dragon=True)
        if result:

    elif map[verm, horm] == 12:
        print("You want to grab the items in the floor (yes/no)?", end='')
        if input(' ') == 'yes':
            restore(verm, horm, 15)

    elif map[verm, horm] == 15:
        restore(ver, hor, previous)
        previous = map[verm, horm]
        map[verm, horm] = 13

def door(answer):
    if answer == 'yes':
        return True
    elif answer == 'no':
        return False
        console.append('Invalid command')

def identify_doors():
    """Hardcode: Identify each door and zip() it with
    a description stored previously
    doors_array = np.where(map == 8)
    doors = zip(doors_array[0], doors_array[1])
    dict_doors = {}
    narrate = open('narrate.txt')
    for x in doors:
        dict_doors[x] = narrate.readline()
    return dict_doors

def fight(answer, dragon=False):
    if answer == 'yes':
        if dragon == False:
            monster = choice([Orc(), Goblin(), Cyclops()])
            monster = Dragon()

        print('Fighting against',

        while True:
            print(, 'is attacking you', end = '')

            answer = input('fight (1) or defend (2)? ')
            if answer == '1':
                player.defense = 1 + player.armor
       = - player.damage
            elif answer == '2':
                player.defense = 1.5 + player.armor
                print('Invalid command')

            print(, 'counterattack!!')
   = - (monster.damage / player.defense)

            print(, 'remaining',, 'life')
            print(, 'remaining Player life')

            if <= 0:
                print('The End')

            if <= 0:
                print('\n' * 5)
                print('Enemy defeated')
                print('You earn',, 'gold coins')
        return True

        return False

def moredamge(val):
    player.damage += val

def morearmor(val):
    player.armor += val

golds = {'Fire sword': (moredamge, 20),
            'Oak shield': (morearmor, 0.1),
            'Anhilation sword': (moredamge, 40),
            'Iron shield': (morearmor, 0.2),
            'Siege shield': (morearmor, 0.5)

def gold():
    bunch = randint(10, 200)
    print('You have found', bunch, 'gold coins') += bunch
    print('Is there anything else?')
    if randint(0, 10) > 7:
        obtained = choice(list(golds))
        print('You have get', obtained)
        golds[obtained][0](golds[obtained][1]) #access to key: (function, quantity)
        del golds[obtained]
        print('Oooohh.. nothing else')

def win():
    print('You have win the game')
    print('You get',, 'gold coins!!')

class Orc():
    def __init__(self): = 'Orc' = 100
        self.damage = 10
        self.defense = 1 = 100
        self.appearance = open('orc.txt')

    def show_appearance(self):

class Player(Orc):
    def __init__(self): = 1000
        self.damage = 20 = 0
        self.appearance = open('knight.txt')
        self.armor = 0

class Goblin(Orc):
    def __init__(self): = 'Goblin' = 50
        self.damage = 10 = 50
        self.appearance = open('goblin.txt')

class Dragon(Orc):
    def __init__(self): = 'Dragon' = 400
        self.damage = 40 = 2000
        self.appearance = open('dragon.txt')

class Cyclops(Orc):
    def __init__(self): = 'Cyclops' = 150
        self.damage = 20 = 120
        self.appearance = open('cyclops.txt')

def presentation():
    print('Welcome hero, are you ready for fight?')

if __name__ == "__main__":
    player = Player()

    console = []


    doors_dict = identify_doors()

    while True:
        if console:
            for x in console:
        console = []
share|improve this question
up vote 7 down vote accepted

I'll assume you're fairly new to Python; tell me if you feel this stuff is too basic, and I'll start criticizing you for things that you don't deserve to be criticized for.


Ok, so the first thing that made me go "eww" in the code was global. In general, whenever you use global, you're probably doing something wrong. If I was to write your createmap function, it might look something more like this:

def createmap():
    The map is sketched in bmp, I make a array
    to use it like a tileset
    bmp ='sketch.bmp')
    bmp_data = bmp.getdata()
    bmp_array = np.array(bmp_data)
    map = bmp_array.reshape(bmp.size)
    return bmp, map

Instead of using a global bmp and map variable, I'm returning the non-global bmp and map variables. This means that I have a lot more flexibility in calling the function:

# I can use it to create the old global variables:
global bmp, map
bmp, map = createmap()

# Or some local ones
local_bmp, local_map = createmap()

# But most importantly, I can use it to create multiple sets of them
bmp1, map1 = createmap()
bmp2, map2 = createmap()

The last case is the most important; using your version of createmap(), when I call createmap() for the second time, it overwrites the previous bmp and map variables. This might not seem like a big problem; why would you ever need more than one map? but it makes it much harder to test things when they change global variables.

We can do similar things to your other functions to avoid using global variables. The array_to_text_map function currently uses the map global variable, but we can replace that with a map argument. This way we will be able to convert any map to a text map, not just the one stored in the global map variable.

One downside to this is that it does require more typing; I have to explicitly pass around all of my state. However, one of the core tenets of Python is that explicit is better than implicit, so get used to it ;) I should mention that people often wrap up their program's commonly used variables into classes, though that's probably more complex than you are looking for right now.

Switch Statements

Another thing that makes me feel uncomfortable about the code is when you have long chains of elif statements:

if y == 0:
elif y == 3:
elif y == 8:
elif y == 9:
elif y == 10:
elif y == 12:
elif y == 13:
elif y == 15:
     text_map.write(' ')

This is ugly, and violates the DRY principle. What you are doing here is looking up a value, based on the value of the variable y. The Pythonic (and sane) way of doing this is to define a dictionary (mapping) from the values of y to the string that should be written to text_map:

    0: "#",
    3: ".",
    8: "+",
    9: "e",
    10: "D",
    12: "$",
    15: " "

We can then look up the correct character, and then write that to the file:

char = TEXT_MAP_CHARS[y]

Which is much neater, and doesn't mix up program data (the mapping from numbers to characters) and program logic (the writing of the character to a file based on the value of y).

With that said, I would praise you for your style. For every problem your code may have semantically, it is formatted very well (hooray for PEP8)

share|improve this answer

Your Answer


By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.