Python rock paper scissors

Question

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.

Answer 1

Single Responsibility Principle

choice() should do one thing: get the player's choice. It does three things: gets the player's choice, picks a choice for the computer, and then plays.

If we just drop the other two, we'd get down to:

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

However, making this recursive isn't great. We could just run out of stack space. There's no advantage here. Let's just loop until done:

def choice():
    while True:
        player_choice = raw_input('Choose rock, paper or scissors: ')
        if player_choice.lower() in choices:
            return player_choice

It would be better to flip the namings here too, and call the function player_choice and the variable choice.

Similarly, play() does too many things. Ideally, you'd want to structure your main like:

while True:
    player = player_choice()    ## JUST grab the player's choice
    cpu = computer_choice()     ## JUST grab the CPU's choice
    show_results(player, cpu)   ## JUST print the outcome

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

Determining Success

Right now, your win_table is explicitly listing all 9 cases. That's super repetitive. First of all, the message is the same regardless of which win case we're in, so you could simplify the win_table to actually be a table of wins:

win_table = {'rock': ['scissors'],
             'scissors': ['paper'],
             'paper': ['rock']}

if cpu in win_table[player]:
    # You win!

Or we can do this programmaticly. Rock, Paper, Scissors is a wheel. Rock > Paper > Scissors > Rock. We win if the opponent's choice is the next one along in the wheel.

That is:

if player == cpu:
    # Tie
else:
    idx = choices.index(player)
    if cpu == choices[(idx + 1) % len(choices)]:
        # We win
    else:
        # We lose

Answer 2

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.