Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

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 wrote a simple Rock, Paper Scissors game in Python for class. As a beginner, I would like some input as to how I could abstract or make my code smoother. I feel like the repeated use of if or elif statements are overbearing. I would like any tips or advice, or perhaps a different method of approach.

import random
user = ''

while user != 'q':
    print('Enter your choice')
    print('R or r for rock\nP or p for paper\nS or s for scissors\nQ or q to quit')
    user = input('')
    user = user.lower()
    if user == 'q':
        break
    comp = random.randrange(1, 4)
    choice = comp

    # assign (int) to user choices
    if user == 'r':
        user = 1
    elif user == 'p':
        user = 2
    elif user == 's':
        user = 3

    # assign string to computer random choice
    if   comp == 1:
        choice = 'rock'
    elif comp == 2:
        choice = 'paper'
    elif comp == 3:
        choice = 'scissors'

    print('Computer picked:', choice)

    # compare selection to figure out the winner
    if user == 1 and comp == 3:
        print('You win against the computer\n')
    elif user == 3 and comp == 1:
        print('You lose against the computer\n')
    elif user > comp:
        print('You win against the computer\n')
    elif user < comp:
        print('You lose against the computer\n')
    elif user == comp:
        print('You tie against the computer\n')
share|improve this question

migrated from stackoverflow.com 23 hours ago

This question came from our site for professional and enthusiast programmers.

    
What about 'switch case'? – the scion 23 hours ago
4  
@thescion: As this is Python, there is no switch case. That said, RPS is trivially coded up with a dictionary. – Martijn Pieters 23 hours ago
2  
We have a rock-paper-sciccors tag. Feel free to browse those questions for alternative implementations. There should be a couple in python as well. – Mast 22 hours ago
2  
Your title should indicate what the code is intended to do - please read our How to Ask, because it's quite different to that of Stack Overflow. – Toby Speight 21 hours ago
1  
I don't think scissors made of paper would be very effective. – TylerH 14 hours ago
up vote 4 down vote accepted

The way you are trying to solve this problem does not seem pythonic. Since switch statements don't exist in python, you can simulate switch-like behaviour using dictionaries.

For example, instead of using if-else to set the value of user, you can so something like

user_options = {
    'r': 1,
    'p': 2,
    's': 3
}
user = user_options[user]

Since functions and classes are first-class objects in python, if you want to accomplish some less trivial, you can use them too:

def func1():
    # do some operation

def func2():
    # some other operation

def default_func():
    # some default action

options = {
    '1': func1,
    '2': func2,
    'default': default_func
}

value = raw_input('enter a number')

result = options.get(int(value), options.get('default'))()

If your functions are simple enough, you can use lambdas as values, which would be definitely be more concise.

share|improve this answer
2  
What is user = user_options[user] doing? You define the dictionary first and then set user equal to the corresponding value in the dictionary? – Richard Peña 22 hours ago
1  
In your code, you are setting user =1 if it is equal to 'r', that's exactly what that statement is doing – hspandher 22 hours ago

In Python you can compare multiple values the following way:

if (user, comp) in {(0, 2), (1, 0), (2, 1)} :
    print('You win against the computer\n')
elif user == comp :
    print('You tie against the computer\n')
else :
    print('You lose against the computer\n')
share|improve this answer

Here are 3 corrections. Instead of manually converting r,s,p and 1,2,3, use a dictionary. Instead of using a random number for the computers choice, use the players options. Instead of having extra win/lose/tie checks, have one per outcome.

Fix Conversion

This is a simple way to convert r,p,s into a name or a value:

  • options['r']['name'] == 'Rock'
  • options['r']['value'] == 1
options = {
    'r':{'name':'Rock','value':1},
    'p':{'name':'Paper','value':2},
    's':{'name':'Scissors','value':3}
}

Fix Computer's Choice

By choosing a random key from options prevents having to hard-code values. It also allows us to lookup the name and value for the computer's choice. compChoice will be r, p, or s since those are options' keys.

compChoice = random.choice(list(options.keys()))

Fix win/lose/tie Checks

A tie only happens if compValue==userValue. Now consider +1 mod 3, for any value if you "+1 mod 3" you will get the only value that can beat it:

  • rocVal+1 %3 == papVal
  • papVal+1 %3 == sciVal
  • sciVal+1 %3 == rocVal
compVal = options[compChoice]['value']
userVal = options[userChoice]['value']
result = 'win'

if userVal == compVal: result = 'tie'
elif userVal+1 % 3 == compVal: result = 'lose'
# else: result = 'win'

print('You '+result+' against the computer!\n')

Advanced Check

But what if you want to play Rock-Paper-Scissors-Lizard-Spock? Or a any version with more than 3 options? If that is the case, here is a scalable solution from sch. Note that the number of options should always be odd. This way each element has the same number of superiors and inferiors.

Explanation: the key piece here is the decider. We want the difference between the two choices to be greater than or equal to 0 so we have to add numOpns to it. Now picture these options as being on a circle (e.g. an analog clock). decider is the clockwise distance from b to a. [Note: if this distance is even, then the distance from a to b is odd] For any particular option, half of the remaining options are an even distance and half are an odd distance. Here we arbitrary choose that odd distances correspond to a loss.

compVal = options[compChoice]['value']
userVal = options[userChoice]['value']
numOpns = len(options)
decider = (numOpns+userVal-compVal) % numOpns
result = 'win'

if decider == 0: result = 'tie'
elif decider%2 == 0: result = 'lose'
# else decider%2 == 1: result = 'win'

print('You '+result+' against the computer!\n')

In this case, hints are definitely a problem if hard coded. So here is a fix for that:

for k,v in options.iteritems():
    print(k.upper()+" or "+k+" for "+v['name'])
print('Q or q to quit')

And while we are at it, here is a options dictionary that simulates the correct Rock-Paper-Scissors-Lizard-Spock relationship.

options = {
    'r':{'name':'Rock','value':5},
    'l':{'name':'Lizard','value':4},
    'o':{'name':'Spock','value':3},
    's':{'name':'Scissors','value':2},
    'p':{'name':'Paper','value':1}
}
share|improve this answer
1  
You could probably replace your "magic" 3 with len(options). – Dubu 15 hours ago
1  
@Dubu The magic number works for 1 option and 3 options but fails for 5 and larger. Changing to len(options) won't improve this (and it will make it less readable). But abstracting away the 3 is a good idea. I added an 'advanced check' section which contains a generic solution for any odd number of options. – NonlinearFruit 13 hours ago
    
Is 'r':{'name':'Rock','value':1} a dictionary, within a dictionary? options = { } is the "parent" dictionary? – Richard Peña 12 hours ago
    
@RichardPeña Yes, it is a dictionary in a dictionary with options being the parent. It makes the code a little more readable. – NonlinearFruit 12 hours ago
    
@NonlinearFruit I am rewriting my code and am getting an error with a call similar to compChoice = random.choice(options.keys()) I should be able to yield the same results with comp_choice = {1: 'rock', 2: 'paper', 3: 'scissors'} comp_play = random.choice(comp_choice.keys()) – Richard Peña 12 hours ago

My second pass,

Improvements:

  1. Use list and string to find the user's and computer's move easily, instead of doing if-elses.
  2. Never trust user input. Verify that the user input is one of r, p, s or q.

    import random
    
    user = ''
    while user != 'q':
        print('Enter your choice')
        print('R or r for rock\nP or p for paper\nS or s for scissors\nQ or q to quit')
        user = input('')
        user = user.lower()
        choices = 'rps'
    
        if user == 'q':
            break
        if user not in choices:
            print 'invalid choice'
            continue
        user = choices.find(user)
    
        comp = random.randrange(0, 3)
        choice = comp
    
        choices = ['rock', 'paper', 'scissors']
        print('Computer picked:', move_choices[comp])
    
        # compare selection to figure out the winner
        if user == 0 and comp == 2:
            print('You win against the computer\n')
        elif user == 2 and comp == 0:
            print('You lose against the computer\n')
        elif user > comp:
            print('You win against the computer\n')
        elif user < comp:
            print('You lose against the computer\n')
        elif user == comp:
            print('You tie against the computer\n')
    
share|improve this answer
1  
'if choices.find(user) == -1': I find it more readable to write 'if user not in choices' – MKesper 22 hours ago
    
agreed and updated :) – Fallen 22 hours ago
1  
Your indentation seems to have become corrupted. – MKesper 22 hours ago

Apart from the if/elif improvements, you can also to some other things (here I take tips from all the answers to give you a good solution):

import random

print('Tip:\nR or r for rock\nP or p for paper\nS or s for scissors\nQ or q to quit')
while True:
    user = input('Enter you choice\n> ').lower()
    options = {'r': 1, 'p': 2, 's': 3}
    comp_trans = {1: 'rock', 2: 'paper', 3: 'scissors'}
    if user == 'q':
        break
    elif user not in options.keys():
        print('Invalid input, please re try')
        continue
    comp_choice = random.randint(1, 3)
    user_choice = options[user]
    print('Computer choosed:', comp_trans[comp_choice])
    if (user_choice, comp_choice) in {(1, 3), (2, 1), (3, 2)}:
        print('You won!')
    elif user_choice == comp_choice:
        print('You tied!')
    else:
        print('You lost!')

Changes:

1) Instead of giving tips every game, we give it once.

2) We dont need to check if user == 'q' each loop (in while statement) because you already do it in the loop

3) if you need only once a value, instead of creating a variable for it and using the variable directly use the value (in displaying the computer choose)

4) we need to check the input, or we may get a KeyError on the line

user_choice = options[user]

5) you asked what does the previous line do: it defines the variable user_choice to the value options[user]. options[user] returns the value in the dictionary options that has the key user. For example, if user = 'r', options[user] will return 1.

share|improve this answer
    
Welcome to Code Review, your first answer looks good, enjoy your stay! – ferada 19 hours ago
    
@ferada thanks! – BlackBeans 19 hours ago
    
How does if (user_choice, comp_choice) in {(1, 3), (2, 1), (3, 2)}: work? If user is: 1 while comp is 3, 2 while comp is 1, 3 while comp is 2? – Richard Peña 13 hours ago

Python has native lists, tuples and dictionaries (and a number of other data-structures). If you use the builtin data structures wisely you can often drastically simplify your code. In your case you are mapping back and forth between ints (1,2,3), characters ('r', 'p' and 's') and names ('rock',.. etc). Having the ints isn't necessary and the mapping from chars to names is better done with a dict.

import random

options = {"r": "rock", "p": "paper", "s": "scissors"}

while True:
    user = input('Choose r for rock, p for paper, s for scissors or q to quit: ')
    user = user.lower()

    if user == 'q':
        break

    if user not in options.keys():
        continue

    choice = random.choice(list(options.keys()))
    print('Computer picked:', options[choice])

    if choice == user:
        print('You tie against the computer\n')
    elif (user, choice) in (("r", "s"), ("p", "r"), ("s", "p")):
        print('You win against the computer\n')
    else:
        print('You lose against the computer\n')
share|improve this answer

Here, Dictionaries are used to make the selection of user and comp. This may remove the unnecessary comparisons.And the conditions which may lead to same results (win or loss) is being combined so it may compact the code.

import random
user = ''

while user != 'q':
    print('Enter your choice')
    print('R or r for rock\nP or p for paper\nS or s for scissors\nQ or q to quit')
    user = input('')
    user = user.lower()
    if user == 'q':
        break
    comp = random.randrange(1, 4)
    user_dict={'r':1,'p':2,'s':3}
    user=user_dict[user]
    comp_dict={1:'rock',2:'paper',3:'scissors'}
    choice=comp_dict[comp]

    print('Computer picked:', choice)

    # compare selection to figure out the winner
    if (user == 1 and comp == 3) or (user > comp):
        print('You win against the computer\n')
    elif (user == 3 and comp == 1) or (user < comp):
        print('You lose against the computer\n')
    elif user == comp:
        print('You tie against the computer\n')
share|improve this answer
2  
This site is aimed at improving by peer review, can you explain why your approach is an improvement so that the OP can learn from your thought processes. – forsvarir 21 hours ago
    
Updated in answer section..! – Chanda Korat 21 hours ago
    
Your code will give the wrong result for user==3, comp==1. – Dubu 15 hours ago

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.