I'm wondering whether anybody would be willing to review this code for a game I'm working on with my friends. I feel quite confident with Python but am wondering whether there are some places that it could be made more efficient or errors I haven't spotted.

# Copyright 2015 Wet Ferret Studios

# Initialize Player Stats

playerstats = {
'basehealth': 100,
'name': 'Gerald Lagley',
'inventory': {

},
'damage': 4,
'equipped': {
'weapon': {

},
'ring': {
'wedding ring': {
'name': 'Wedding Ring',
'value': 20,
'equippable': True
}
},
'amulet': {

},
'armor': {

},
'armorclass': 0,
'extrahealth': 0,
'negativehealth': 0,
'negativedamage': 0,
'negativearmor': 0,
'totalhealth': 0,
},
}

# Initialize Rooms
roomnumberint = 0
roomnumber = str(roomnumberint)
room = {
'0': {
'name': 'Temple Atrium',
'items': {
'noitems': False,
'wooden stick': {
'name': 'Wooden Stick',
'desc': 'A wooden stick, could be useful',
'value': 0,
'equippable': True,
'damagemult': 12,
'type': 'weapon'
},
},
'desc': 'Insert desc here',
'entryinfo': 'Insert room desc here',
'directions': {
'n': True,
's': False,
'e': False,
'w': False,
'u': False,
'd': False,
'directions': ['N', 'S', 'E', 'W', 'U', 'D']
}
},
'1000': {
'name': 'Temple Antechamber',
'items': {
'noitems': True,
},
'desc': 'desc',
'entryinfo': 'entryinfo',
'directions': {
'n': True,
's': True,
'e': True,
'w': True,
'u': False,
'd': False,
'directions': ['N', 'S', 'E', 'W', 'U', 'D']
},

},
'2000': {
'name': 'Temple Hallway',
'items': {

}
}

}
roomtrapped = False
roomnoitems = room[roomnumber]['items']['noitems']
currentlyequipped = {
'currentweapon': '',
'currentring': '',
'currentamulet': '',
'currentarmor': '',
}

# Initialize Commands
def movefunc(direction):
global roomnumberint
global roomnumber
if direction.lower() == 'n' and room[roomnumber]['directions'][direction.lower()] == True:
roomnumberint += 1000
roomnumber = str(roomnumberint)
elif direction.lower() == 's' and room[roomnumber]['directions'][direction.lower()]:
roomnumberint -= 1000
roomnumber = str(roomnumberint)
elif direction.lower() == 'e' and room[roomnumber]['directions'][direction.lower()]:
roomnumberint += 10
roomnumber = str(roomnumberint)
elif direction.lower() == 'w' and room[roomnumber]['directions'][direction.lower()]:
roomnumberint -= 10
roomnumber = str(roomnumberint)
elif direction.lower() == 'u' and room[roomnumber]['directions'][direction.lower()]:
roomnumberint += 100000
roomnumber = str(roomnumberint)
elif direction.lower() == 'd' and room[roomnumber]['directions'][direction.lower()]:
roomnumberint -= 100000
roomnumber = str(roomnumberint)
else:

def roomproperties():
global roomnumber
global roomnumberint
global roomnoitems
if room[roomnumber]['items']['noitems'] is True:
roomnoitems = True
else:
roomnoitems = False

def examinefunc(item):
global roomnumberint
global roomnumber
global roomnoitems
if roomnoitems is False:
print('Name: ' + room[roomnumber]['items'][item]['name'])
print('Description: ' + room[roomnumber]['items'][item]['desc'])
print('Value: ', end='')
print(room[roomnumber]['items'][item]['value'], end='')
print(' gold')
elif roomnoitems is True:
print('You look around the room but can\'t find a single item')
else:
assert isinstance(item, str)
print('You look but fail to find ' + item)

def lookfunc():
global roomnumber
global roomnumberint
global roomnoitems
global room
roomnumber = str(roomnumberint)
print(room[roomnumber]['name'])
print(room[roomnumber]['desc'])
if roomnoitems is True:
print('There are no items in this room')
elif roomnoitems is False:
print('Items you can see: ')
for item in room:
print('Name: ' + room[roomnumber]['items'][item]['name'])
print('Description: ' + room[roomnumber]['items'][item]['desc'])
print('Value: ', end='')
print(room[roomnumber]['items'][item]['value'], end='')
print(' gold')
else:
print('Error')

def userinput(func):
global roomnumber
global room
if func == 'examine':
examinefunc(input("What would you like to examine"))
elif func == 'look':
lookfunc()
elif func == 'move':
print('Valid Directions are: ')
for items in room[roomnumber]['directions']['directions']:
print(items + ': ', end='')
print(room[roomnumber]['directions'][items.lower()])
movefunc(input('Which direction would you like to go: '))
else:

def pickupfunc(item):
global room
global roomnumber
global playerstats
if item in room[roomnumber]['items']:
if input('Do you want to pick up ' + item).lower() == 'yes' or 'y':
playerstats['inventory'].append(room[roomnumber]['items'][item])
room[roomnumber]['items'].remove(item)
else:
print('You look all around but utterly fail to find' + item)

def equipfunc(item):
global room
global playerstats
global roomnumber
global currentlyequipped
if item in playerstats['inventory']:
if playerstats['inventory'][item.lower()]['equippable']:
if playerstats['inventory'][item.lower()]['type'] == 'weapon':
for keys in playerstats['equipped']['weapon']:
playerstats['inventory'].append(playerstats['equipped']['weapon'][keys])
playerstats['equipped']['weapon'].clear()
playerstats['equipped']['weapon'].append(playerstats['inventory'][item.lower()])
playerstats['inventory'].remove(item.lower())
currentlyequipped['currentweapon'] = item.lower()
print(playerstats['equipped']['weapon'][item.lower()]['name'] + ' has been equipped to your weapon slot')
elif playerstats['inventory'][item.lower()]['type'] == 'ring':
for keys in playerstats['equipped']['ring']:
playerstats['inventory'].append(playerstats['equipped']['ring'][keys])
playerstats['equipped']['ring'].clear()
playerstats['equipped']['ring'].append(playerstats['inventory'][item.lower()])
playerstats['inventory'].remove(item.lower())
currentlyequipped['currentring'] = item.lower()
print(playerstats['equipped']['ring'][item.lower()]['name'] + ' has been equipped to your ring slot')
elif playerstats['inventory'][item.lower()]['type'] == 'amulet':
for keys in playerstats['equipped']['amulet']:
playerstats['inventory'].append(playerstats['equipped']['amulet'][keys])
playerstats['equipped']['amulet'].clear()
playerstats['equipped']['amulet'].append(playerstats['inventory'][item.lower()])
playerstats['inventory'].remove(item.lower())
currentlyequipped['currentamulet'] = item.lower()
print(playerstats['equipped']['amulet'][item.lower()]['name'] + ' has been equipped to your amulet slot')
elif playerstats['inventory'][item.lower()]['type'] == 'armor':
for keys in playerstats['equipped']['armor']:
playerstats['inventory'].append(playerstats['equipped']['armor'][keys])
playerstats['equipped']['armor'].clear()
playerstats['equpped']['armor'].append(playerstats['inventory'][item.lower()])
playerstats['inventory'].remove(item.lower())
currentlyequipped['currentarmor'] = item.lower()
print(playerstats['equipped']['armor'][item.lower()]['name'] + ' has been equipped to your armor slot')
else:
# Creating variable for failure notice as it exceeds character limit
failure1 = "You rummage through the seemingly endless depths of your rucksack but fail to find"
print(failure1 + item.lower)

def statscalc():
global playerstats
global currentlyequipped
playerstats['damagemult'] = playerstats['equipped']['weapon'][currentlyequipped['currentweapon']]['damagemult'] + \
playerstats['basedamage'] - playerstats['negativedamage']
playerstats['armorclass'] = playerstats['equipped']['armor'][currentlyequipped['currentarmor']]['armorvalue'] - \
playerstats['negativearmor']
playerstats['totalhealth'] = playerstats['basehealth'] - playerstats['negativehealth'] + playerstats['extrahealth']

• Welcome to Code Review! Is there anything in particular you'd like optimised or are you looking for general feedback? – SuperBiasedMan Oct 13 '15 at 8:31
• Just general feedback – MicroTransactionsMatterToo Oct 13 '15 at 9:05
• Have you tried profiling your code to find bottlenecks? Bottlenecks are the places where you'd need to focus your optimization efforts, so it's a good place to start. – code_dredd Oct 13 '15 at 11:00
• The title of a question should be the purpose or function of the code, not what help you'd like. Aside from that, good question, and welcome! – Fund Monica's Lawsuit Oct 13 '15 at 12:09
• The desire to improve code is implied for all questions on this site. Question titles should reflect the purpose of the code, not how you wish to have it reworked. See How to Ask. – Jamal Oct 20 '15 at 3:24

### Get yourself acquainted with Object-Oriented Programming.

Why?

It will remove constructions like:

def movefunc(direction):
global roomnumberint
global roomnumber


Which doesn't do what you think it does. If you'd make a Player class, that class could have a move function which is only allowed to move the current Player.

class Player:
def __init__(self, room_number, stats):
self.room_number = room_number
self.stats = stats
def move(self):
# rewrite movefunc so it fits here


Where in the main of your program, you could create a Player like:

main_character = Player(0, {
'basehealth': 100
})


Something along those lines. Note that snake_case is the preferred method of writing function names in Python.

The moment you see so many global, you know your code won't survive in the long run. It's not maintainable and not extensible. Classes are your friend.

So what's the deal with global?

global is required if you want to modify a global variable inside a function's scope. You can read a variable without that keyword, but modifying it without one only modifies the local version of that variable. Basically, you're potentially shadowing variables with multiple versions of themselves depending on what local scope they're currently in.

This means you'll have to declare your variable is global in every function where you modify it. This could have it's cases in variables which aren't changed often, but I hope we can agree a room_number is a variable which is supposed to change. Keeping up your current method of writing will lead to big lists of variables at the top of each function, which is among other things considered needless repetition. Simplified your code looks like this and it will only get worse:

def foo():
global w
global x
global y
global z

def bar():
global w
global x
global y
global z

def faz():
global w
global x
global y
global z


Nobody wants their code to look like that. Re-factoring it now will prevent you from having to re-factor it when it looks worse.

• Thanks. I had read many places that global variables were a bad idea but I couldn't think of any other way to do it. – MicroTransactionsMatterToo Oct 13 '15 at 18:19

This function has so many almost identical if statements, it's hard to follow. It would be far easier if you just did one test for the key existing at the start, like so:

def movefunc(direction):
try:
if not room[roomnumber]['directions'][direction.lower()]:
except KeyError:


This attempts to test if room[roomnumber]['directions'][direction.lower()] is True, and if it's not prints the error. But it also prints the error if the user put input that doesn't match any of the directions.

You could test if the key exists by calling if direction.lower() in room[roomnumber]['directions']. But it's actually faster to use try except instead and it's more Pythonic. This is the "Easier to Ask Forgiveness than Permission" principle.

But why not make this even simpler? Instead of having a complicated arithmetic based system and storing booleans in room[roomnumber]['directions'][direction.lower()], just make each room's direction key have the ID of an actual room. Then you can directly set roomnumber, and your function becomes tiny:

def movefunc(direction):
try:
roomnumber = room[roomnumber]['directions'][direction.lower()]
except KeyError:


When you're making functions, try and think of ways to improve the clarity and efficiency like this. If you have a long list of lines that seem very similar then you can probably improve it. Don't be afraid to change your data structure to make other code easier. Use Python's dynamic nature to make your code easier. Especially look at places like your equipfunc, where it's huge and clearly could be shortened a lot since so much of it is so similar.

Also about names, you can just call them move and equip. Python is designed to be readable, and usually it'll be clear from how you use the basic data types what something is. var["string"] is clearly a dictionary for example, so putting func or dict in a name is usually redundant or possible points to another problem if the usage is confusing enough that you do need to distinguish what it is.

• Can you explain the reason for the try / except KeyError ? It could be done with a simple if key in dict. I personally know about the (religious war of ) LBYL / EAFP use in Python, but it might be useful to mention it in answer ? – Jiby Oct 13 '15 at 12:30
• @Jiby Good point, I added in a section about that. – SuperBiasedMan Oct 13 '15 at 13:14

Alongside @Mast's answer on the class structure, you should consider implementing a better inventory system.

Currently, you have a huge mess of inventory values that are empty and unused.

Consider a structure where the configurations are optional, and added if not present during configuration checking.

For example, instead of a structure like the following (with empty fields):

player = {
'name': 'Quill',
'occupation': 'Programmer',
'skills': {

},
'inventory': {

},
...
}


Add the empty fields when the config is added to the main class during initialisation.

So that way you can just pass in what you need:

player = {
'name': 'Quill',
'occupation': 'Programmer',
'skills': [
'Slayer of Dragons',
'Talk to Dogs',
'Make really good tacos'
],
'inventory': {
'Money': 1000000
'Rep': 5000
}
}


I would also consider taking a look at the Cactus project, a text-based game engine, with similar ideas.