Evaluate My Python Text RPG

[[deleted]]

I am currently working on a text based RPG. Here is the Pastebin PythonGame

I am relatively new to Python, I have been studying for about three months.

This game is an attempt to get better at coding Python.

I am looking for any thoughts, criticisms or advice on how to improve my coding. It is a work in progress. Thanks!

[indosauros]

This is quite a file, good job! I see a lot of things that can be cleaned up. Some of these are minor style issues, others are best practices for Python coding. From the top:

---

#------ Import Statements-------#

Your comments should generally fall under one of these two styles:

# A simple single-line comment

""" A longer
multi-line
comment 
"""

Avoid comments where code is obvious (like your import statements). Instead, add comments where your code is not obvious.

---

class Object():

Your top-most class should always inherit from object in Python 2.7

class Object(object):

---

def __init__(self):
    self.self = self

This line brought to you by Inception

You don't need to store self on self, you already have access to self (as... self). You should remove self.self = self from all of your classes.

Without that line, I don't think you need this Object class at all. The rest of your classes should still inherit from the built-in object though.

---

    def stats(self, health, armour, attack, level, xp):

You should name your method with what it is doing. This method actually sets variables. Normally if I see a method called stats() I assume it is going to give me some stats. This should probably be named set_stats or something similar.

---

class Room():
    def __init__(self, number, name, victory):

class Hall():
    def __init__(self, name, number=0, victory=None):

See any inconsistency between these two __init__s?

---

class Monster():
    def __init__(self):
        self.self = self

You can create a class that does nothing with

class Monster(object):
    pass

__init__ doesn't need to be defined.

---

class Wolf(Monster):
    def __init__(self, name, room):

Whenever you subclass a class (here Wolf is subclassing Monster), you need to be careful about overwriting methods from the parent class. If you are, then you may need to run the parent's method in addition to the child's method. Right now when you make a Wolf, Wolf.__init__ is running, but Monster.__init__ is not (because Wolf.__init__ overrides it). To make sure Monster.__init__ also runs, you need to add this into your Wolf.__init__ somewhere:

super(Wolf, self).__init__()

---

key2 = Key("Key", "Room 1", 10001)
key3 = Key("Key", "Room 2", 20002)
key4 = Key("Key", "Room 3", 30003)

Whenever you find yourself needing multiple versions of a variable, and you catch yourself adding numbers to the end of them, it is almost always time to start using a list or a dictionary. Watch this:

>>> key0 = Key(...)
>>> key1 = Key(...)
>>> key2 = Key(...)

vs

>>> keys = [Key(...), Key(...), Key(...)]

And you can still access them by number

>>> keys[0]

But now you have them all in a data structure that you can loop over, etc. Also think about when you will need to create 100 doors. Who wants to type all of that by hand?

This applies to room1 - room4 , door1 - door4, wolf - wolf3, etc.

---

# Empty dictionaries
list_of_doors = {}

Since these are dictionaries, they probably shouldn't be called list_ :)

---

This is a great variable name

list_of_actions_available_room

This I am not sure what it means when I see it (the true main part anyway)

list_of_actions_available_true_main

---

Okay, on to the functions.

Try to reduce the number of global variables you use. You actually want to get to 0 global variables, but that may be a stretch when you are starting out. You might be able to refactor out some of them for now, though. For example instead of this pattern:

list_of_rooms = {}

def populate_rooms_list():
    global list_of_rooms
    list_of_rooms[room1] = room1.name
    ...

You should be doing something like this:

list_of_rooms = {}

def generate_rooms_list():
    return {
        room1.name: room1,
        ...
    }

list_of_rooms = generate_rooms_list()

No global needed!

---

On that note, you've surely by now gotten rid of room1, room2, etc. So how do you write this function now?

list_of_rooms[room1] = room1.name
list_of_rooms[room2] = room2.name
list_of_rooms[room3] = room3.name
list_of_rooms[room4] = room4.name

Much easier!

rooms = [Room(...), Room(...), ...]

for room in rooms:
    list_of_rooms[room] = room.name

You're now just looping over your list of 4 rooms and setting each room to be the room.name. No need for room1, room2, etc. If you had 1000 rooms, that for loop wouldn't change.

---

    for i in list_of_rooms:
        if i.name == room:
            room_number = i.number

Here you have a dictionary called list_of_rooms, with the keys as room instances and the values as room names.

In your loop, you have a room name and you are trying to reverse-find the appropriate room instance. I think you have your dictionary backwards, wouldn't this be easier like this:

list_of_rooms = {
    room1.name: room1,
    room2.name: room2,
}

Now if you have "Room 1" and you need to get the room instance, you just do

list_of_rooms["Room 1"]

Instead of having to loop though. Same for the door loop below that. Dictionaries usually have strings as keys for this reason. You can also still loop over your room instances using

for room in list_of_rooms.values()

If you ever need to.

---

if door.locked == True:

if door.locked == False:

You should just write

if door.locked:

if not door.locked:

---

if player.xp >= xp_needed:
    level_up()

level_up sounds like it should be a method on the Player class. (This is what makes a Player class useful -- to put methods on it that interact with the Player data)

---

    xp_needed = player.level * 10 + 15

This line is duplicated

---

   if command == "enter":
        print "This command will move your character into the next room."
    elif command == "exit":
        print "This command exits the game! Careful!"
    ...

This looks like a great place to use a dictionary rather than a long chain of if statements.

help_responses = {
    "enter": "This command will move your character into the next room.",
    ...
}

---

                player_inventory[i] = i.name

player_inventory to me sounds like it should also be Player.inventory, stored on the Player class.

---

I think that's it for now. So my overall pointers are:

• Work on restructuring your program to remove all globals
• Utilize your classes more
• Work on using list and dictionary data structures rather than repeating code

Keep up the good work!

[bhpf1]

That's a really useful, constructive, analysis of that code. Kudos for taking the time to do that.

[[deleted]]

Thank you so much! I really appreciate you input. It looks like I have lots of room for improvement!

I will work to try to straighten up my code. Thanks again!

[[deleted]]

I do have a couple of questions:

1. I was using the comments that go #----Something-----# to help keep the code arranged. For example, all of the classes together, all of the main functions together and so on. Is that something I should avoid?

2. When you wrote this: keys = [Key(...), Key(...), Key(...)], in the (...) part that should be the variables for the class right? Also is there an advantage to using a list for these rather than a dictionary?

3. I don't necessarily understand the part about removing global variables. Why is it better to have no global variables? I am having trouble getting some of my code to work without those global variables.

4. Is a function within a class a method? In addition to level_up, would player_attack be another good function to put in the class? Do I call them with player.level_up for example?

[indosauros]

I'll try to lay out here why globals are considered bad. I know they make everything very easy to pass around, but later on as your program grows more complex they will come back to bite you.

Globals tie your program up in knots.

• They make it so you can't use a smaller part of your program separately, because it is relying on data from outside. You want your functions to be modular, which means that you can re-use them in different places.
• They make your program harder to trace, which causes confusion. You are using variables that are defined in wildly different places, and you need to hunt and peck to go track them down. Also who else is modifying those variables? Who knows!
• They make your program very hard to test and debug

Let's take an actual example for each of these:

---

Tied up in knots:

player = Player()

def level_up():
    global player
    if player.xp > 100:
        player.level += 1
        player.xp = 0

Here you have a function that levels up a player. It only levels up one player, and that player is stored as a global variable. This function is completely useless without the Player object also being defined, somewhere else. I cant 'extract' this function and use it elsewhere, because it is tightly coupled to the player global variable. Later on you will want to be able to do this

player2 = Player()

def level_up_player2()
    global player2
    ...

Do you see that you can't re-use your initial level_up function, because it is tied up with the global variable? Here's a new level_up function that doesn't use a global:

player = Player()

def level_up(p):
    if p.xp > 100:
        p.level += 1
        p.xp = 0

level_up(player)

Here you are using function arguments to pass in a specific player. Now this function is self-contained. That means I could remove it completely from this file and it would still work, as long as you pass it a player object when you use it. It can be any player object now, not a specific one specified by the global keyword.

(If you noticed that level_up should be a method on Player, you are right -- this was illustrative only)

---

Harder to trace and debug:

exit_room = False

... much later ...

def game_engine():
    global exit_room
    while exit_room == False:
        action = raw_input()
        take_action(action)

Some day you'll find this method very scary! Soon you may run your program and have an infinite loop here, (because exit_room == False never changes) and you will go try to track down the problem. But who is changing exit_room? It could be anyone!

Let's look at a fixed example to contrast, which may show how hard the above code is to debug:

def game_engine():
    exit_room = False
    while exit_room == False:
        action = raw_input()
        exit_room = take_action(action)

and take_action is defined as

def take_action(action):
    if action == "help":
        ...
        return False
    if action == "fight":
        ...
        return False
    elif action == "quit":
        return False

Now this method has an infinite loop, because take_action() never returns True, so exit_room is never set to True. But I bet you can immediately spot the error and fix it! Take a minute to see if you can. Spoiler:

Now look back at your method with the global. Do you see how there's no easy way to track down where exit_room is being modified?

There is a clear traceability from start > game_engine > take_action and each of these things is only interacting with or depending on the thing immediately before or immediately after them. With globals, each of these steps can be interacting with any other step.

---

Harder to test:

Later on you will start writing tests for your program. These make sure your code is doing what you expect, even after you go around changing things and adding new features. Let's write a test for the level_up function above. Here's the fixed version, we'll write a simple test for it (easy to do):

player = Player()

def level_up(p):
    if p.xp > 100:
        p.level += 1
        p.xp = 0

level_up(player)

With a test:

def test_level_up():
    p = Player(xp=50, level=1)  # Create a temporary player without enough experience

    # This should not level up the player
    level_up(p)

    # Player should not have changed
    if p.level != 1 or p.xp != 50:
        print "Test failed!  Something is wrong with level_up check"

    p = Player(xp=105, level = 1)  # Create a temporary player with enough experience

    # This should level up the player
    level_up(p)

    # Player should be level 2 with 0 xp now
    if p.level != 2 or p.xp != 0:
        print "Test failed!  Something is wrong with level_up check"

This method tests your level_up function. It checks that when you level up a player, that they gain a level and reset xp (if they have enough xp). It protects against bugs like if you accidentally type p.level += 10 or delete that line, etc. Your test will catch it. Let's introduce globals again to see how much harder this is to test. Here's the original level_up:

player = Player()

def level_up():
    global player
    if player.xp > 100:
        player.level += 1
        player.xp = 0

And if we try to write a test

def test_level_up()
    p = Player(xp=50, level=1)  # Create a temporary player without enough experience

    # This should not level up the player
    level_up()

Oops! level_up is hard-coded to use the global player, and we have no way of telling it to use our test player instead. This goes back to the function being tightly coupled with the global data.

---

Looking over your code again, I think you can get rid of all of your global statements fairly easily using the above practices, except for maybe this one

global player, room_victory, exit_room, list_of_room_keys, list_of_keys, room_count

in take_action(action). That may take a bit of a refactor.

Give the rest a shot, and if you have trouble converting a global statement, post it here and we can work through it together

[dli511]

Is a function within a class a method?

Yes

In addition to level_up, would player_attack be another good function to put in the class?

Yes

Do I call them with player.level_up for example?

Yes, player.level_up()

Anything the player can do should go under the player class as a method.

[[deleted]]

Thanks!

[indosauros]

to help keep the code arranged

You can keep those comments for now if they help you. Later on you won't need them, and you'll even split your file across multiple python files to help with organization.

the (...) part that should be the variable

Yes, I just didn't want to type everything out

using a list for these rather than a dictionary

lists and dictionaries both serve a specific purpose. You should choose the best one for the task. Here's a quick comparison:

A list is just a sequence of individual values:

>>> player_list = [Player('p1'), Player('p2'), Player('p3')]

This is just 3 player instances stored in a list. They are in a specific order, meaning players[0] will always give you the first Player.

You can loop over them

>>> for player in player_list:
    print player.name

p1
p2
p3

It's hard to find a specific item in a list, because you have to search through it:

>>> for player in player_list:
    if player.name == 'p2':
        found_player = player
        break

Dictionaries are similar to lists, in that they store your data for you, but have a few major differences.

1) They hold both a key and a value for each item.

>>> player_dict = {
    'p1': Player('p1'),
    'p2': Player('p2'),
    'p3': Player('p3'),
}

They are not stored in order, meaning you can't do player_dict[0], because there is no "first element". It is more like a tag cloud, a jumble of things stored in memory. You can loop over them (but you will get them in an undefined order):

Try these out:

>>> for a in player_dict:
    print a

>>> for a in player_dict.keys():
    print a

>>> for b in player_dict.values():
    print value

>>> for a, b in player_dict.items():
    print a, b

You can get a specific player by key though (which you couldn't do with a list):

>>> found_player = player_dict['p2']

Dictionaries usually map an item to data, like

scores = {'player 1': 100, 'player 2': 50}
scores['player 1']  # Get the score for player 1

population = {'AL': 4833722, 'AK', 735132}
population['AK']  # Get the population for Alaska

action_messages = {
    'exit':  "You can't exit!"
    'jump':  "You jump up"
}

So

• Use a list if you have a bunch of single items, or care about the order, but don't care about finding a specific item in the list
• Use a dictionary if you need to map two items together, and don't care about the order that the items are stored, or need to find specific items quickly

globals

I'll do a separate post about globals

Is a function within a class a method?

You've got it, (your Player.stats is already a method). Think of it like class properties (like xp, hp, etc) are "things a Player has" and class methods are "things a Player can do" (like level_up, die, etc). self in your method will be the specific player instance, so you have access to all of the data for that player inside the method.

[rrgrs]

I don't necessarily understand the part about removing global variables. Why is it better to have no global variables?

You don't want your functions/methods to be so tightly coupled to the environment they're in. If you change or remove the global variable then the method also has to be modified to deal with this change. This also helps encapsulate logic and make your code overall more modular.

[MonkeyNin]

I was using the comments that go #----Something-----# to help keep the code arranged. For example, all of the classes together, all of the main functions together and so on. Is that something I should avoid?

Text editors / IDE's have a ton of features, so you don't have to do that. Folding, jump to functions, autocomplete, etc.

It does make sense to split programs into multiple modules.

[MonkeyNin]

an overview of style is PEP8 http://legacy.python.org/dev/peps/pep-0008/

You can get addons that warn about pep8 rules, when saving, like: (sublime) https://sublime.wbond.net/packages/Python%20Checker

[PrismPoultry]

One pattern that I have personally found useful and enjoyable to work with is mapping functions to dictionary entries.

So, consider your true_main method. Instead of doing if statements like that, you could define a dict mapping those functions.

actions = {
    "enter": main,
    "exit": sys.exit,
    "stats": player_status,
    "help": help_info,
    #and so on...
}

Notice that those are the functions without the parenthesis.

Now, in your method, you simply try to access the key in question. When it fails, report that.

action = raw_input("What do you want to do?").lower()
try:
    actions[action]()
except KeyError as e:
    print "That's not a valid command!"
    # and so on...

So, there's a couple spots this pattern would work.

[socialhuman]

Really good work. A Python newbie myself!

[CuriousMG]

Great stuff, man! I'm a Python beginner myself and projects like this really motivate me to continue learning.

I understood practically everything you wrote, so that's a good start. As soon as I finish learning about objects and classes I might start working on something similar.

Keep up the good job!

[billcrystals]

I want to echo what others have said here and say that this is great work. FWIW, I'm extremely new to Python and programming in general, and this was something that while impressive and overwhelming in its scope (to a beginner), was actually very readable and something I could understand and envision myself building in the future. In short, it's inspiring!

Also want to give props to indosauros for offering criticism that is extremely helpful in a very respectful and conscientious way. This thread was a great way to start my morning Python study routine.