A small Python text adventure "frame"

I've been working on a small "frame" for a text adventure in Python.

So it's less of a real adventure and more of a small testing location for all the commands.

I pretty much just started learning Python after completing the Codecademy tutorial and don't want to pick up any bad habits.

```
#Text Adventure

import shlex

def map():
print ' _____________________________ '
print '| | | |'
print '| | | |'
print '| Room B = Room D = Room E |'
print '| Jack | Sword | Bear |'
print '| | | |'
print '|____||___|____||___|____\____|'
print '| | | |'
print '| | | |'
print '| Steve | Jesse | Room F |'
print '| Room A = Room C = Chest |'
print '| | | |'
print '|_________|_________|_________|'

#The Room class
class Room(object):
def __init__(self, location,objectsinRoom, containersinRooom, wallsinRoom,peopleinRoom,hasDoor,\
description,descriptionpeople,descriptiondirections):

self.location = location
self.objectsinRoom = objectsinRoom
self.containersinRooom = containersinRooom
self.wallsinRoom = wallsinRoom
self.peopleinRoom = peopleinRoom
self.hasDoor = hasDoor
self.description = description
self.descriptionpeople = descriptionpeople
self.descriptiondirections = descriptiondirections

#Returns what will be printed about the Room you're in right meow
def display_Room(self):
stri = (' ')

itemprint = []
contprint = []
peopprint = []

for item in self.objectsinRoom:
if item.islocked == True:
pass
else:
if item.incontainer == True:
itemprint.append('A ' + item.name + ' is in the ' + item.containername + '. ')
else:
itemprint.append('

There's a lot of code here, so it's hard to make a cohesive review. Instead I'll point out a few things related to your comment about avoiding picking up bad habits. The most common problems I see in your code are repeated work, and misplacement of responsibilities.

This isn't comprehensive. I'm not going to even touch on the question of whether the rooms and players should be code or external data files. But here goes:

Room.__init__

• I don't like having 9 parameters used by position. This makes things hard to read when invoking it; how do you know what's what in your RoomA = Room(...) lines? In your case, having named things like wallsInRoomA does clear this up, but that's not always the right answer.

• Good job wrapping the line. However note that inside matched braces, parentheses, or brackets, you don't need the trailing backslash.

Room.display_Room

• Calling it display_Room limits duck typing from the outside. Instead of being able to write one helper function which eventually delegates to a method called display, it has to know the type of the thing it's displaying, and call the appropriate method. Granted today that's only Rooms, and there is no helper.

• The comparisons like if item.islocked == True could be merely if item.islocked; the == True is redundant and distracting.

-
The way an item is displayed seems to put the logic in Room instead of where I think it would belong: on Item. If the code that combines the item name and location was put on Item (and similarly for containers and people), this function would look more like this:

def display():
    stri = ' '
    stritem = stri.join(item.display() for item in self.objectsinRoom if not item.islocked)
    strcont = stri.join(cont.display() for cont in self.containersinRooom)
    strpeop = stri.join(person.display() for person in self.peopleinRoom)
    return self.description + strpeop + strcont + stritem

People.go

• Should go be on People or Player? If you'll have wandering NPCs, it's fine on People, but if you won't, that seems somewhat odd to place this on the base class.

• The conversion of self.location from a tuple to a list and back is cute; it implicitly copies things, allows you to make your change, and so forth. But it also inherently limits how your world is laid out. You can't skip rooms. You can't make infinite loops. You can't make rooms by the North or South poles that don't follow the usual "square" terms. I would probably instead make the next room part of the doors.

• There's a lot of repeated code here. It would be good to refactor this, say by checking Decision and setting variables for the parts that differ (perhaps index, move, and direction), and then combining the remaining code. Or if you put this on a Door, you just have to look up your door, then process the door.

• Rather than using world.get and checking for None, I would probably just index world[tuple(newlocation)] and handle the potential KeyError in one spot. This change from get to indexing is more important in Player.printlocation, where using world.get(self.location) might return None, and you'd get a hard to read exception about NoneType not having attribute display_Room. If this was just world[self.location].display_Room(), you'd get a KeyError about self.location not being in the dictionary. Note as well that printlocation is not using the passed location parameter.

Player.open and Player.close

• There's a lot of reuse of tokens[1]. This should probably be given a name, at least in the else clauses, to make things more readable.

Player.talk

• The logic here should probably be refactored to the various NPCs. As is, all the talking logic of your entire set of NPCs is in the Player class. The trick is finding the balance between code like you have now, giving them each a different subclass, and trying to abstract just the bits you need somewhere inbetween.

StackExchange Code Review Q#37677, answer score: 6