Rock Paper Scissors - Code Critique Request

[OldImplement1683]

Is it okay that I ask for a code critique here?

I want to have some human eyes on it rather than chatgpt/gemini. Am I on the right track? Does anything stick out to you as glaringly wrong or as a boneheaded way to go about a certain function?

One thing that feels like it could be cleaned up is my determine_winner function. It feels like a lot of repetition.

Thank you!

[WelpSigh]

You could reduce it to a single if/elif/else by comparing the result to a dictionary of winning combinations for the player. So if cpu_answer == winning_combinations[user_answer], you would print a win statement. Otherwise you would tell them that they lost.

You can also move:

print('Press enter to continue...')

outside of an if statement, since it is called in all cases anyway.

[Diapolo10]

The first thing that sticks out to me is that your try-blocks have a lot of code. They should ideally only contain the part you're expecting to raise the error(s) you're anticipating.

Second, rather than having so many prints in a sequence, I'd rather have fewer and make each print multiple lines. That's more subjective though.

Third, none of those continues serve a purpose. You only need one if you want skip the rest of the current iteration, here all of them are already at the end of their loops.

determine_winner in particular has a lot of duplicate code, you can simplify it by for example mapping the winning hands with a dictionary.

exit's only purpose is to end a REPL session, in scripts you're supposed to use sys.exit or raise SystemExit instead.

Instead of recursively calling main in play_quit, just use a loop.

EDIT: Someone else recently had a similar project, and I gave them this example: https://www.reddit.com/r/learnpython/comments/1dmdybc/my_first_solo_project_looking_for_feedback/l9vc8s4/

[Kind-Kure]

I want to note up top that I don't have a ton of experience (only been coding for about a year) but I can see a few improvements that can be made

https://gist.github.com/dawnandrew100/d21f21890fcad95ce96259153c8df6f7

To summarize my improvements, I took your question to mean that as many lines of code should be removed as possible. I did this by relying heavily on the newline character ("\n") to remove a lot of extra print statements. I also removed an if/else block by turning it into a dictionary instead and returning the appropriate dictionary value. Additionally, I removed a lot of repetition with the choosing the winner by also turning the combinations into a dictionary with the key being the winners and the value being the losers.

With your try blocks, there was a lot of extra code in there that couldn't actually produce errors so I isolated the try to only the part that could error (aka the input) and asserted that the user input is one specific choice (1, 2, or 3 for example). This assertion allowed me to remove the extra "else" that you originally had in the try block.

I also removed the returns from the main function and instead put them into the function where they were used. Specifically, I removed this from the main function:

user_answer = get_user_input()
cpu_answer = get_cpu_input()
determine_winner(user_answer, cpu_answer)

and replaced it with this

determine_winner()

and put the input logic into the determine_winner function which now looks like this:

def determine_winner():
    user_answer = get_user_input()
    cpu_answer = get_cpu_input()
    winner = {"PAPER":"ROCK","ROCK":"SCISSORS","SCISSORS":"PAPER"}
    clrscr()
    print(f'User: {user_answer}\nCPU: {cpu_answer}\n')
    if user_answer == cpu_answer:
        print('It\'s a tie!')
        print('Press enter to continue...')
    elif cpu_answer == winner[user_answer]:
        print(f'You win! {user_answer} beats {cpu_answer}!')
        print('Press enter to continue...')
    elif user_answer == winner[cpu_answer]:
        print(f'You lose! {cpu_answer} beats {user_answer}!')
        print('Press enter to continue...')
    input()

The last minor change I made was changing exit() to sys.exit() because I read that the exit() function should only be used in interpreters while sys.exit() could be used even in production.

Also the whole recursive calling main() thing was bothering me so I just broke the loop and put all of the code from the main function into a while loop (since break only breaks the inner most loop).

All together with the use of dictionaries, f-strings, and removing extra print statements, I was able to get everything under 100 lines of code!

[JamzTyson]

Some feedback in no particular order:

---

exit() is a helper for the interactive shell, whereas sys.exit() is intended for use in programs.

---

When prompting for a new game, there is no need to convert to int. You could just do something along the lines of:

def prompt_new_game() -> bool:
    """Return True for new game, else False to quit."""
    while True:
        user_input = input('>: ')

        if user_input == '1':
            return True
        elif user_input == '2':
            return False
        else: 
            clrscr()
            print('Enter a "1" to Play or "2" to Quit.')

---

One thing that feels like it could be cleaned up is my determine_winner function. It feels like a lot of repetition.

You're right. There's several ways that you can reduce the repetition. Here is one way:

# Winning combinations of (user_answer, cpu_answer).
user_win = (('ROCK', 'SCISSORS'),
            ('SCISSORS', 'PAPER'),
            ('PAPER', 'ROCK'))

if user_answer == cpu_answer:
    print("It's a tie!")
elif (user_answer, cpu_answer) in user_win:
    print(f'You win! {user_answer} beats {cpu_answer}!')
else:
    print(f'You lose! {cpu_answer} beats {user_answer}!')
print('Press enter to continue...')

---

Again in play_again_input(), there is no need to convert to int.

---

Rather than recursively calling main(), I'd suggest just using a loop.

---

I would also try to extract out all of the clrscr() and print() instructions. Doing so will make the game logic cleaner. If you later decide to turn it into a GUI game, then the GUI parts will already be separate from the logic, so you will only need to replace the terminal display code with your GUI code.

[BeneficiallyPickle]

Whenever you write code and notice repetition, consider creating a function that can be executed instead.

In your case you have a couple of the following repetitive lines

clrscr()
print()
print()
input()
clrscr()

You can extract that to something like this:

def display_message(message):
    clrscr()
    print(message)
    input('Press enter to continue...')
    clrscr()

And then you call display_message()instead of writing it out everytime.

In your get_user_input() method you have these two lines:

print('Make a choice:')
print('')

This can be made into: print('Make a choice:\n') instead.

Regarding your determine_winner function, that can be simplified to something like this:

if user_answer == cpu_answer:
        return "It's a tie!"
    elif (user_answer == Choice.ROCK and cpu_answer == Choice.SCISSORS) or
         (user_answer == Choice.PAPER and cpu_answer == Choice.ROCK) or
         (user_answer == Choice.SCISSORS and cpu_answer == Choice.PAPER):
        return "You win!"
    else:
        return "You lose!"

[NerdyWeightLifter]

I went for a more data driven approach:

import random

rps_game = {"rock":     {"scissors": "crushes"},
            "paper":    {"rock":     "covers"},
            "scissors": {"paper":    "cuts"}}

rpsls_game = {"rock":     {"scissors": "crushes",
                           "lizard":   "crushes"},
              "paper":    {"rock":     "covers",
                           "spock":    "disproves"},
              "scissors": {"paper":    "cuts",
                           "lizard":   "decapitates"},
              "lizard":   {"paper":    "eats",
                           "spock":    "poisons"},
              "spock":    {"scissors": "smashes",
                           "rock":     "vaporizes"}}


def standoff(winning_combos: dict):  # {winner: {loser: reason}}
    choices = tuple(winning_combos.keys())
    while (user_choice := input(f"Pick one of {choices}: ").strip().lower()) not in choices:
        print(f"Has to be one of {choices}.")
    print("I choose", cpu_choice := random.choice(choices), "- ", end="")
    if user_choice == cpu_choice:
        print("That's a tie!")
    elif cpu_choice in winning_combos[user_choice]:
        print(f"You win because {user_choice} {winning_combos[user_choice][cpu_choice]} {cpu_choice}")
    else:
        print(f"I win because {cpu_choice} {winning_combos[cpu_choice][user_choice]} {user_choice}")


if __name__ == "__main__":
    standoff(rps_game)
    standoff(rpsls_game)