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

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?

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.

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