Basic python rock papers scissors game (first code)

Question

I'd just appreciate if anyone can suggest some incremental improvements that would help me to think better like a coder.

Here is the very basic RPS (rock, paper, scissors).

One thing I may next add is some tests.

import random
import sys
from enum import Enum
welcome='''
Enter 1 for Rock, 2 for Paper, 3 for Scissor
'''
user_val = int(input(welcome))
class RPS(Enum):
 Rock=1
 Paper=2
 Scissor=3
results = {
 "user": "You won.",
 "py": "You lost.",
 "tie": "Tie."
}
def who_wins(user_val, py_val, result):
 py_val =str(RPS(py_val)).replace("RPS.","")
 user_val=str(RPS(user_val)).replace("RPS.","")
 return "You chose {}, Python chose {}. Result: {}".format(user_val, py_val, result)
is_number = isinstance(user_val,int)
if(is_number):
 outside_boundary=user_val < 1 or user_val > 3
 if(outside_boundary):
 sys.exit('you must enter 1, 2 or 3.')
 # if input is user_valid, we choose a random user_value
 our_val = random.randint(1,3)
 # case 1: user chooses Rock
 if(user_val==RPS.Rock.value):
 if(our_val==RPS.Rock.value):
 print(who_wins(user_val, our_val, results["tie"]))
 else:
 print(who_wins(user_val,our_val, results["user"]))
 # case 2 user chooses Paper
 elif(user_val==RPS.Paper.value):
 if(our_val==RPS.Rock.value or our_val==RPS.Scissor.value):
 print(who_wins(user_val, our_val, results['py']))
 else:
 print(who_wins(user_val,our_val, results["tie"]))
 # case 3 user chooses scissor
 else:
 if(our_val==RPS.Rock.value):
 print(who_wins(user_val, our_val, results["py"]))
 
 elif(our_val==RPS.Scissor.value):
 print(who_wins(user_val, our_val, results["tie"]))
 else:
 print(who_wins(user_val, our_val, results["user"]))

• Toby Speight: Please don't modify the code in your question once it has been answered. You could post improved code as a new question, as an answer, or as a link to an external site - as described in I improved my code based on the reviews. What next?. I have rolled back the edit, so that it's clear exactly what version has been reviewed.
• user272752: I must be the only non-python person in the room, but... Spotted inconsistent use of single v. double quote characters... Trivial, but demonstrates a lack of attention to finicky details and leads the reader to wonder what other less-trivial problems may be in there... You asked for comments...

Answer 1

This is my first code review, so feel free to take it with a grain of salt. English is not my primary language, I apologize for any typos.

General comment, if this is your first code, it is really good! Areas to improve: user-friendliness, put your code into functions (as it is your code is hard to read for what it is), test your code! I caught some mistake in your logic.

Regarding user-friendliness, instead of quitting if the input is not "correct", you may want to let the player try again a couple of times. Also, in this case, I believe using an Error instead of exit could be helpful in the future if you ever want to catch it and recover from it.

def get_player_number(mssg=welcome, white_list=["1","2","3","q"], max_try=3):
 """
 :param mssg: a string will be displayed to the use
 :param white_list: the authorized user input
 :param max_try: the maximum number of time a user can fail to enter a proper value, before raising an AssertionError
 :return:
 """
 inp = input(mssg)
 cpt = 1
 while inp not in white_list:
 cpt += 1
 inp = input(mssg)
 if cpt >= max_try:
 raise AssertionError("you must enter 1, 2, 3 or q")
 # in your case you may want to quit if q or cast to int here as this function is still short
 return inp

This function also tests that your program only accepts specific inputs, which simplifies the rest of the code.

The logic to determine the winner part lacks consistency. Your condition checks are inconsistent and erroneous. The first one is missing a condition (you cannot lose if you pick rock), the second use "or" (you cannot win if you pick paper) and the last one use "if, elif, else". Being consistent here would have helped you catch mistakes in your condition checks. And in your case, as pointed by others, there are ways to simplify your test. For example, a tie will always happen if and only if the player and python pick the same value. So you should test for it early.

@toolic gives you a prime example on how to do it. So I will give you another example that uses the fact that you use a class/enum. We are going to overload comparison operator for you enum. To be able to use "==, > ,and <".

class RPS(Enum):
 Rock = 1
 Paper = 2
 Scissor = 3
 def __eq__(self, other):
 return self.value == other.value
 def __gt__(self, other):
 if self == other:
 return False
 if (self is RPS.Rock and other is RPS.Scissor):
 return True
 if (self is RPS.Scissor and other is RPS.Paper):
 return True
 if (self is RPS.Paper and other is RPS.Rock):
 return True
 return False # not mandatory coding style preference
 def __lt__(self, other):
 if self == other:
 return False
 return not self > other

Now we can write the main logic of your code.

player_pick = get_player_number()
if player_pick == "q":
 print("exiting")
 sys.exit(0)
else:
 player_pick = int(player_pick) # this will throw an error on failure.
player_pick = RPS(player_pick)
python_pick = RPS(random.randint(1, 3))
if player_pick == python_pick:
 who_wins(player_pick, python_pick, results["tie"])
elif player_pick > python_pick:
 who_wins(player_pick, python_pick, results["user"])
elif player_pick < python_pick:
 who_wins(player_pick, python_pick, results["py"])
else: # we should never reach this
 AssertionError("Unreachable Code")

The code could still be improved by putting the "game logic" in a function; thus, you could easily implement a play again or a "best of three" functionality. I hope this helps!

Answer 2

Traditionally it's Rock, Paper, Scissors instead of Rock, Paper, Scissor.

You've attempted to validate whether the user input an integer, but what you've written doesn't work. You immediately attempt an int() on an unvalidated string. Instead, you need to inspect the string before the int() call with isdigit.

Better yet - why ask for an integer, when you can ask for the string "rock", "paper" or "scissors"? You've already used an Enum, which is good; you can keep doing that with strings instead of integers.

Indent at four spaces, not two.

Remove the parens () from your if statements.

Move all of your code into functions. There is more, but that will be a good start.

• AJNeufeld: .isdigit() is the wrong function to use. It returns True for ¹ 2 3️⃣ 4 and many other digit-like Unicode characters. isdecimal (ie, a base 10 digit) is the function you want.
• Mah Neh: done all, but inputting the name is a bit annoying for the user imho, i prefer a number, maybe a single letter would be better.

Answer 3

In addition to the good advice offered by the previous answer, here are some other suggestions.

---

Use of whitespace around operators (=, ,, etc.) is inconsistent. You can use the black program to automatically format your code for you.

---

It is great that you use Enum. It is conventional to use upper case for the names:

class RPS(Enum):
 ROCK = 1
 PAPER = 2
 SCISSOR = 3

---

You can simplify your code by eliminating a variable. Change:

outside_boundary=user_val < 1 or user_val > 3
if(outside_boundary):

to:

if user_val < 1 or user_val > 3:

---

Simplify your code using an f string. Change:

return "You chose {}, Python chose {}. Result: {}".format(user_val, py_val, result)

to:

return f"You chose {user_val}, Python chose {py_val}. Result: {result}"

---

Consider also using the pyinputplus module. It has a feature where you allow the user to keep trying to input valid data. This is handy if the user makes a typo.

---

I think a better name for the who_wins function would be something like report_result because the game may not result in a player winning.

---

I see a lot of repeated code in your 3 "cases" at the end. It can be simplified with a single call to who_wins, passing a variable to the result. You can also take advantage of the ternary operator:

 if user_val == our_val:
 result = "tie"
 else:
 if user_val == RPS.Rock.value:
 result = "py" if our_val == RPS.Paper.value else "user"
 elif user_val == RPS.Paper.value:
 result = "py" if our_val == RPS.Scissor.value else "user"
 else: # user chose scissor
 result = "py" if our_val == RPS.Rock.value else "user"
 print(who_wins(user_val, our_val, results[result]))