"Rock, Paper, Scissors" game

Question

This is a simple "Rock, Paper, Scissors" game I made in Python 3. Feel free to critique me and give suggestions on how can I improve my newbie coding skills.

import os
from random import randint


def ask_player():
    while True:
        print("Rock, paper or scissors?")
        ans = input(">")
        if ans in ('Rock', 'Paper', 'Scissors'):
            return ans


def results_msg(x, y, result):
    message = f"{x} beats {y}. You {result}!"
    return message


def comp_play():
    comp_choice = randint(0, 2)
    if comp_choice == 0:
        comp_choice = 'Rock'
    elif comp_choice == 1:
        comp_choice = 'Paper'
    else:
        comp_choice = 'Scissors'
    return comp_choice


def results(wins, losses, draws):
    player_choice = ask_player()
    comp_choice = comp_play()
    if player_choice == comp_choice:
        print("Draw. Nobody wins or losses.")
        draws += 1
    elif player_choice == 'Rock':
        if comp_choice == 'Paper':
            print(results_msg(comp_choice, player_choice, 'lost'))
            losses += 1
        else:
            print(results_msg(player_choice, comp_choice, 'won'))
            wins += 1
    elif player_choice == 'Paper':
        if comp_choice == 'Rock':
            print(results_msg(player_choice, comp_choice, 'won'))
            wins += 1
        else:
            print(results_msg(comp_choice, player_choice, 'lost'))
            losses += 1
    else:
        if comp_choice == 'Rock':
            print(results_msg(comp_choice, player_choice, 'lost'))
            losses += 1
        else:
            print(results_msg(player_choice, comp_choice, 'won'))
            wins += 1
    return wins, losses, draws


def play_again():
    while True:
        print("\nDo you want to play again?")
        print("[1] Yes")
        print("[2] No")
        ans = input("> ")
        if ans in '12':
            return ans


def main():
    wins = 0
    losses = 0
    draws = 0
    while True:
        os.system('cls' if os.name == 'nt' else 'clear')
        print(f"Wins: {wins}\nLosses: {losses}\nDraws: {draws}")
        wins, losses, draws = results(wins, losses, draws)
        if play_again() == '2':
            break


if __name__ == '__main__':
    main()

The follow-up is ""Rock, Paper, Scissors" game - follow-up".

Answer 1

Welcome back! There are a lot of good things to say about your program, and a few not-so-good. First, the good:

1. The code is clean, well laid-out, and generally written in a Pythonic style.

2. You have used functions to break things down.

3. You have structured your program as a module, which should make it easier to test.

Here are some things that I think could be improved:

1. Your function names need a little work. Consider this code:

   player_choice = ask_player()
   comp_choice = comp_play()
   

   The object is to get two choices, one made by the player and the other made by the computer. Why are the two names so different? ask_player doesn't sound like getting a player's choice. It sounds like a generalized function that asks the player something and gets a response (i.e., input()). On the other hand, if player is spelled out why do you abbreviate the opponent in comp_play?

   Using get is not always a good thing. It's one of the times when a function or method name doesn't need a verb in it - because it is frequently implicit when you are doing is_... or has_... or get_... or set_.... I don't think you need to spell out get_player_choice and get_computer_choice, but certainly player_choice and computer_choice would be appropriate.

This same logic applies to results. Instead of calling a function named results, why not call play_once? Or one_game? It's obvious from the code in main what is going on, but the function name doesn't really match the nature of the "step" being executed.

2. Your code breakdown is uneven. Consider this code:

   def main():
       wins = 0
       losses = 0
       draws = 0
       while True:
           os.system('cls' if os.name == 'nt' else 'clear')
           print(f"Wins: {wins}\nLosses: {losses}\nDraws: {draws}")
           wins, losses, draws = results(wins, losses, draws)
           if play_again() == '2':
               break
   

   Let's break those lines down. The key point I want to make is to keep neighboring statements at a similar level of abstraction.

   First, you initialize your variables:

   wins = 0
   losses = 0
   draws = 0
   

   Because you are not using a class, and are not using globals (which would be appropriate in this scenario, IMO), you are stuck with doing variable initialization here. I suggest that you make this consistent with how you update the variables after each game:

   wins, losses, draws = starting_scores()
   

   Now, starting_scores could just return 0,0,0 or it could load from a saved-game file. But it makes the initialization sufficiently abstract, and it also spells out what you are doing.

   Next, you loop:

   while True:
       ...
       if play_again() == '2':
           break
   

   The while True ... break could be rewritten to use a boolean variable. That's not super-critical, since the value of that variable is determined at only a single location. I consider the break to be equivalent in this case.

   However, the comparison == '2' is not acceptable! Why? Because that's a detail, and your function name play_again should take care of that detail for you! Don't ask a question and then interpret the answer. Make your question-asking code handle the interpretation for you. Obviously play_again is short for "do you want to play again?" and '2' is not a valid answer. True or False are valid answers, so the code should look like:

   while True:
       ...
       if not play_again():
           break
   

   Finally, the inside of your loop has the same problem:

   os.system('cls' if os.name == 'nt' else 'clear')
   print(f"Wins: {wins}\nLosses: {losses}\nDraws: {draws}")
   wins, losses, draws = results(wins, losses, draws)
   

   What are you doing here? Well, you are clearing the screen, showing a summary of the games played, and playing one more round of the game. So say that!

   clear_screen()
   show_statistics(wins, losses, draws)
   wins, losses, draws = play_one_round(wins, losses, draws)
   

3. Use appropriate data structures.

   Your main code passes three variables to your play-game code. That code then returns three data items in a tuple, which you unpack into three variables.

   In fact, you never use one of those variables without also having the others at hand. This should tell you that you are dealing with one aggregate data item, instead of three independent pieces of data. If that's true, just treat the scores as a single item:

   def main():
       scores = starting_scores()
   
       while True:
           clear_screen()
           show_statistics(scores)
           scores = rock_paper_scissors(scores)
   
           if not play_again():
               break
   

   Similarly, you can treat the scores as an aggregate until you have to update them:

   # NB: was `results(wins, losses, draws):`
   def rock_paper_scissors(scores):
       player = player_choice()
       computer = computer_choice()
       outcome = game_outcome(player, computer)
       show_results(outcome, player, computer)
       new_scores = update_scores(scores, outcome)
       return new_scores
   

   At this point, the "play one game" has also become a collection of abstract statements. But notice that I'm treating scores as an opaque blob that I don't need to deal with: I just pass it along to the lower levels, with another data item describing the update to make.

4. Be consistent!

   I notice that when asking the player to choose rock, paper, or scissors, you allow them to type in an answer. But given a Yes/No question, you require a selection of either 1 or 2. That's consistently surprising. When I ran your code, I wanted to keep typing my answers. (I kept hitting 'y' to play again.)

   I suggest you either present the Rock/Paper/Scissors options as a menu, or present the Yes/No options as a string input and look for 'y' or 'n'. Making the interface that much more consistent will be an improvement.

5. Use data, or code. Not both.

   This one is a little subtle, but take a look:

   if comp_choice == 'Paper':
       print(results_msg(comp_choice, player_choice, 'lost'))
       losses += 1
   else:
       print(results_msg(player_choice, comp_choice, 'won'))
       wins += 1
   

   What's significant here is that you have an if/then statement that decides whether you won or lost. And then you pass that into your results_msg function as a string parameter. The result of this is that you have a string parameter to be substituted that gives information you already knew: whether the player won or lost.

   Let's look at results_msg:

   def results_msg(x, y, result):
       message = f"{x} beats {y}. You {result}!"
       return message
   

   You have to consider that Python f-strings are code. And they're a pretty compact form of code, compared to the horror of str.format(). So writing:

   print(results_msg(player_choice, comp_choice, 'won'))
   

   is not really an improvement on writing:

   print(f"{player} beats {computer}. You won!")
   

   It's not clearer. It's not shorter. It does avoid problems with changing the text of the message, although there isn't much text in the message to change.

   I don't think you need to hoist the f-string up into the calling function. I do think you should not pass 'won' or 'lost' as a parameter: you already decided you won or lost. Call a separate function instead.

   if ...
       win_message(player_choice, comp_choice)
   else:
       lose_message(player_choice, comp_choice)
   

   Note that this will appear to conflict with the code structure I showed above- because in that code structure, I chose to treat the result as data, not code. I'm not saying you have to use data, or that you have to use code. I'm saying that you should pick one and stick with it. If you determine your outcome as code, go ahead and hard-code the outcome. If you determine your outcome as data, go ahead and treat it as data.

   And as a side note, strings with substitutions in them make it hard to do i18n. So there's nothing wrong with having an array of totally spelled out messages at the bottom. It also gives a bit more "flavor" if you customize the verbs:

   "Rock breaks scissors. You won!",
   "Scissors cuts paper. You won!",
   "Paper covers rock. You won!",
   ...
   

Answer 2

Just a real fast suggestion before I go to bed. When doing the check for what type of action the player is doing set there input to lowercase so they dont have to get the case correct.

Example

if player_choice.lower() == 'paper':

In this example the code will run the paper logic if the player inputted Paper or paper

Answer 3

In comp_play, you should return immediately instead of setting a variable to return at the end (although this is slightly controversial). However, you should replace your random.randint/if/elif/else with random.choice:

def comp_play():
    return choice(('Rock', 'Paper', 'Scissors'))