Code Review for Paper, Rock, Scissors

[Chris Angelico]

On Tue, Oct 14, 2014 at 7:04 PM, Revenant
<faceofoblivionofficial at gmail.com> wrote:
> 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!

> # Creates the main menu.
> def 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.

>     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()

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:").

> def play():
>     # 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/
)

>     import random

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

> # 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()

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.

> # Creates the about section.
> def about():
>     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> :)

> # 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()

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