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

Python rock paper scissors

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

Problem

My code implementing rock, paper and scissors in Python, using dicts to hold choices.

import random

choices = ['rock', 'paper', 'scissors']

def choice():

    player_choice = raw_input('Choose rock, paper or scissors: ')
    if player_choice.lower() in choices:
        computer_choice = random.choice(choices)
        play(player_choice, computer_choice)
    else:
        choice()

def play(p_choice, c_choice):

    win_table = {'rock' : {'rock':'It was a Tie', 'paper':'You Lose', 'scissors':'You Win'},
                 'paper' : {'rock':'You Win', 'paper':'It was a Tie', 'scissors':'You Lose'},
                 'scissors' : {'rock':'You Lose', 'paper':'You Win', 'scissors':'It was a Tie'}}

    print 'Computer chose', c_choice
    print win_table[p_choice][c_choice]

    new = raw_input('Play again ? (Y/N):  ')
    if new.lower() == 'y': 
        choice()
    else:
        print 'Have a nice day !'

choice()


win_table dictionary names are player choices and dictionary keys are computer choices, seems more efficient to me. I haven't run any memory usage tests, though.

Solution

The biggest problem with this code is that you are using functions as glorified GOTO labels, when choice() is called from within choice, when choice() is called from play, and when choice() is called initially. If you hit CtrlC after a few rounds, you'll see that the call stack is inappropriately deep. The remedy is to use functions, return values, and loops properly.

The other change I would suggest is removing the redundancy between the choices list and the win_table.

import random

MOVES = {
    'rock': {'rock':'It was a Tie', 'paper':'You Lose', 'scissors':'You Win'},
    'paper': {'rock':'You Win', 'paper':'It was a Tie', 'scissors':'You Lose'},
    'scissors': {'rock':'You Lose', 'paper':'You Win', 'scissors':'It was a Tie'},
}

def choices():
    """Produce a tuple of the player's choice and the computer's choice."""
    while True:
        player_choice = raw_input('Choose rock, paper or scissors: ')
        if player_choice.lower() in MOVES:
            computer_choice = random.choice(MOVES.keys())
            return player_choice, computer_choice

def reveal(p_choice, c_choice):
    """Print both players' choices and the result of the game."""
    print 'Computer chose', c_choice
    print MOVES[p_choice][c_choice]

def play():
    """Play a round of rock-paper-scissors, then repeat if desired."""
    while True:
        reveal(*choices())
        if raw_input('Play again? (Y/N):  ').lower() != 'y':
            break
    print 'Have a nice day!'

play()


There isn't much benefit to having choices() produce both the player's choice and the computer's choice, though. You should break that up into two functions.

Code Snippets

import random

MOVES = {
    'rock': {'rock':'It was a Tie', 'paper':'You Lose', 'scissors':'You Win'},
    'paper': {'rock':'You Win', 'paper':'It was a Tie', 'scissors':'You Lose'},
    'scissors': {'rock':'You Lose', 'paper':'You Win', 'scissors':'It was a Tie'},
}

def choices():
    """Produce a tuple of the player's choice and the computer's choice."""
    while True:
        player_choice = raw_input('Choose rock, paper or scissors: ')
        if player_choice.lower() in MOVES:
            computer_choice = random.choice(MOVES.keys())
            return player_choice, computer_choice

def reveal(p_choice, c_choice):
    """Print both players' choices and the result of the game."""
    print 'Computer chose', c_choice
    print MOVES[p_choice][c_choice]

def play():
    """Play a round of rock-paper-scissors, then repeat if desired."""
    while True:
        reveal(*choices())
        if raw_input('Play again? (Y/N):  ').lower() != 'y':
            break
    print 'Have a nice day!'

play()

Context

StackExchange Code Review Q#111684, answer score: 5

Revisions (0)

No revisions yet.