Rock-Paper-Scissors game for learning purposes

Question

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()

Answer 1

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.

Answer 2

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.

Answer 3

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.

Answer 4

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] ()

Answer 5

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

Answer 6

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()