Code Review for Paper, Rock, Scissors

[Revenant]

Hi all!

I'm new to Python and programming in general, and am trying to learn as much as I can about it.

Anyway, for a basic first program I made a simple game of Paper, Rock, Scissors. For this program, I incorporated a main menu that presented three different options, allowed the user to play a game of Paper, Rock, Scissors, allowed them to play the game again, and most importantly checked the user's input to make sure the program doesn't fail due to errors.

One thing I want to try to add is a "Press any key to continue" function that occurs at the end of the program when the user decides to quit. I looked at some options online, but haven't quite figured it out yet.

As previously stated, I am new to Python and would also like to see if any of you programming gurus have some suggestions about how I can simplify code, and also if there are any other good starter programs to work on to improve my skills.

Thanks for reading! Code is below:


# Creates the main menu.
def menu():
# Sets the scores to 0.
global playerscore
global compscore
global draws
playerscore = 0
compscore = 0
draws = 0
menuselection = input('Please enter a selection: (Play/Help/About): ')
# Checks for an invalid selection.
while menuselection != 'Play' and menuselection != 'play' and menuselection != 'Help' and menuselection != 'help' \
and menuselection != 'About' and menuselection != 'about':
print('You have entered an invalid selection.')
menuselection = input('\nPlease type a selection: (Play/Help/About): ')
else:
if menuselection == 'Play' or menuselection == 'play':
play()
elif menuselection == 'Help' or menuselection == 'help':
instructions()
else:
about()


# Creates the game.
def play():
global playerscore
global compscore
global draws
# Player chooses Paper, Rock, or Scissors.
playerselect = input('\nPlease choose Paper, Rock, or Scissors: ')
# Checks for an invalid selection.
while playerselect != 'Paper' and playerselect != 'paper' and playerselect != 'Rock' and playerselect != 'rock' \
and playerselect != 'Scissors' and playerselect != 'scissors':
print('You have entered an invalid selection.')
playerselect = input('\nPlease choose Paper, Rock, or Scissors: ')
else:
if playerselect == 'Paper' or playerselect == 'paper':
print('\nYou have selected Paper.')
playerselect = 1
elif playerselect == 'Rock' or playerselect == 'rock':
print('\nYou have selected Rock.')
playerselect = 2
else:
print('\nYou have selected Scissors.')
playerselect = 3
# Computer chooses Paper, Rock, or Scissors.
import random
compselect = random.randint(1, 3)
if compselect == 1:
print('The Computer has selected Paper')
elif compselect == 2:
print('The Computer has selected Rock.')
else:
print('The Computer has selected Scissors.')
# Results if player selects paper.
if playerselect == 1 and compselect == 1:
print('Draw!')
draws += 1
score()
else:
if playerselect == 1 and compselect == 2:
print('Paper beats rock. You win!')
playerscore += 1
score()
elif playerselect == 1 and compselect == 3:
print('Paper is beaten by scissors. You lose!')
compscore += 1
score()
# Results if player selects rock.
if playerselect == 2 and compselect == 2:
print('Draw!')
draws += 1
score()
else:
if playerselect == 2 and compselect == 1:
print('Rock is beaten by paper. You lose!')
compscore += 1
score()
elif playerselect == 2 and compselect == 3:
print('Rock beats scissors. You win!')
playerscore += 1
score()
# Results if player selects rock.
if playerselect == 3 and compselect == 3:
print('Draw!')
draws += 1
score()
else:
if playerselect == 3 and compselect == 1:
print('Scissors beat paper. You win!')
playerscore += 1
score()
elif playerselect == 3 and compselect == 2:
print('Scissors are beaten by rock. You lose!')
compscore += 1
score()
again()


# Determines if the player wants to play another game.
def again():
replay = input('\nDo you want to play again (Y/N)? ')
while replay != 'Y' and replay != 'y' and replay != 'N' and replay != 'n':
print('You have entered an invalid selection.')
replay = input('\nDo you want to play again (Y/N)? ')
else:
if replay == 'Y' or replay == 'y':
play()
else:
print('\nThanks for playing!')


# Creates the instructions.
def instructions():
print('\nPaper, Rock, Scissors is a simple game played against a computer opponent.')
print('The player will have a choice of selecting paper, rock, or scissors.')
print('The player\'s result will be compared with that of the computer to determine who wins the round.')
print('In the event that both the player and the computer have the same selection, the round will end in a tie.')
print('\nPaper beats rock but loses to scissors.')
print('\nRock beats scissors but loses to paper.')
print('\nScissors beats paper but loses to rock.')
print('\nGood luck, and have fun!\n')
menu()


# Creates the about section.
def about():
print('\nPaper, Rock, Scissors\n\nVersion 1.0\n\nCreated by <Name>, 07 October 2014\n')
menu()


# Calculates score.
def score():
print('\nCurrent Scores: ')
print('\nPlayer Score:', playerscore)
print('\nComputer Score:', compscore)
print('\nDraws:', draws)


# Start of program operations.
print('Welcome to Paper, Rock, Scissors!\n')
menu()

[Glenn Hutchings]

Hi there! Welcome to Python.

Post by Revenant
I am new to Python and would also like to see if any of you programming
gurus have some suggestions about how I can simplify code, and also if
there are any other good starter programs to work on to improve my
skills.

I'm sure you'll get a lot of helpful advice here. Here's a start: you have
a lot of places where you're comparing variables to the capitalized and
lower-case versions of the same string. You could halve that number if you
converted your input to lowercase first. For example:

if menuselection == 'play' or menuselection == 'Play':

changes to:

if menuselection.lower() == 'play':

There are plenty of other string methods you might find useful.

[Dave Angel]

Revenant <faceofoblivionofficial at gmail.com> Wrote in message:
....

Post by Revenant
One thing I want to try to add is a "Press any key to continue" function that occurs at the end of the program when the user decides to quit. I looked at some options online, but haven't quite figured it out yet.

Use the curses function getch, as documented on:
https://docs.python.org/3/howto/curses.html

Or if you're on Windows, try msvcrt.getch.

If you're writing code that needs to be portable, look at:
http://code.activestate.com/recipes/577977-get-single-keypress/

Post by Revenant
As previously stated, I am new to Python and would also like to see if any of you programming gurus have some suggestions about how I can simplify code, and also if there are any other good starter programs to work on to improve my skills.
# Creates the main menu.
# Sets the scores to 0.
global playerscore
global compscore
global draws
playerscore = 0
compscore = 0
draws = 0
menuselection = input('Please enter a selection: (Play/Help/About): ')

Save yourself trouble by using the lower method on the result of
each input function. I'll assume that for subsequent comments.

Post by Revenant
# Checks for an invalid selection.
while menuselection != 'Play' and menuselection != 'play' and menuselection != 'Help' and menuselection != 'help' \
print('You have entered an invalid selection.')
menuselection = input('\nPlease type a selection: (Play/Help/About): ')
play()
instructions()
about()

func = {"play":play, "help":instructions, "about":about}
func[menuselection]()

Post by Revenant
# Creates the game.
global playerscore
global compscore
global draws
# Player chooses Paper, Rock, or Scissors.
playerselect = input('\nPlease choose Paper, Rock, or Scissors: ')
# Checks for an invalid selection.
while playerselect != 'Paper' and playerselect != 'paper' and playerselect != 'Rock' and playerselect != 'rock' \

Again use playerselect not in ("pap... form

Post by Revenant
print('You have entered an invalid selection.')
playerselect = input('\nPlease choose Paper, Rock, or Scissors: ')
print('\nYou have selected Paper.')
playerselect = 1
print('\nYou have selected Rock.')
playerselect = 2
print('\nYou have selected Scissors.')
playerselect = 3
# Computer chooses Paper, Rock, or Scissors.
import random
compselect = random.randint(1, 3)

You could make life a little easier by using 0 to 2 instead, here
and above.

Post by Revenant
print('The Computer has selected Paper')
print('The Computer has selected Rock.')
print('The Computer has selected Scissors.')

print (None, "paper", "rock", "scissors")[compselect])

The next section can be simplified a lot by exploiting some
symmetries.
diff = (playerselect - compselect ) % 3

This number will be zero for a draw, 1 for computer win, and 2 for
player win. So you can have 3 cases below instead of
9.

Post by Revenant
# Results if player selects paper.
print('Draw!')
draws += 1
score()
print('Paper beats rock. You win!')
playerscore += 1
score()
print('Paper is beaten by scissors. You lose!')
compscore += 1
score()
# Results if player selects rock.
print('Draw!')
draws += 1
score()
print('Rock is beaten by paper. You lose!')
compscore += 1
score()
print('Rock beats scissors. You win!')
playerscore += 1
score()
# Results if player selects rock.
print('Draw!')
draws += 1
score()
print('Scissors beat paper. You win!')
playerscore += 1
score()
print('Scissors are beaten by rock. You lose!')
compscore += 1
score()
again()

This makes me a little nervous. You have mutual recursion going on
between functions play and again. It probably won't matter here,
but it's a bad habit to get into.

What you really should do is something like

def play_once (): (current body of play function, but without
ref to again)

def play ():
while True:
play_once ()
if not again():
break
And change again so it returns True or False

Post by Revenant
# Determines if the player wants to play another game.
replay = input('\nDo you want to play again (Y/N)? ')
print('You have entered an invalid selection.')
replay = input('\nDo you want to play again (Y/N)? ')
play()

Replace the call to play () with
return True

Post by Revenant
print('\nThanks for playing!')

return False

Post by Revenant
# Creates the instructions.
print('\nPaper, Rock, Scissors is a simple game played against a computer opponent.')
print('The player will have a choice of selecting paper, rock, or scissors.')
print('The player\'s result will be compared with that of the computer to determine who wins the round.')
print('In the event that both the player and the computer have the same selection, the round will end in a tie.')
print('\nPaper beats rock but loses to scissors.')
print('\nRock beats scissors but loses to paper.')
print('\nScissors beats paper but loses to rock.')
print('\nGood luck, and have fun!\n')
menu()
# Creates the about section.
print('\nPaper, Rock, Scissors\n\nVersion 1.0\n\nCreated by <Name>, 07 October 2014\n')
menu()
# Calculates score.
print('\nCurrent Scores: ')
print('\nPlayer Score:', playerscore)
print('\nComputer Score:', compscore)
print('\nDraws:', draws)
# Start of program operations.
print('Welcome to Paper, Rock, Scissors!\n')
menu()

[Chris Angelico]

On Tue, Oct 14, 2014 at 7:04 PM, Revenant

Post by Revenant
As previously stated, I am new to Python and would also like to see if any of you programming gurus have some suggestions about how I can simplify code, and also if there are any other good starter programs to work on to improve my skills.

Hi! Happy to help out. But first, there's one meta-suggestion that I'd
like to make: Use something other than Google Groups. As shown by the
line above, Google Groups has some issues with line breaks and text
wrapping; and it gets much worse when you reply to someone else's
post. I suggest using the mailing list instead:

https://mail.python.org/mailman/listinfo/python-list

Also, it can be helpful to state what version of Python you're using,
and on what platform. Your "press any key to quit" request suggests
you're probably invoking the script using a GUI, quite possibly
Windows, but that's far from certain; and as Dave's response shows,
the solutions depend on platform. I'm presuming you're using some 3.x
version of Python, as you use input() and print() functions, but I
can't tell whether you're on 3.2, 3.3, 3.4, or even an unreleased 3.5.
It's not likely to make a huge amount of difference with a script this
simple, but as a general rule, more information is better than less.

So, on to the code!

Post by Revenant
# Creates the main menu.
# Sets the scores to 0.
global playerscore
global compscore
global draws
playerscore = 0
compscore = 0
draws = 0

You're importing these globals into several places that don't need
them. The menu shouldn't have to concern itself with these scores; you
could initialize them all to zero at top-level, and then keep the
different functions more self-contained. The displaying of the menu
doesn't logically involve zeroing out the scores; imagine making the
very slight (and quite logical) change of having the full menu
redisplayed after each game, instead of just having the "Again? (Y/N)"
prompt - suddenly your scores aren't getting carried over.

Post by Revenant
menuselection = input('Please enter a selection: (Play/Help/About): ')
# Checks for an invalid selection.
while menuselection != 'Play' and menuselection != 'play' and menuselection != 'Help' and menuselection != 'help' \
print('You have entered an invalid selection.')
menuselection = input('\nPlease type a selection: (Play/Help/About): ')
play()
instructions()
about()

The "else" clause on a while loop isn't necessary here, because you're
not using break. All you need is to put your code outside the loop.
Alternatively, you can combine the verification and iteration into one
pass, which reduces redundancy and makes it easier to add more
options. I'm also incorporating some suggestions already made,
including Dave's dispatch dictionary.

func = {"play":play, "help":instructions, "about":about}
while True:
menuselection = input('Please enter a selection:
(Play/Help/About): ').lower()
if menuselection in func: break
print('You have entered an invalid selection.')
func[menuselection]()

Note how the loop condition is now inside the loop ("if ...: break"),
with the while statement simply creating an infinite loop ("while
True:").

Post by Revenant
# Player chooses Paper, Rock, or Scissors.
playerselect = input('\nPlease choose Paper, Rock, or Scissors: ')

Try to avoid comments that simply state what the next line(s) of code
do. At best, they're redundant; at worst, they're confusing, because
they can get out of sync with the code. Imagine adding a fourth option
(Claw Hammer) and forgetting to change the comment.

(Why claw hammer?
http://www.theregister.co.uk/2003/06/09/the_bastard_interviewer_from_hell/
)

Post by Revenant
import random

It's much more common to put your imports at the top of the script,
rather than inside a function.

Post by Revenant
# Creates the instructions.
print('\nPaper, Rock, Scissors is a simple game played against a computer opponent.')
print('The player will have a choice of selecting paper, rock, or scissors.')
print('The player\'s result will be compared with that of the computer to determine who wins the round.')
print('In the event that both the player and the computer have the same selection, the round will end in a tie.')
print('\nPaper beats rock but loses to scissors.')
print('\nRock beats scissors but loses to paper.')
print('\nScissors beats paper but loses to rock.')
print('\nGood luck, and have fun!\n')
menu()

As noted elsewhere, you have mutual recursion happening here. That's
not an advised technique in Python, as you can blow your stack
(there's no tail call optimization here). Much more common would be to
have self-contained functions that perform one job (in this case,
displaying the instructions on the console), and have looping done in
the places that want looping - probably inside menu().

Incidentally, this function doesn't *create* anything. Your comment
(which would be better as a docstring, fwiw) is slightly confusing
here.

Post by Revenant
# Creates the about section.
print('\nPaper, Rock, Scissors\n\nVersion 1.0\n\nCreated by <Name>, 07 October 2014\n')
menu()

Huh, I didn't know your name was actually <Name> :)

Post by Revenant
# Calculates score.
print('\nCurrent Scores: ')
print('\nPlayer Score:', playerscore)
print('\nComputer Score:', compscore)
print('\nDraws:', draws)
# Start of program operations.
print('Welcome to Paper, Rock, Scissors!\n')
menu()

Suggestion: Where possible, follow a general policy of "Define Before
Use". For any given global name (like function names), the first
occurrence in the file should be its definition, and all usage should
be lower in the file than that. Once you clean up the mutual
recursion, you'll be able to lay your code out like this; there'll be
a single menu() function near the bottom, and functions like score()
will be much higher up, as they have almost no dependencies. It'd look
something like this (function bodies and blank lines elided for
brevity):

import random
playerscore = compscore = draws = 0
def instructions():
def about():
def score():
def again(): # which will return True or False, as per Dave's suggestion
def play():
def menu():
print("Welcome")
menu()

When someone comes looking at a program like this, s/he can see
exactly what depends on what. There can be exceptions, of course
(sometimes mutual recursion really is the right thing to do,
especially with tree-walking routines), but those can then be noted
with comments ("Mutually recursive with xyz()."), or in some cases may
be obvious from the names. In any case, the bulk of most programs can
be written in this style, and it does improve clarity.

Hope that's of value. Like all advice, it's just one person's opinion,
and you're most welcome to reject it; but you did ask for code review,
so I'm hoping you'll at least know *why* you're rejecting any piece of
advice :) All the best!

ChrisA

[Larry Hudson]

Post by Revenant
Hi all!
I'm new to Python and programming in general, and am trying to learn as much as I can about it.
Anyway, for a basic first program I made a simple game of Paper, Rock, Scissors. For this program, I incorporated a main menu that presented three different options, allowed the user to play a game of Paper, Rock, Scissors, allowed them to play the game again, and most importantly checked the user's input to make sure the program doesn't fail due to errors.
One thing I want to try to add is a "Press any key to continue" function that occurs at the end of the program when the user decides to quit. I looked at some options online, but haven't quite figured it out yet.
As previously stated, I am new to Python and would also like to see if any of you programming gurus have some suggestions about how I can simplify code, and also if there are any other good starter programs to work on to improve my skills.

[snip]

You've already received a lot of good advice, which I'm not going to repeat here. Rather, I'm
going to show you the version that I wrote some time ago. Whether this version is
good/bad/indifferent is up to someone else to say, but I'm showing it here just as an example of
a different approach that you might find interesting to study. Although, being new to Python,
some of the things here will probably be unfamiliar to you, particularly the new-style string
formatting which I use a lot.

The key to this approach is to recognize that there are only nine possible combinations. I
handle these in the get_result() function by stitching together pieces of strings in a
dictionary. This function is interesting because, although it has a total of 34 lines, 17 are
comments or blanks, 16 are data definitions, and only 1 line is actual code.

The get_rps() function is used to get the player's rock/paper/scissor choice. It also allows
'quit' as a fourth choice. This makes your proposed "Press any key..." unnecessary, because
this version will continue automatically until you specifically tell it to stop. Also it
accepts an empty input as the same as a 'quit' choice. Whether this is good or bad is
debatable, but it can easily be written either way. It returns the choice as a single
lower-case character, 'r', 'p', 's' or 'q'.

This version also makes heavy use of the constants WIN/LOSE/DRAW, which are defined as globals
at the top of the program. They help the clarity of the source. Normal convention is to make
the names of constants all upper-case. This is a convention only, not a requirement. The
scores are tracked in a three-element list, which use the WIN/LOSE/DRAW constants as the
indexes. The instruction display in my version is very short, but it could easily be replaced
with a longer version like yours.

FWIW, here is my code...
Note the first line is Linux-specific -- Windows will ignore it, or you can remove it.

#------------ Code starts here ------------
#!/usr/bin/env python3

# rps.py -- Rock, Paper, Scissors game

from random import choice

# Global constants
WIN = 0
LOSE = 1
DRAW = 2

def get_result(h, c):
"""Determine who wins this round

Parameters:
h: Human's selection (r, p or s)
c: Computer's selection (r, p or s)

Returns a tuple of the WIN/LOSE/DRAW value
and the string describing the results
"""

# Strings used in results
pr = 'Paper covers Rock. {}'
rs = 'Rock smashes Scissors. {}'
sp = 'Scissors cuts Paper. {}'
ti = 'We both have {}. {}'

# Win/lose/draw strings
wins = ('You win', 'I win', "It's a draw")

# Dictionary defining the results
res = {
'rr' : (DRAW, ti.format('rocks', wins[DRAW])),
'rp' : (LOSE, pr.format(wins[LOSE])),
'rs' : (WIN, rs.format(wins[WIN])),
'pr' : (WIN, pr.format(wins[WIN])),
'pp' : (DRAW, ti.format('paper', wins[DRAW])),
'ps' : (LOSE, sp.format(wins[LOSE])),
'sr' : (LOSE, rs.format(wins[LOSE])),
'sp' : (WIN, sp.format(wins[WIN])),
'ss' : (DRAW, ti.format('scissors', wins[DRAW]))
}

# Return the result tuple
return res[h + c]

def get_rps():
"""Get Rock/Paper/Scissor choice."""
while True:
select = input('\nDo you choose Rock, Paper or Scissors ').lower()
# Check for empty input or quit command
if not select or select in ['q', 'quit']:
return 'q' # return quit code
# Check for valid input
if select in ['r', 'rock', 'p', 'paper', 's', 'scissors']:
return select[0] # return first character
print('What did you say?? Try again please')

#================= Main Program starts here ==============
# Keep track of results:
# scores[0] = number of human wins
# scores[1] = number of computer wins
# scores[2] = number of draws
scores = [0] * 3
things = {'r':'a rock', 'p':'paper', 's':'scissors'}

print("Let's play a game or Rock, Paper, Scissors.\n")
print('Enter "r", "p", "s", "rock", "paper", or "scissors" for your choice.')
print('Use empty input, "q" or "quit" to end the game.')

while True:
computer = choice('rps') # Computer selects
human = get_rps() # Human selects
if human == 'q':
break
print('You have {}, I have {}. '.format(
things[human], things[computer]), end='')
scr, res = get_result(human, computer)
scores[scr] += 1 # Count win/loss/draw
print(res) # And show results

# Show final scores
print('\nTotal scores:')
print('\tYou won {} games'.format(scores[WIN]))
print('\tComputer won {} games'.format(scores[LOSE]))
print('\tThere were {} tie games'.format(scores[DRAW]))
print('\nThanks for playing with me. Bye now.')

#------------ End of code --------------

-=- Larry -=-