10
\$\begingroup\$

I wrote a rock, paper, scissors program. It works fine, but I'm curious if there are any constructive critiques, tips, tricks, comments, or advice anyone may have on how the code operates, performs, looks, etc.

# This code is a PvC Rock-Paper-Scissors Game!
# Import Random Library for Computer Choice
import random

# Greeting & Introduction
print ("Hello! Let's play Rock-Paper-Scissors!");

# Just to Make things Smoother.
options = {1:'rock', 2:'paper', 3:'scissors'};
player_result = ["tie!", "win!", "lose!"];

# Obtain Player & CPU Inputs
def collection(user_choice = 42):
    # user_choice Collection
    while (user_choice not in ['1','2','3']):
        # Prompts the User for a number & Stores as raw_input
        print ("Pick a Number: \n 1) \t Rock \n 2) \t Scissors \n 3) \t Paper \n");
        user_choice = raw_input();
        # Checks to see if the choice is acceptable
        if (user_choice in ['1','2','3']):
            break;
        # If not acceptable, restart the loop (ask again).
        else:
            print("\nPlease pick either 1, 2, or 3!\n");

    # Convert user_choice to Int
    user_choice = int(user_choice);

    # choose comp_choice
    comp_choice = random.randint(1,3);
    return (user_choice, comp_choice);

# Translate Results into English
def translate_choices(user,comp):
    print ("\nYou chose " + options[user] +"!");
    print ("Computer chose " + options[comp] +"!");

# Comparison Function returns Game Results
def compare(x,y):
    result = player_result[ (x-y)%3 ];
    return result;

(user_choice,comp_choice) = collection();
translate_choices(user_choice,comp_choice);
result = compare (user_choice, comp_choice);
print ("You " + result);
\$\endgroup\$
13
\$\begingroup\$

From the top:

Imports

Since you're only using one thing from random, you might as well only import that one thing:

from random import randint

As PEP8 recommends, whenever the time comes order your imports by standard imports, then third-party imports, then lastly local imports.

Use of Parentheses

In Python 2, your print statements do not require parentheses. If you're on Python 3, print function calls require them. Other areas (such as your while loop and return statement for collections) do not require parentheses around them.

As a later note: You can use commas within print statements to have spaces between things or you can concatenate two strings with the + operator, e.g.

>>> print "Hello", "World!"
Hello World!
>>> print "Hello" + "World!"
HelloWorld!

I wouldn't mix the two though (might make things look messy), and this is just a helpful note on the difference.

Unnecessary Comments

A lot of your code is self-documenting already and do not require comments (which is a good thing).

You should only comment areas that you believe might be hard to understand, e.g. calculation for the game result.

If you end up using functions, you might want to add some helpful docstrings to describe what's going on, or rename the function e.g.

def calculate_winner(x, y):
    """<Explain how calculate_winner works here>."""
    #...etc...

Code Organization

A lot of your code doesn't seem to need to be in functions, e.g. translate_choices and compare. You only ever use them once, and its all related to your game.

This is a bit of personal preference, but it might be nicer to move all your code into a function and avoid having global variables. This way, if others end up importing your code they won't encounter conflicts.

def play_rps():
    # ... rest of your code here ...

Also, it might be good to make sure your code only runs if it's called as main, e.g.

if __name__ == '__main__':
    play_rps()

and being called via python rps_name.py or however the file name hosting your code may be.

More info on __main__ here.

Handling User Choice

Instead of making a list every time, why not use the keys from the dictionary you already made?

while user_choice not in options:
    # do user_choice stuff here

Also, to make the logic a bit simpler in the while loop, you can remove the break statement by only checking for invalid inputs:

while user_choice not in options:
    #...print prompt for user choice here...
    user_choice = raw_input()

    if user_choice not in options:
        print '\nPlease pick either 1, 2, or 3!\n'

Lastly, you can also merge the later conversion of raw_input into an integer into your while loop:

try:
    user_choice = int(raw_input())
except ValueError:
    # input was not a valid integer
    user_choice = None

if user_choice not in options:
    print '\nPlease pick either 1, 2, or 3!\n'

Unnecessary Semicolons

Do you come from a C++/Java background? Reason I ask is because you seem to have a lot of semicolons in your code (which aren't required for python).

End Product

With the suggestions I listed above (and a few small things such as splitting up your option prompt), here's the code I came up with:

from random import randint

def play_rps():
    print "Hello! Let\'s play Rock-Paper-Scissors!\n"

    options = {1:"rock", 2:"paper", 3:"scissors"}
    player_result = ["tie", "win", "lose"]

    user_choice = None

    # get user choice
    while user_choice not in options:
        print "Game options:"
        print "-------------"
        print "1) Rock"
        print "2) Scissors"
        print "3) Paper"

        try:
            user_choice = int(raw_input("Your choice: "))
        except ValueError:
            # input was not a valid integer
            user_choice = None

        if user_choice not in options:
            print "\nPlease pick either 1, 2, or 3!\n"

    # calculate computer choice
    computer_choice = randint(1, 3)

    print "\nYou chose: " + options[user_choice] + "!"
    print 'Computer chose: ' + options[computer_choice] + "!"

    # calculate winner
    result = player_result[ (user_choice-computer_choice) % 3]
    print 'You ' + result + '!'

if __name__ == '__main__':
    run_game = play_rps()

NOTE: With the above format, it also makes creating a loop (if you choose to have a replay option) much easier.

\$\endgroup\$
  • 1
    \$\begingroup\$ Why is get_user_choice a nested function? Also, PEP8 has a convention that makes the comments on imports unnecessary: separate blocks for standard lib, third party and local, in that order, with a blank line in between. The comments on why you import things would scale poorly if you use multiple names or have multiple uses; if you only use one, the from x import y form keeps your namespace clean. There is a certain irony to suggesting the addition of unnecessary comments to the imports then dinging the OP on those they already had! \$\endgroup\$ – jonrsharpe Mar 13 '17 at 7:50
  • \$\begingroup\$ I figured get_user_choice should be a nested function since it makes folding the code (in an editor such as Sublime) easier to manage. You're right on the irony (I didn't notice that until now, hah). Just finished making edits. \$\endgroup\$ – Joe Greene Mar 13 '17 at 8:13
  • \$\begingroup\$ I'm not seeing the option to edit my last comment here, but thanks for the notes @jonrsharpe \$\endgroup\$ – Joe Greene Mar 13 '17 at 8:21
  • 3
    \$\begingroup\$ Did not know about "Hello", "World" would automatically add a space! Learn something new everyday. \$\endgroup\$ – alexyorke Mar 13 '17 at 15:00
  • \$\begingroup\$ @alexy13 There is a lot of different stuff you can do in regards to Python's print statements. Good list here: stackoverflow.com/questions/21542694/… \$\endgroup\$ – Joe Greene Mar 13 '17 at 21:10
5
\$\begingroup\$

In general this code looks pretty good. I only have a couple of small comments.

Pep8:

Python has a strong idea of how the code should be styled, and it is expressed in pep8.

I suggest you get a style/lint checker. I use the pycharm ide which will show you style and compile issues right in the editor.

Unneeded else:

This if:

# Checks to see if the choice is acceptable
if (user_choice in ['1','2','3']):
    break;
# If not acceptable, restart the loop (ask again).
else:
    print("\nPlease pick either 1, 2, or 3!\n");

...could be:

# Checks to see if the choice is acceptable
if (user_choice in ['1','2','3']):
    break

# If not acceptable, restart the loop (ask again).
print("\nPlease pick either 1, 2, or 3!\n")

since the break means the else is not needed.

\$\endgroup\$
0
\$\begingroup\$

In addition to the other great feedback here, I'd like to recommend extracting the user input into a separate function. This part of the code is non-trivial, and its complexity obscures the game code.

So I'd suggest a function such as:

OPTIONS = {1: "rock", 2: "paper", 3: "scissors"}


def get_user_move():
    """Ask the user for a move until receiving valid input"""
    while True:
        # Prompts the User for a number & Stores as raw_input
        print("Pick a Number:")
        for possible_move in OPTIONS.items():
            print(" %s) \t %s" % possible_move)

        user_choice = raw_input()
        try:
            # If the choice is acceptable, we're done
            if (int(user_choice) in OPTIONS):
                return int(user_choice)
        except ValueError:
            # Catch inputs that cannot be converted to an int
            pass
        # Keep asking again
        print("\nThe choice '%s' is invalid" % user_choice)

...

Also note that now that the options dict is used in multiple functions; the convention is to make it a module-level constant.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.