HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMajor

Rock-Paper-Scissors game for learning purposes

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
scissorspaperlearninggamerockforpurposes

Problem

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

Solution

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.

Code Snippets

ROCK = 1
PAPER = 2
SCISSORS = 3
if user == ROCK and comp == PAPER:
   # user loses
if choice == 1:
    print "Chose ROCK!"
elif choice == 2:
   print "Chose PAPER!"
names = { 1: "ROCK", 2: "PAPER", 3: "SCISSORS" }
print "Chose {}!".format(names[choice])

Context

StackExchange Code Review Q#119614, answer score: 34

Revisions (0)

No revisions yet.