Python 3.6 Rock-Paper-Scissors game

Question

I was asked to write a "Rock Paper Scissors" game as a practice exercise for class. I ended up with this code which works as intended. It's a one player vs. the computer game.

Overview:

• Computer and player choose their 'weapons'
• Decide the winner
• Print the winner

Does anyone have any constructive feedback or tips on how I could improve this?

import random as r
#the possible moves in rock paper scissors
RPS = ["ROCK", "PAPER", "SCISSORS"]
def main():
 comp = computerMove()
 player = playerMove()
 winner, tie = calcWinner(comp, player)
 dispWinner(winner, tie)
#the move made by the computer
def computerMove():
 comp = r.choice(RPS)
## print(comp)
 return comp
#move made by the user
def playerMove():
 player = ""
 while not player in RPS:
 player = input("Choose your weapon(rock, paper, or scissors: ").upper()
## print(player)
 return player
#determining a winner
def calcWinner(comp, player):
 print("The computer chooses {0}.".format(comp))
 winner = None
 tie = False
## if theres a tie
 if player == comp:
 print("No winner.... Restarting....\n")
 main()
 tie = True
## if the user wins 
 elif ((comp == "ROCK") and (player == "PAPER")) or ((comp=="PAPER") and (player == "SCISSORS")) or ((comp=="SCISSORS") and (player == "ROCK")):
 winner = True
## if computer won
 else:
 winner = False
 return winner, tie
def dispWinner(winner, tie):
 if tie == True:
 print("")
 elif winner == True:
 print("You beat the computer!")
 elif winner == False:
 print("The computer beat you!")
main()

• 200_success: Please see What to do when someone answers . I have rolled back Rev 3 → 2.

Answer 1

First of all, welcome to codereview! It's nice to see that you are attending Python courses at University / highschool.

---

Styling

As you're at the beginning of the road, you should know that Python has a style-guide which is called PEP8. You can read there things like:

1. Naming conventions: function / variable names should be lower_cased
2. Comments: you should add a space after # (a single one is enough, it's not necessary to abuse it) when you comment out the code, you should avoid obvious comments and also try to write docstrings to each function
3. Maximum line length: lines should have a maximum length of 79 characters (this is debatable as some editors - like PyCharm suggests a maximum of 120).
4. Spacing: you should have two new lines between methods, and one whitespace around operators

That being said, your code after some styling might look like this:

import random as r
RPS = ["ROCK", "PAPER", "SCISSORS"]
def main():
 """This is the main function"""
 comp = computer_move()
 player = player_move()
 winner, tie = calc_winner(comp, player)
 disp_winner(winner, tie)
def computer_move():
 """The move made by the computer."""
 comp = r.choice(RPS)
 return comp
def player_move():
 """The move made by the user."""
 player = ""
 while not player in RPS:
 player = input("Choose your weapon(rock, paper, or scissors: ").upper()
 return player
def calc_winner(comp, player):
 """Determining a winner."""
 print("The computer chooses {0}.".format(comp))
 winner = None
 tie = False
 if player == comp:
 print("No winner.... Restarting....\n")
 main()
 tie = True
 elif (((comp == "ROCK") and (player == "PAPER")) or 
 ((comp == "PAPER") and (player == "SCISSORS")) or 
 ((comp == "SCISSORS") and (player == "ROCK"))):
 winner = True
 else:
 winner = False
 return winner, tie
def disp_winner(winner, tie):
 """Display the winner of the game."""
 if tie == True:
 print("")
 elif winner == True:
 print("You beat the computer!")
 elif winner == False:
 print("The computer beat you!")
main()

---

More about styling

Keep in mind that your comments should be succinct and explicit and should emphasise what the method does.

I usually put the main() function after the others because it comes more naturally when you read the code.

Give descriptive names to your functions / variables. Doing this will make the code speak for itself (more readable) and will make you remove unnecessary comments.

You only use random once. Don't give it an alias.

---

Equality vs Identity Comparisons

is is identity testing, == is equality testing.

You should use:

if tie is True:
 ...

Instead of:

if tie == True:
 ...

Or shorted:

if tie:
 ...

! Remember that if you use is, the result will be true if and only if the object is the same object. However, == will be true any time the values of the object are the same.

---

Testing for membership

You should write:

while player not in RPS:
 ...

Instead of:

while not player in RPS:
 ...

---

Tuples vs Lists

Because your RPS is not changing, I'd recommend you use a tuple instead. A couple of advantages of doing the above:

1. Tuples are faster than lists. If you're defining a constant set of values and all you're ever going to do with it is iterate through it, use a tuple instead of a list.
2. It makes your code safer if you "write-protect" data that does not need to be changed. Using a tuple instead of a list is like having an implied assert statement that this data is constant, and that special thought (and a specific function) is required to override that.

---

Fix the bug

If the computer chooses scissors and you choose paper there won't be any result because you only test for half of the cases.

Choose your weapon(rock, paper, or scissors: paper
The computer chooses SCISSORS.
Process finished with exit code 0

I'll let you fix this. HINT (think of a suitable data structure that can make the things easier)

Double hint: maybe a dictionary?

• qazwsx598: First off thank you for your very detailed comment. I feel I learned a great deal. I had never heard of PEP8 and the function/variable naming conventions I was using I actually learned from my prof and textbook! I cleaned up my code based on your recommendations and I think I'm in-line with the PEP8 format now. I'll post my revised code down below if you'd like to look over it. Secondly, I wasn't able to recreate your issue of the program returning no result when I chose paper and it chose scissors. The program only ended after printing either myself or the computer as the winner.
• Daniel: <expr> is True should actually be avoided in most situations. If a function returns a boolean, equality tests should be if <expr> / if not <expr>.

Answer 2

Don't call main

Make two changes, first:

def computerMove():
 return "ROCK"

And:

def dispWinner(winner, tie):
 if tie == True:
 print("TIE!!!!") # <---- actually print something
 // etc ... same as before

Now play and enter ROCK ... does it scream TIE!!!! at you? No. Because you call main in calcWinner. This has two problems:

• It makes it harder to follow the control flow of your program. That's because one does not expect a "lower level" (in terms of program architecture) function like calcWinner to call a "higher level" function.
• It builds up unnecessary stack depth. Very dedicated users (or if you decide to run this as some server application one day) will first eat up your memory and then crash the program (stack overflow).

PEP8 and stuff

Indentation

Use 4 spaces per indentation level.

def computerMove():
...comp = r.choice(RPS)
...return comp

Here, you only use 3 spaces. Use 4. Everywhere.

Blank lines

Surround top-level function and class definitions with two blank lines.

Please do so.

Use blank lines in functions, sparingly, to indicate logical sections.

You use blank lines to separate parts of calcWinner. I wouldn't here, because it hurts readability IMO too much.

Function names

Function names should be lowercase, with words separated by underscores as necessary to improve readability.

Apart from this PEP rule, IMO function names should reflect what is being done, i.e. an action. Thus not computerMove but rather obtain_computers_move, not playerMove but ask_player_for_move.

Comments

Comments should be complete sentences.

This means one or more sentences! Why? Because they should explain why you wrote something the way you did, not what the code is doing!.

Generally speaking your comments don't help in any way, they just hurt readability. Better remove them! It's important to make your code so readable that you don't need a comment to tell the reader what the code does.

Enumerations

Since Python 3.4 there's support for enumerations, which is exactly what this is:

RPS = ["ROCK", "PAPER", "SCISSORS"]

So you could:

from enum import Enum, auto
class Moves(Enum):
 ROCK = auto()
 PAPER = auto()
 SCISSORS = auto()

Moves["ROCK"] gives you a Moves.ROCK, then.

Calculating a winner

Instead of winding yourself up in some ifs I'd suggest a more data-driven approach:

MOVES_BEATEN_BY = {
 Moves.ROCK: [Moves.SCISSOR],
 Moves.PAPER: [Moves.ROCK],
 Moves.SCISSOR: [Moves.PAPER]
 }
class Outcome(Enum):
 TIE = auto()
 WON = auto()
 LOST = auto()
def determine_game_outcome(player_move, computer_move):
 if computer_move in MOVES_BEATEN_BY[player_move]:
 return Outcome.WON
 elif player_move in MOVES_BEATEN_BY[computer_move]:
 return Outcome.LOST
 else:
 return Outcome.TIE

Separate logic from IO

Don't put any print or input calls inside of "business logic" functions (like when you "calculate" the winner). Separating these two concerns allows for easy adaption to other IO situations (web app, GUI) and makes it (better) possible to test the business logic.

TEST

Your code should have tests. Look also into TDD (test driven development), where you write your tests before you write any code.

• qazwsx598: I removed the call to the main function like you suggested. I haven't yet learned of classes so I'm not going to implement that change until I understand it more. Thank you for giving me a few topics to research.

Answer 3

I've cleaned up my code based on the suggestions for formatting from MrGrj and Daniel Jour. I don't feel comfortable implementing ideas I haven't learned yet so I'm sticking with PEP8 and removing the call to main() from inside the main() function.

I think this looks tremendously better and all my code from here out will be in this format.

import random
RPS = ("ROCK", "PAPER", "SCISSORS")
def main():
 """The main function."""
 computer = computer_makes_a_move()
 player = player_makes_a_move()
 winner, tie = calculate_winner(computer, player)
 display_winner(winner, tie, computer)
def computer_makes_a_move():
 """The computer chooses a weapon."""
 computer = random.choice(RPS)
 return computer
def player_makes_a_move():
 """The user chooses a weapon."""
 player = ""
 while player not in RPS:
 player = input("Choose your weapon(rock, paper, or scissors): ").upper()
 return player
def calculate_winner(computer, player):
 """Determines the winner."""
 winner = None
 tie = False
 if player == computer:
 tie = True
 elif ((computer == "ROCK") and (player == "PAPER")) or \
 ((computer == "PAPER") and (player == "SCISSORS")) or \
 ((computer == "SCISSORS") and (player == "ROCK")):
 winner = True
 else:
 winner = False
 return winner, tie
def display_winner(winner, tie, computer):
 """"Displays the winner."""
 print("The computer chooses {0}.".format(computer))
 if tie:
 print("Tie! Restarting game...\n\n")
 restart_the_game()
 elif winner:
 print("You beat the computer!")
 elif not winner:
 print("The computer beat you!")
def restart_the_game():
 """Restarts the game after a draw."""
 computer = random.choice(RPS)
 player = ""
 while player not in RPS:
 player = input("Choose your weapon(rock, paper, or scissors): ").upper()
 winner, tie = calculate_winner(computer, player)
 display_winner(winner, tie, computer)
main()