12
\$\begingroup\$

I'm currently working on a text adventure game for my first project. I am a beginner so I am seeking help with my code. Please comment any tips and general help regarding with making my code better, faster or more organised! I only started Python two weeks ago so any help would be greatly appreciated. I decided to only submit the code for the first room as I don't want to submit too much code at once.

health = 100
coins = 0
tutorial = True
gameStart = False
death  = False
spawn = True
livingRoom = False
Bathroom = False
Bedroom = False
Kitchen = False
wardrobeSpawn = False
spawnStart = True
telev = False


def countCoins():
    print
    print("You have: " + str(coins) + " coins")


while gameStart == True:
    if spawn == True:
        if spawnStart == True:
            print
            print(bcolors.FAIL + "You wake up in a room you don't 
recognise, with no idea how you got there")
            print
            print(bcolors.ENDC + "You look around and see a wardrobe 
and a door")
            spawnStart = False
        elif spawnStart == False:
            print
            spawnIn = raw_input("")
            spawnIn = spawnIn.lower()
            if spawnIn == "coins":
                countCoins()
                spawnStart = False
            elif spawnIn == "wardrobe":
                if wardrobeSpawn == False:
                    print
                    print "The wardrobe seems quite empty, but you do 
find a coin in the far left corner, you put it in your pocket"
                    print
                    coins += 1
                    wardrobeSpawn = True
                    spawnStart = False
                elif wardrobeSpawn == True:
                    print "This wardrobe looks oddly familiar, you have 
already explored it"
                    print
                    spawnStart = False
            elif spawnIn == "door":
                print
                print ("You walk out the door")
                print
                spawn = False
                livingRoom = True
            elif spawnIn == "look":
                print("You look around and see a wardrobe and a door")
                print
                spawnStart = False
            else:
                print("That is an unvalid command. Try again")
                print
                spawnStart = False
\$\endgroup\$
  • 3
    \$\begingroup\$ Being that your new to Python I would suggest you run you code thru pylint before posting to code review. \$\endgroup\$ – Ricky Wilson May 28 '17 at 13:08
13
\$\begingroup\$

Bugs

In CountCoins:

print

... has no effect. The same applies to other parts of your code where you used this. If you meant to print a newline, use print() instead. You've also used print inconsistently, sometimes just leaving out the parentheses. Your tags include python-3.x, so you should fix that.

raw_input("")

... there's no such thing as raw_input in Python 3. I'm starting to think you've used a wrong tag.

Comparisons to True

Python automatically compares to True in if / elif / else / while statements. Therefore, you can shorten this:

while gameStart == True:

if spawn == True:

if spawnStart == True:

... to this:

while gameStart:

if spawn:

if spawnStart:

... what this also means is that instead of writing:

if wardrobeSpawn == False

... you can write:

if not wardrobeSpawn:

... which is shorter and more readable.

F-strings

Instead of doing:

print("You have: " + str(coins) + " coins")

You might want to use a so-called f-string:

print(f"You have {coins} coins")

This is easier for you and other developers. f-strings are only available in Python 3.6 and later.

Functions

You seem to be using just one function. I suggest splitting the game up into several functions, perhaps like this:

def Level1():
    print("You wake up ...")

def Choice1()

def Choice2()

Styling

You should definitely read PEP8. It states that you should, for example, use snake_case for variables and function names. Also, you should leave enough whitespace in between blocks of code to make it easier to read and manage. Really, take a look, it's a good read.

\$\endgroup\$
  • 4
    \$\begingroup\$ f-strings are available only in Python 3.6. I think you should mention that :) \$\endgroup\$ – Grajdeanu Alex. May 28 '17 at 8:42
  • \$\begingroup\$ Roger. I work mainly with 3.6 so forgot to add that :) \$\endgroup\$ – Daniel May 28 '17 at 9:54
  • 10
    \$\begingroup\$ Note that print('\n') is equivalent to print('\n', end = '\n') which will print two new lines, not one. The correct answer is print(). \$\endgroup\$ – wizzwizz4 May 28 '17 at 15:42
  • \$\begingroup\$ Using an IDE helps enforcing styling rules and python idioms. \$\endgroup\$ – FMaz May 29 '17 at 11:31
7
\$\begingroup\$

preface - OOP background

please note that i'm from an OOP background and my answers will necessarily reflect that bias. i have no experience with python or any functional languages, so please keep in mind.

use comments

far too many if statements in the one block ....an explanation would be very helpful and handy to understand what is going on. if once comes back in 3 months to read the code - it might be difficult for the author to understand

please write use useful names

why do you have so many boolean variables? and they all sound the same: spawnStart, Spawn, SpawnIn, GameStart, wardrobeSpawn please use useful names. I can't emphasise the importance of giving useful names. sometimes i see methods like this: DoStuff(). please don't do things like that.

printing new lines

you can also print a new line with print "\n\nYou wake up in a room you don't recognise....." there's no need to repeat the print method more than once. cleaning up the print statements with \n alone improves the code.

also look up string interpolation in python.

the flow of the program

how will the game ever end: you don't set gamestart to false? also the wakeup statement is run only once so you needn't put it in the loop.

spawn is never set to false either. so you can safely remove this bit from the loop:

 if spawn == True:
        if spawnStart == True:
            print
            print(bcolors.FAIL + "You wake up in a room you don't 
recognise, with no idea how you got there")
            print
            print(bcolors.ENDC + "You look around and see a wardrobe 
and a door")
            spawnStart = False

lastly, what is it that you are trying to accomplish with all these booleans: you don't want two actions to be repeated twice? i don't quite understand what's going on here.

It's not very oop

you don't want to see if statements too much. you want to see objects in your code. and you want to see this type of thing:

Action.door()

or:

Action.wardrobe()

where Action is an instantiated object. apologies if that's the wrong python syntax - i don't know any python. In other words you want to see objects send "messages" to each other. Please read this:

http://esug.org/data/Old/ibm/tutorial/OOP.HTML

The only place where you actually want to see if statements are in factory methods (actually this is not true - but you gotta be very skeptical of if statements as a developer starting out). too many of them and it may mean that your code will be very difficult to change and understand.

place code snippets in their own methods

this type of code: can be placed in a method. that way if you need to repeat it, you can do so easily.

 print
                    print "The wardrobe seems quite empty, but you do 
find a coin in the far left corner, you put it in your pocket"
                    print
                    coins += 1
                    wardrobeSpawn = True
                    spawnStart = False

Hide Implementation Details

You should hide implementations details to make your code look something like this:

            SetUpAllBooleanValues()

            WakeUp()
while gameStart == True:            
            spawnIn = GetAnswer()

            if spawnIn == "coins":
                    CountCoins()                
            elif spawnIn == "wardrobe":
                    FindCoinInWardrobe()
            elif wardrobeSpawn == True:
                    AlreadyExploredWardrove()
            elif spawnIn == "door":                
                    WalkOutDoor()
            elif spawnIn == "look":
                    LookAround()
            else:
                    InValidComment()

I haven't really changed anything, but noticed how it's much easier to read and understand? Of course you will have to move some code into the new methods i've named above.

testing boolean values

you can do

if spawn

you don't need to write if spawn == true they are both the same thing in fact.

Use Case Statements

Case statements are much easier to read than if / else statements. remember your code will be read many times after it is written. make it easy to understand.

make it readable

make the indentations clear so that it's readable. overall i applaud you for posting it on code review and you should be congratulated for your efforts. perhaps revise your code and repost.

anyways, I hope this code review helps.

\$\endgroup\$
  • 4
    \$\begingroup\$ Python doesn't actually have switch-case statements. \$\endgroup\$ – Daniel May 28 '17 at 14:06
  • \$\begingroup\$ Python doesn't really have factories. There are metaclasses, but those aren't used much. Far more common is the lambda-returning function, but that isn't a factory as it doesn't produce a class, despite both being callable. \$\endgroup\$ – wizzwizz4 May 28 '17 at 15:43
  • 1
    \$\begingroup\$ @Coal_ Python does not have switch-case, but you can use dictionaries for a similar effect. action = {'coins': CountCoins, ...} \$\endgroup\$ – Graipher May 29 '17 at 9:03
  • 2
    \$\begingroup\$ Python is not OOP only. I dislike this answer for giving such one-sided software design advice. 'You want to see objects in your code' makes me very much want to cry out loud. You should definitely take a look at a functional language. It will teach you a lot. \$\endgroup\$ – FMaz May 29 '17 at 11:36
  • 1
    \$\begingroup\$ Given that you haven't used Python much, how about starting with that? You could challenge yourself to build something without using a single class and instead rely on well thought-out algorithms and data-structures. That will get your feet wet. Python only supports functional though it does not generally promote it. If you want to try purely functional programming (and all it's benefits), you could take a look at Haskell. I would never put one over the other, but functional programming will help you get a deeper understanding of being a software engineer. Thank you for the preface :) \$\endgroup\$ – FMaz May 29 '17 at 12:47
4
\$\begingroup\$

Blank lines

You don't need more than one in a row. Blank lines are for separating logical sections of code. If you end up having blocks so complex that you need multiple blank lines, it would be better if you split it up into multiple functions anyways.

Capitalized names

Use them for classes, not for room names.

Use First-Class Functions

A text adventure game can be represented as a state machine (with some extra properties like health, etc.). A good way to model a state machine is with first-class functions:

def place1():
    print("You are in place1.")
    i = input("Where would you like to go? ")
    if i == "place2":
        return place2
    elif i == "place3":
        return place3

def place2():
    # some example code here
    pass

def place3():
    # more example code
    pass

location = place1
while True:
    location = location()
\$\endgroup\$
4
\$\begingroup\$

If/Else Cases

if spawnStart == True:
   ...
elif spawnStart == False:
   ...

can be replaced by:

if spawnStart == True:
   ...
else:
   ...

If the first if statement fails, then it is because spawnStart is not True -- and, therefore, spawnStart must be False. (Or None, but unless you plan on that happening we'll ignore the possibility.) Therefore, if the interpreter reaches the elif at all (only if the first 'if' failed), we can be sure that its statement will resolve to True, and so a single guaranteed else will have the same behavior.

(Also, as pointed out above, spawnStart == True can just be spawnStart).

You use this idiom elsewhere too:

if wardrobeSpawn == False:
    ...
elif wardrobeSpawn == True:
    ...

Note, however, that a chain of elifs is correct and necessary where you compare spawnIn to a bunch of things, since there are multiple potential options and a default of invalid command.

Order of Execution

Another point is that you are constantly setting spawnStart to false. For instance:

elif spawnIn == "look":
    print("You look around and see a wardrobe and a door")
    print
    spawnStart = False

This might normally be desired behavior. The odd thing, though, is that it all falls under the following:

if spawnStart == True:
    ...
elif spawnStart == False:
    <all the rest of the code>

It's all under the elif (or, if you take my above advice, under the else). Therefore, spawnStart is already False! Unless you in the future plan to add a bunch of stuff that sets spawnStart to True, setting it to False now is redundant.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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