Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I just want your opinions about my code. How could I improve it? Is there a better method to do it?

from sys import exit
from random import randint

print "Welcome to the PIN Game 1!"
print "Tell me your name"

user = raw_input('> ')

#asks the user for an entry and prints what he has chosen
def start(): 
    print "%s, choose one of the options: " % user

    print """
    1. Rock
    2. Paper
    3. Scissors
    """

    choice = 0
    cpu_choice = randint(1,3)

    while choice <= 0 or choice >= 4:

        choice = int(raw_input('> '))
        if choice == 1:
            print "You chose ROCK!"
        elif choice == 2:
            print "You chose PAPER"
        elif choice == 3:
            print "You chose SCISSORS"
        else:
            print "What is your problem? Try again, this time choose a number between 1 and 3!"
    cpu_decision(cpu_choice)
    result(choice, cpu_choice)

#random cpu input
def cpu_decision(cpu):
    if cpu == 1:
        print "Your opponent chose ROCK!"
    elif cpu == 2:
        print "Your opponent chose PAPER!"
    else:
        print "Your opponent chose SCISSORS"

#if user loses
def lose():
    print "Sorry, you lost the match"

#if user wins
def win():
    print "Congratulations! You beat your opponent!!"

#prints the result
def result(user, comp):
    if user == comp:
        print "Gee! You tied!"
    elif user == 1 and comp == 2:
        lose()
    elif user == 1 and comp == 3:
        win()
    elif user == 2 and comp == 1:
        win()
    elif user == 2 and comp == 3:
        lose()
    elif user == 3 and comp == 1:
        lose()
    elif user == 3 and comp == 2:
        win()

    again()

#ask if the user wants to play again
def again():
    print """
    Wanna play again?
    1 for YES
    Anything else to quit
    """

    again1 = int(raw_input('> '))

    if again1 == 1:
        start()
    else:
        exit()

start()
share|improve this question
2  
I have rolled back the last edit. Please see what you may and may not do after receiving answers. – Vogel612 Feb 11 at 16:27
up vote 34 down vote accepted

Choice representation

You're using numbers to represent choices (rock/paper/scissors). That's a reasonable choice, but as it stands, you have the mapping between choices and numbers smeared all across your program. So you have to know when reading it that 1=rock, 2=scissors---no, wait, 2=paper---you can see how easy it is to make a mistake.

If you start off your program with something along the lines of:

ROCK = 1
PAPER = 2
SCISSORS = 3

then you can write code like:

if user == ROCK and comp == PAPER:
   # user loses

which is much easier to read.

The all-uppercase names are used by convention to indicate constants; python doesn't treat them differently.

Also note that this isn't really the best way to do this; what I'm describing is a clumsy way of doing an enumerated type; there are cleaner approaches in all versions of Python, and proper support for enums in Python 3. I'm suggesting a lightweight approach to suit where I've guessed your skill level to be.

Choice output

At the moment, you have lots of conditionals of the approximate form:

if choice == 1:
    print "Chose ROCK!"
elif choice == 2:
   print "Chose PAPER!"

You could define this mapping once at the top of your program:

names = { 1: "ROCK", 2: "PAPER", 3: "SCISSORS" }

and then refer to it later:

print "Chose {}!".format(names[choice])

Note that it would make sense to replace those literal numbers with the enumerated constants above; I'm just showing the literal numbers to avoid excessive coupling within this answer.

Data-driven victory conditions

You have a huge chunk of code:

if user == comp:
    # tie
elif user == 1 && comp == 2:
    lose()
elif user == 1 && comp == 3:
    win()
#...etc.

Even if we write it using named constants for the choices, it's still unwieldy, and the interaction between the conditional branches makes it easy to write bugs.

It would be easier to read and understand if you described those relationships in data:

outcomes = {
    ROCK: {PAPER: lose, SCISSORS: win},
    PAPER: {ROCK: win, SCISSORS: lose},
    SCISSORS: {ROCK: lose, PAPER: win}
}

and then just looked them up:

if user == comp:
    # tie
else:
   outcomes[user][comp]()

In that example, you have to declare outcomes after win() and lose(), as you're storing references to those functions in the dictionary; you could also choose some other way of representing the outcome (strings, a boolean, etc), although then you'd have to interpret it rather than just calling the function returned from the lookup.

Obviously, that outcomes data structure is rather redundant, as it encodes the rules both ways round: for a problem this size, that's probably simplest; if you had a larger, more complex game, I would suggest something like a relation:

beats = {(ROCK, SCISSORS), (PAPER, ROCK), (SCISSORS, PAPER)}

and then compute the symmetric closure of that. But don't worry too much about that in your context.

share|improve this answer
    
That was a very complete answer. Thank you. I have updated my code and I used your insights – mschlindwein Feb 11 at 16:18
1  
The obvious choice for clearly limited named options of a particular type is an enum and not 3 theoretically independent constants, apart from that good answer. – Voo Feb 11 at 19:09
    
@Voo I agree in general---I was trying to pitch it at a very simple level to suit the OP (particularly given that they're using a "roll-your-own-enums" version of Python). – David Morris Feb 11 at 19:39
    
Fair point, installing third party libraries ( the backport of the official enum module to 2.x) is a bit much. I just forgot about the non existence since it's part of my template for any new project. – Voo Feb 12 at 5:48
    
This answer (particularly the data driven victory conditions part) has the added benefit that your code does not get significantly longer when you start to implement Rock, Paper, Scissors, Lizard, Spock. – Boluc Papuccuoglu Feb 12 at 12:06

You'd do well to use an enum or constants for your rock, paper and scissors.

For instance, what you've got here...

elif user == 1 and comp == 2:
    lose()
elif user == 1 and comp == 3:
    win()
elif user == 2 and comp == 1:
    win()
elif user == 2 and comp == 3:
    lose()
elif user == 3 and comp == 1:
    lose()
elif user == 3 and comp == 2:
    win()

That's not readable at all!

If you defined constants instead, you could do like this:

#near the top
rock = 1
paper = 2
scissors = 3

#in result
elif user == rock and comp == paper:
    lose()
elif user == rock and comp == scissors:
    win()
elif user == paper and comp == rock:
    win()
elif user == paper and comp == scissors:
    lose()
elif user == scissors and comp == rock:
    lose()
elif user == scissors and comp == paper:
    win()

And it becomes easier to spot if you made a mistake (like if you accidentally swapped the win condition for scissors vs paper with scissors vs rock).

If you want to learn more about that, I suggest googling about "enums" or "magic numbers". Additionally, we might want to use the style convention that constants are UPPER_SNAKE_CASE:

#near the top
ROCK = 1
PAPER = 2
SCISSORS = 3

#in result
elif user == ROCK and comp == PAPER:
    lose()
elif user == ROCK and comp == SCISSORS:
    win()
elif user == PAPER and comp == ROCK:
    win()
elif user == PAPER and comp == SCISSORS:
    lose()
elif user == SCISSORS and comp == ROCK:
    lose()
elif user == SCISSORS and comp == PAPER:
    win()

By using a specialized style for constants, it's easier to see that it's a constant, and not some local variable.

share|improve this answer
    
I updated the code with that ;) – mschlindwein Feb 11 at 16:19

It's good that you're trying to use functions, but I think you're overusing them a bit.

#if user loses
def lose():
    print "Sorry, you lost the match"

#if user wins
def win():
    print "Congratulations! You beat your opponent!!"

Unless you intend to add more code to these (like perhaps a score keeping function) they're pretty redundant lines. You've just abstracted away a simple print call, but it might look like something more to someone else, so you're just creating confusion.

You have a function for cpu_decision, which is worthwhile, but you call randint outside the function, why not just call it from inside?

#random cpu input
def cpu_decision():
    choice = randint(1,3)
    if choice == 1:
        print "Your opponent chose ROCK!"
    elif choice == 2:
        print "Your opponent chose PAPER!"
    else:
        print "Your opponent chose SCISSORS"
    return choice

By using return choice you can get what number was chosen in the function instead of having to call randint elsewhere and pass the value into cpu_decision.

I also think that you should have the user input in its own function, similar to cpu_decision above, it can handle the printing and return the user's choice. Thaat way your start code can be much neater:

def start():
    print "%s, choose one of the options: " % user

    print """
    1. Rock
    2. Paper
    3. Scissors
    """
    choice = user_decision()
    cpu_choice = cpu_decision()
    result(choice, cpu_choice)

Your result code is confusing. It would be better if you grouped conditions instead.

#prints the result
def result(user, comp):
    if user == comp:
        print "Gee! You tied!"
    elif ((user == 1 and comp == 2) or (user == 2 and comp == 3)
          or (user == 3 and comp == 1)):
        lose()
    elif ((user == 1 and comp == 3) or (user == 2 and comp == 1)
          or (user == 3 and comp == 2)):
        win()

This is still confusing, just because there's a lot of condition checking of arbitrary numbers, but Pimgd's suggestion about enums would help that a lot.

You call again from result. Then again calls start which calls result which calls again which calls start which calls result which calls again which calls start which calls result which calls again which calls start which calls result which calls again which calls start...

You see where this is going? You've made a recursive pattern. After a certain number of calls Python will error out because of being too deep. That would be after a LOT of games, but it's still better practice to avoid it. You could instead set up an infinite game loop:

def main():
    while True:
        start()
        print """
        Wanna play again?
        1 for YES
        Anything else to quit
        """

        again1 = int(raw_input('> '))

        if again1 != 1:
            break

Now instead you can call main, which will loop forever until the user enters anything other than 1 after their game.

share|improve this answer
    
Ì think that while 0 >= choice >= 4: will alway give False – oliverpool Feb 11 at 11:45
    
@oliverpool Derp, you're right, it's an or condition, and I turned it into an and without thinking it through. Thanks for correcting me! – SuperBiasedMan Feb 11 at 11:49
1  
You can still change that condition to while not (0 < choice < 4). – Mathias Ettinger Feb 11 at 13:09
    
I've turned the functions into variables using a dictionary, that way I could shrink a lot the result function. What do you think of it? The updated code is in the OP. – mschlindwein Feb 11 at 16:23
    
@mschlindwein A dictionary is also a good solution! I didn't suggest it but it definitely neatens code without needing full function definitions. – SuperBiasedMan Feb 12 at 12:17

Will I win maddest solution prize?

if user == comp:
    draw()
elif (user+2 ) % 3 == (comp + 3 ) % 3:
    win()
else:
    lose()

EDIT

[win, draw, lose][ (comp + 3 ) % 3 - (user+2 ) % 3] ()
share|improve this answer
    
Terrible for readability, but +1 anyway because modulo's the first thing I thought of when I saw this, too. =P – Mike Kellogg Feb 11 at 17:20
1  
feel free to come check out PPCG any time ;) – undergroundmonorail Feb 12 at 11:01
1  
-1 for being worse than OP. The problem with terse python/perl/haskell crap is their WTF/LOC is very high, for having fewer lines for more WTFs. – abuzittin gillifirca Feb 12 at 11:27
    
But hey, if we continue to increase wtf density, can we eventually create a wtf black hole? – Eugene Feb 13 at 8:35
    
Actually, I'm trying to fight back overengineering, which produces all kind of superflexible megametaconfigurable helloworlds. You see, just the opposite side of spectrum. – Eugene Feb 13 at 8:41

A further suggestion to @Pimgd's answer:

You could also use 3 constants for WIN, TIE, LOSE and a function user_result(user, comp) which returns the result of the left argument. In this function you test for equality and return TIE if it's the case, otherwise you compare the choices

def user_result(user, comp):
    if user == comp:
        return TIE
    elif user == ROCK:
        return WIN if comp == SCISSORS else LOSE
    elif user == PAPER:
        return WIN if comp == ROCK else LOSE
    elif user == SCISSORS:
        return WIN if comp == PAPER else LOSE
share|improve this answer
    
I used that in combination with a dictionary. – mschlindwein Feb 11 at 16:24

I've changed my code according to your answers:

  • I created variables for the answers, and dictionaries to reference to them as suggested. Symmetric closure is a thing that I'll take a look at.
  • I created a user_decision function and changed the again for the main function. I changed the result function, and now I am using variables and a dictionary to return win and lose (now they are variables, instead of functions).
  • I created variables for the results.

from sys import exit
from random import randint

print "Welcome to the PIN Game 1!"
print "Tell me your name"

user = raw_input('> ')
print "%s, choose one of the options: " % user

#Constants
ROCK = 1
PAPER = 2
SCISSORS = 3

names = {ROCK: "ROCK", PAPER: "PAPER", SCISSORS: "SCISSORS"}

#asks the user for an entry and prints what he has chosen
def start(): 
   print """
   1. ROCK
   2. PAPER
   3. SCISSORS
   """

   choice = user_decision()
   cpu_choice = cpu_decision()
   result(choice, cpu_choice)
#user input
def user_decision():
    choice = 0

    while choice <= 0 or choice >=4:

        choice = int(raw_input('> '))
        if choice >= 1 or choice <= 3:
            print "You chose %s" % names[choice]
        else:
            print "What is your problem? Try again, this time choose a number between 1 and 3!"
    return choice

#random cpu input
def cpu_decision():
    choice = randint(1,3)
    print "Your opponent chose %s!" % names[choice]
    return choice

lose = "Sorry, you lost the match..."
win = "Congratulations, you won!!"

outcomes = {
    ROCK: {PAPER: lose, SCISSORS: win},
    PAPER: {ROCK: win, SCISSORS: lose},
    SCISSORS: {PAPER: win, ROCK: lose}
}

#prints the result
def result(user, comp):
    if user == comp:
        print "Gee! You tied"
    else:
        print outcomes[user][comp]

#ask if the user wants to play again
def main():
    while True:
        start()
        print """
    Wanna play again?
    1 for YES
    Anything else to quit
        """

        again1 = int(raw_input('> '))

        if again1 != 1:
            break

main()
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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