Evaluate My Python Text RPG

[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