patternpythonMajor
Rock-Paper-Scissors game for learning purposes
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:
then you can write code like:
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:
You could define this mapping once at the top of your program:
and then refer to it later:
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:
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:
and then just looked them up:
In that example, you have to declare
Obviously, that
and then compute the symmetric closure of that. But don't worry too much about that in your context.
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 = 3then you can write code like:
if user == ROCK and comp == PAPER:
# user loseswhich 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 = 3if user == ROCK and comp == PAPER:
# user losesif 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.