15
\$\begingroup\$

Here is another one of my simple beginner projects I did in my free time in under 30 minutes, a Rock, Paper, Scissors game in Python against a Computer (random module). As usual, I would like to know if there are any improvements/suggestions/criticism/feedback that you guys have about my code. Thanks in advance.

import random
import os

def clear():
    os.system('cls' if os.name == 'nt' else 'clear')

def lobby():
    clear()
    totalgames = len(history)
    totalwins = 0
    totallosses = 0
    totalties = 0

    for game in history:
        if(game == 0):
            totalties += 1
        elif(game == 1):
            totalwins += 1
        elif(game == 2):
            totallosses += 1

    winrate = (2 * totalwins + totalties) / (2 * totalgames) * 100
    winrate = round(winrate, 2)

    print("Your Stats")
    print("_________________________________________________\n")
    print("TOTAL GAMES: " + str(totalgames))
    print("WINS: " + str(totalwins))
    print("LOSSES: " + str(totallosses))
    print("TIES: " + str(totalties) + "\n")
    print("YOUR WINRATE: " + str(winrate) + "%")
    print("_________________________________________________\n")

def game():
    clear()
    computer = random.randint(0,2)
    player = input("Rock, Paper or Scissors?\n").lower()

    if(player[0] == 'r'):
         playerInt = 0
    elif(player[0] == 'p'):
        playerInt = 1
    elif(player[0] == 's'):
        playerInt = 2

    intToOption = [
        "Rock",
        "Paper",
        "Scissors"
    ]

    if(computer == playerInt):
        result = 0

    elif(computer == 0 and playerInt == 1):
        result = 1

    elif(computer == 1 and playerInt == 2):
        result = 1

    elif(computer == 2 and playerInt == 0):
        result = 1

    elif(computer == 0 and playerInt == 2):
        result = 2

    elif(computer == 1 and playerInt == 0):
        result = 2

    elif(computer == 2 and playerInt == 1):
        result = 2
        
    resultMessages = [
        "It's a tie! Both of you chose " + str(intToOption[playerInt] + "."),
        "Player Win! Player chose " + str(intToOption[playerInt] + " while Computer chose " + str(intToOption[computer]) + ". " + intToOption[playerInt] + " beats " + intToOption[computer] + "."),
        "Computer Win! Player chose " + str(intToOption[playerInt] + " while Computer chose " + str(intToOption[computer]) + ". " + intToOption[computer] + " beats " + intToOption[playerInt] + ".")
    ]

    print(resultMessages[result] +"\n")
    input("Input any key to return to lobby.\n")
    history.append(result)
    return result

history = []

def main():
    game()
    lobby()
    while(input("Would you like to play again? (Y/N)\n")).lower()[0] == 'y':
        game()
        lobby()

    clear()
    print("Thanks for playing darrance's Rock Paper Scissors game!")
    input("Input any key to exit the program.\n")

main()
\$\endgroup\$
1
  • \$\begingroup\$ I magically get pinged when such questions arise... I wonder why. ;) \$\endgroup\$ Jun 30, 2021 at 1:05

5 Answers 5

34
\$\begingroup\$

Cleanup

For cleanliness:

if(computer == playerInt):
    result = 0

elif(computer == 0 and playerInt == 1):
    result = 1

elif(computer == 1 and playerInt == 2):
    result = 1

elif(computer == 2 and playerInt == 0):
    result = 1

elif(computer == 0 and playerInt == 2):
    result = 2

elif(computer == 1 and playerInt == 0):
    result = 2

elif(computer == 2 and playerInt == 1):
    result = 2

Can actually be replaced with just a single line:

result = (playerInt - computer) % 3

It's not always possible to find this one-liner in your code, but notice your code has to test so many scenarios. If in any one of those you made a mistake and accidentally wrote the wrong number for the result, it could be difficult the track down the problem. Of course, you would hopefully have unittests to handle all cases, but it still is extra code that can be avoided, so we do it here.

F-Strings

First, you called intToOption to get the player's and computer's choices a lot in your strings. It's better to store these in a temporary variable because it adds readability, and if you decide you want to change how a choice is selected (for example, maybe you'll want to show the integer too) you can change it in one place instead of 5 or 6.

Second, try to use f strings. They make your code more condensed and easier to read:

playerChoice = intToOption[playerInt]
computerChoice = intToOption[computer]
resultMessages = [
    f"It's a tie! Both of you chose {playerChoice}.",
    f"Player Win! Player chose {playerChoice} while Computer chose {computerChoice}. {playerChoice} beats {computerChoice}.",
    f"Computer Win! Player chose {playerChoice} while Computer chose {computerChoice}. {computerChoice} beats {playerChoice}."
]

Simple error handling

If a player doesn't input rock, paper, or scissors, don't let the game crash, just ask them to input again:

while True:
    player = input("Rock, Paper or Scissors?\n").lower()
    if(player[0] == 'r'):
        playerInt = 0
        break
    elif(player[0] == 'p'):
        playerInt = 1
        break
    elif(player[0] == 's'):
        playerInt = 2
        break
    else:
        print(f"{player} is not a valid input!")

You might implement something similar on your would you like to play again screen, although on that one the game doesn't crash if you put an invalid input, it just exits.

Enum

Another way to handle your choices is to define an Enum:

from enum import Enum

class Choice(Enum):
    Rock = 0
    Paper = 1
    Scissors = 2

Now, on input:

while True:
    player = input("Rock, Paper or Scissors?\n").lower()
    if(player[0] == 'r'):
        player = Choice.Rock
        break
    elif(player[0] == 'p'):
        player = Choice.Paper
        break
    elif(player[0] == 's'):
        player = Choice.Scissors
        break
    else:
        print(f"{player} is not a valid input!")

You can drop intToOption entirely and write

result = (player.value - computer.value) % 3

The computer's choice is

computer = Choice(random.randint(0,2))

and for your resultMessages

playerChoice = player.name
computerChoice = computer.name
resultMessages = [
    f"It's a tie! Both of you chose {playerChoice}.",
    f"Player Win! Player chose {playerChoice} while Computer chose {computerChoice}. {playerChoice} beats {computerChoice}.",
    f"Computer Win! Player chose {playerChoice} while Computer chose {computerChoice}. {computerChoice} beats {playerChoice}."
]

Here we use player.name. Normally, if we had chosen Rock, player would appear as Choice.Rock, but player.name appears as Rock only. Using Enums is useful when you have a few unchanging values that you need to represent. Before, you had intToOption which was related to the choices made, but it is possible to alter intToOption without modifying the behaviour of the ints which could cause errors down the road. An Enum is more strict because it pairs the integer and name of an instance together in the class definition. Enums also make the code and your intentions easier to understand.

You could also create an enum for the result of Win, Loss or Tie.

Main

When defining and writing a main function, you probably want to call it as follows:

if __name__ == "__main__":
    main()

This says run main() if this module is run directly. What this means is main() will not be called if you decide to import these functions to another module, but still runs normally if you run this file.

Comments

Add comments to your code and especially your functions! You might not need inline comments to explain what is happening, but docstrings for functions make a big difference. First, they tell you what a function does, but the side benefit of that is they encourage you to write an enclosed function that makes sense. Sometimes, certain behaviours end up in the wrong function so that the code still works, but you end up with two functions not really making sense independently. Of course, you might only ever intend for a function to be used with another one, but the function alone should be summarizable: you should be able to say what a function does without needing information from another one. Your code is pretty good for having these independent functions, but you can still make a docstring like:

def lobby(): 
"""Update player statistics and display them to the user."""

In this case (though not always), you can give a brief oneline description of what is going on, and you can see this encourages you to think about what additional code might go in the lobby definition: if you add more, then update the docstring, but you'll still want the docstring to make sense when you do. The intention of a docstring is of course just to provide a summary of what is happening in a function, but especially for new programmers, it's a good way to make you think about if your functions make sense.

\$\endgroup\$
4
  • 5
    \$\begingroup\$ Most of your comments are on point, with the exception of the line result = (player.value - computer.value) % 3. Sure, it does get rid of a lot of error-prone boilerplate code, but it's relatively obscure, and hinges on the arbitrary mapping of user input to numeric values, and the arbitrary interpretation of the the value of result. Instead, I'd recommend using constants or enums to make this mapping and interpretation explicit but concise. \$\endgroup\$
    – Schmuddi
    Jun 29, 2021 at 6:25
  • \$\begingroup\$ I agree with @Schmuddi, and I'll add that for such things often time a "table", whether array or dictionary, to encode the rules is more obvious (descriptive) than code. Also, when writing such tables, symmetry should be exploited, so here you would have: (1) if same choice, then draw, then (2) if WIN_OVER[x] == y, then player of x wins. \$\endgroup\$ Jun 29, 2021 at 10:52
  • \$\begingroup\$ @Schmuddi Would you be able to suggest an edit (or provide your own answer which I could then link in mine) to demonstrate what you mean? Reading your comment and knowing that later in my answer I introduce an Enum for Choice, I realize that if I also create an Enum called Result, I could create a method for Choice which returns a Result, which I think would really cutdown on the obscurity in the code, but I'm curious if you're suggesting that we remove the numeric representations entirely. \$\endgroup\$
    – Kraigolas
    Jun 29, 2021 at 12:02
  • 1
    \$\begingroup\$ Yes, I indeed suggest removing the numeric representations entirely. What I'd do is to define a Result enum as you suggested, and also a dictionary that defines the winning condition for each possible choice: win_condition = {Choice.Rock: Choice.Scissors, Choice.Paper: Choice.Rock, Choice.Scissors: Choice.Paper}. Now, to determine the result of a match, I'd do something like result = (Result.Win if win_condition[player] == computer else Result.Loss if win_condition[computer] == player else Result.Tie), which should be quite readable with proper line breaks. \$\endgroup\$
    – Schmuddi
    Jun 29, 2021 at 14:48
3
\$\begingroup\$

Choosing Options

This code block

    if(player[0] == 'r'):
         playerInt = 0
    elif(player[0] == 'p'):
        playerInt = 1
    elif(player[0] == 's'):
        playerInt = 2

would be better served to compare the whole string. In this case, srock would be interpreted as scissors. That doesn't seem right. Also, the conversion to int avoids magic numbers if you use a dictionary mapping. Last, you want to handle the case where the user doesn't give valid input with a helpful error message, and printing that message by catching a KeyError might make sense here:

choices = {
    'rock': 0,
    'paper': 1,
    'scissors': 2
}

while True:
    player_choice = input("Rock, Paper or Scissors?\n").strip().lower()
    
    try:
        player_int = choices[player_choice]
    except KeyError as e:
        print(
           (f"Did not get valid input, expected one of {', '.join(choices)}"
            f" got {player_choice}. Please try again")
        )

Alternatively, building on the Enum idea from @Kraigolas's answer:

# Change the values to strings to avoid the conversions
class Choice(Enum):
    Rock = 'rock'
    Paper = 'paper'
    Scissors = 'scissors'


while True:
    player_choice = input("Rock, Paper or Scissors?\n").strip().lower()
    
    try:
        player_choice = Choice(player_choice)
    except ValueError:   # an invalid choice will raise a ValueError
        print(
            ("Expected one of 'Rock', 'Paper', or 'Scissors', but got " 
            f"{player_choice}. Please choose a valid option")
        )

computer_choice = random.choice(('rock', 'paper', 'scissors'))
computer_choice = Choice(computer_choice)

Checking win condition

Using the enum idea makes this a lot more explicit, rather than a collection of magic number checking. Last, you only need to check for a draw scenario and the scenarios in which the player wins, the rest are losses:


if player_choice == computer_choice:
    result = 0
elif player_choice == Choice.Rock and computer_choice == Choice.Scissors:
    result = 1
elif player_choice == Choice.Paper and computer_choice == Choice.Rock:
    result = 1
elif player_choice == Choice.Scissors and computer_choice == Choice.Paper:
    result = 1
else:
    result = 2

Alternatively, reimagined as a set:

# The conditions under which the player wins, where
# the tuples are (user, computer) choices
win_conditions = set(
    (Choice.Rock, Choice.Scissors),
    (Choice.Paper, Choice.Rock),
    (Choice.Scissors, Choice.Paper)
)

if user_choice == computer_choice:
    result = 0
elif (user_choice, computer_choice) in win_conditions:
    result = 1
else:
    result = 2

Updating Game History

As suggested in the comments, it might be better to use a Counter to track game history. This way you can return more concrete results such as win, loss, and draw, rather than 1, 2, and 0, respectively:

from collections import Counter

game_history = Counter({'win': 0, 'loss': 0, 'draw': 0})

def check_win(player_choice, computer_choice):
    """Checks the result of the game and returns either win, loss, or draw"""
    # The conditions under which the player wins, where
    # the tuples are (user, computer) choices
    win_conditions = set(
        (Choice.Rock, Choice.Scissors),
        (Choice.Paper, Choice.Rock),
        (Choice.Scissors, Choice.Paper)
    )
    
    if user_choice == computer_choice:
        result = 'draw'
    elif (user_choice, computer_choice) in win_conditions:
        result = 'win'
    else:
        result = 'loss'

    print(f'Your choice was {user_choice}, computer chose {computer_choice}. Player {result}!')
  
    return result

result = check_win(user_choice, computer_choice)
game_history[result] += 1

This aggregates your game history into something that can easily be summarized. Then, when summarizing your game, you can use the Counter, which is more explicit:

# explicitly take history as an argument so that you aren't
# relying on a global variable here
def show_game_history(history):
    """Shows aggregated game statistics"""
    wins, draws = history['win'], history['draw']
    total_games = sum(history.values())

    win_rate = (2 * wins + draws) / (2 * total_games) * 100

    print("Your Stats")
    print("_________________________________________________\n")
    print(f"TOTAL GAMES: {total_games}")
    print(f"WINS: {wins}")
    print(f"LOSSES: {history['loss']}")
    print(f"TIES: {draws}")
    print(f"YOUR WINRATE: {win_rate}%")
    print("_________________________________________________\n")

f-strings also make this a bit more compact and more pythonic than string concatenation.

Updating your main function

With these changes in mind, your game history can be kept in main, rather than in global scope:

def main():
    game_history = Counter({'win': 0, 'loss': 0, 'draw': 0})

    while True:
        result = game()
        game_history[result] += 1
        show_game_history(game_history)
  
        choice = input('Would you like to play again? (Y/N)\n').strip().lower()
        if choice == 'n':
            break
        elif choice == 'y':
            continue
        else:
            print('Invalid choice, quitting')
            break
\$\endgroup\$
4
  • 2
    \$\begingroup\$ The set idea is so beautiful! +1 Is there a reason to map these to 0, 1 and 2 instead of using a default dict? history = defaultdict(int) and then history('draw') += 1 is imho clearer \$\endgroup\$ Jun 29, 2021 at 21:06
  • \$\begingroup\$ @N3buchadnezzar No particular reason you couldn't, I just didn't review that specific part of the code. Good suggestion though \$\endgroup\$
    – C.Nivs
    Jun 30, 2021 at 12:54
  • \$\begingroup\$ Per your first point, if I were playing this, I'd likely discover early that I can type just r, p or s instead of typing out the whole word. As Homer Simpson once said after discovering he could type y instead of yes, I've tripled my productivity! Only more than that because these words are longer than 3 letters. (I guess if you typed stone expecting it to be the same as rock you might be surprised...) \$\endgroup\$ Jun 30, 2021 at 20:56
  • \$\begingroup\$ @DarrelHoffman ultimately, a nice clear set of instructions makes either way work \$\endgroup\$
    – C.Nivs
    Jul 9, 2021 at 13:12
1
\$\begingroup\$

I find that os.system can be a bit hit and miss depending on how you run it. It tend to look for alternatives.

If you are only using one os.system and random command, it is probably better just to import the command rather than the whole module.

totalgames = len(history) but I dont think you have defined history at that point or else you are using it as a global variable, which I would not recommend.

I wouldn't normally advise you to use integers as the outcome and I would normally go with Kraigolas and use enumeration types or if you are not sure of them strings. However, the way you are using numbers you could make a "result matrix/ 2D jagged list" where the computers option was the row number and player option the column number. Then you don't need to do all those elifs for every case. Perhaps a better alternative to a 2D list would be a dictionary of dictionaries so you can index according to the rock, scissors stone rather than numbers.

I would probably just ditch putting the code into the main method and then calling it. It doesn't really serve any function. As has been mentioned the main can be used to stop the code running from an imported module, but would you ever import this code?

Hope it helps and all the best!

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

You can create a list outcomes, and then your for game in history block can be simplified to either

for game in history:
    outcome[game] +=1

or

outcomes = [sum([game == i for game in history]) for i in range(3)]

Your winrate can be simplified with some math, but that math is easier if you reorder the results as loss = 0, tie = 1, win = 2. Then

winrate = 50*sum(history)/len(history)

When printing out the stats, they'll be easier to read with some padding, and you probably don't need full float precision:

print("Your Stats")
print("_________________________________________________\n")
print("TOTAL GAMES: ".ljust(15) + str(len(history)).rjust(5))
print("WINS: ".ljust(15) + str(outcomes[2]).rjust(5))
print("LOSSES: ".ljust(15) + str(outcomes[0]).rjust(5))
print("TIES: ".ljust(15) + str(outcomes[1]).rjust(5) + "\n")
print("YOUR WINRATE: ".ljust(15) + str(round(winrate,2).rjust(5) + "%")
print("_________________________________________________\n")

Defining a dictionary making converting to an int simpler (note: you should define this dictionary outside the loop):

choice_lookup = {'r':0, 'p':1, 's':2}

Then inside the loop:

playerInt = choice_lookup[player[0]]

With my re-ordering of the results (that is, win = 2), result can be calculated as

result = (playerInt - computer + 1) % 3

Also defining, resultMessages in each iteration of the loop, and defining string you're not using, is a bit wasteful. Also, I don't see any matching parentheses for your elements in resultsMessages. Outside the loop, you can define intToOption and put:

def resultMessage(result, playerInt, computer):
    if result == 1:
        return "It's a tie! Both of you chose " + str(intToOption[playerInt] + "."
    choices_string = "Player chose " + str(intToOption[playerInt] + " while Computer chose " + str(intToOption[computer]) + ". " 
   if result == 0:
       return "Computer Win! "+ 
                  choices_string + 
                  intToOption[computer] + 
                  " beats " + 
                  intToOption[playerInt] + 
                  "."
   return "Player Win! " + 
              choices_string + 
              intToOption[playerInt] + 
              "beats " + 
              intToOption[computer] + 
              "."

Then in the loop put:

print(resultMessage(result, playerInt, computer) +"\n")
\$\endgroup\$
1
  • \$\begingroup\$ (1) Python format strings allow you to do the padding in a much more readable way. (2) As you noticed yourself, your line result = (playerInt - computer + 1) % 3 depends on the arbitrary order of keys and values in choice_lookup. If you change the order, that line breaks. This line is also relatively obscure as it only works because you use magic numbers for the choices. \$\endgroup\$
    – Schmuddi
    Jun 29, 2021 at 6:32
0
\$\begingroup\$

One extra point which I don't think anyone has mentioned: when checking the start of an inputted string, don't use

if(player[0] == 'r'):

but rather

if(player[:1] == 'r'):

These will function identically unless player is the empty string, in which case the first will give an IndexError.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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