0
\$\begingroup\$

I tried to implement what I understood from previous advices here.

  • use more functions
  • improve the logic of "play"
  • allow wrong user input
  • indent to 4 spaces and remove ()

Mind that I only have access to google colab so I cant benefit from formatting tools or tests.

I also added comments and types to the extent I could do it, and allowed a user to keep playing and accumulate the result.

I'd appreciate general advice in how to get a more readable and neat code, just do not go to extreme detail because I'm quite new to python.

import random
import sys
from typing import TypedDict

welcome='''
    
    Possible values: r, p, s, q.
    Enter r for ROCK, p for PAPER, s for SCISSORS, q to quit the game.

    '''

def request_valid_input(msg:str, allowed_values:list[str], max_tries:int):
    '''
    Gives the user a certain number of tries in case it types the wrong letter.
    returns: valid user input
    '''
    user_input = input(msg).strip()
    if max_tries <=0:
        raise Exception(f"Max tries reached. Input must be one of {allowed_values.join(',')}")
    while user_input not in allowed_values:
        max_tries-=1
        request_valid_input(max_tries=max_tries)
    return user_input

        
class Score(TypedDict):
    user:int
    py:int

class LastRound(TypedDict):
    user_score:int
    py_score:int
    user_letter:str
    py_letter:str

class RPS():
    '''
    Represents a simple Rock Paper Scissors game
    '''
    def __init__(self):
        self.total_score:Score = {"user":0, "py":0}
        self.last_round:LastRound|None = None 
        self.welcome = welcome
        self.max_tries=3 # when user tries an invalid letter or input.
        self.letter_mapping = {"r":"ROCK", "s":"SCISSORS", "p":"PAPER"} # for informing user
        self.allowed_values=['r', 's', 'p', 'q']

    def random_letter(self):
        '''
        Generates r, s or p used as the computer's input value / choice.
        '''
        letters=['r', 's', 'p']
        our_val = random.randint(0,2)
        return letters[our_val]

    def report_result(self)->None:
        '''
        Print a formatted string telling who wins.
        Returns: None
        '''
        user_selection  = self.letter_mapping[self.last_round["user_letter"]]
        py_selection  = self.letter_mapping[self.last_round["py_letter"]]

        print(f"You chose {user_selection}, Python chose {py_selection}")
        if self.last_round["user_score"] > self.last_round["py_score"]:
            print("User wins.")
        elif self.last_round["py_score"] > self.last_round["user_score"]:
            print("Python wins.")
        else:
            print("Ties.")


    def play(self, user_input):
        user_score=0
        py_score=0
        if user_input == "q":
            sys.exit("Exiting game.")
        user_val = user_input
        our_val = self.random_letter()

        # test tie separately.
        if user_val==our_val:
            pass

        # case 1: user chooses ROCK
        elif user_val=="r":
            if our_val=="s":
                user_score+=1

            else:
                py_score+=1
        # case 2 user chooses PAPER
        elif user_val=="p":
            if our_val=="r":
                user_score+=1
            else:
                py_score+=1
        # case 3 user chooses SCISSORS
        else:
            if our_val=="r":
                py_score+=1
            else :
                user_score+=1

        # put the results in the state of the class
        self.last_round:LastRound = {"user_score":user_score, "py_score":py_score, "user_letter":user_val, "py_letter":our_val}
        self.total_score["user"]+=user_score
        self.total_score["py"]+=py_score

    
def run_game(subsequent_message="remember: r for ROCK, s for SCISSORS, p for PAPER"):
    game = RPS()
    keep_going='y'
    first_try=True
    while(keep_going=='y'):
        msg = game.welcome if first_try else "Type your new choice (r, p, s or q): "
        input = request_valid_input(msg=msg, max_tries=game.max_tries, allowed_values=game.allowed_values)
        game.play(input)
        game.report_result()
        keep_going = request_valid_input(msg="Would you like to play again? Type y for yes, n for no.", allowed_values=['n', 'y'], max_tries=2)
        first_try=False
    print(f"user score: {game.total_score['user']}, python score: {game.total_score['py']}")

run_game()
\$\endgroup\$
2
  • 3
    \$\begingroup\$ Please do not make significant changes to your question after the question has been answered, that becomes incredibly messy for future readers who won't be able to follow along. \$\endgroup\$ Commented Apr 27, 2024 at 10:28
  • 1
    \$\begingroup\$ Abuse will not be tolerated. Please watch your tone before writing your next comment. \$\endgroup\$ Commented Apr 27, 2024 at 12:06

1 Answer 1

5
\$\begingroup\$

stack overflow

Thank you for the nice type annotations (and also on Score and LastRound):

def request_valid_input( ... : str, ... : list[str], ... : int):
    ...
    user_input = input(msg).strip()
    ...
    while user_input not in allowed_values:
        ...
        request_valid_input(max_tries=max_tries)

No, please don't recurse in that way. You risk a stack overflow:

RecursionError: maximum recursion depth exceeded

And besides, you're not even assigning user_input = request_valid_input( ... )

The while loop lets you keep prompting for valid input, without need of pushing anything onto the call stack.

Feel free to annotate the returned value: def ... ) -> str:

meaningful identifier

In Score, I guess that py could be a good identifier? Certainly it's concise.

But it needs an explanatory # comment, since as written it is too terse and cryptic. Maybe you have "python" in mind, where many developers would speak of "the computer's score"? If this was a C or rust program, an end user would be unlikely to think of scores in terms of "the C score", or "the rust score".

Teeny, tiny little nit: the RPS '''docstring''' would conventionally be spelled as a """docstring""", no biggie. Just let "$ black *.py" tidy up that, and some other details, for you. We would expect to see single quotes for an expression which includes double quotes, as that lets us avoid \ backwhack escapes.

print('"Exactly so," said Alice.\n\n"Then you should say what you mean," the March Hare went on.')

useless constant attribute

This makes each RPS object more complex, without offering any benefit:

        self.welcome = welcome

Either bring that module-level welcome string down within the class, or simply keep relying on a global reference to it. It's not like we're going to change it on a per-game basis.

Enum

This is fine, as far as it goes:

        self.letter_mapping = {"r": "ROCK", "s": "SCISSORS", "p": "PAPER"}

But it really feels like an Enum is what's wanted here.

Notice that you can arrange for the .value of each enumerated item to be a single lowercase letter, if desired.

Also, we seem to have replicated some of it, with ['r', 's', 'p', 'q'] and ['r', 's', 'p'] showing up more often than desirable. It seems like something we should be able to DRY up with just a little effort.

Here's a thought experiment. What if the UI suddenly required us to cope with
Chi, Fou, Mi, or
Ro, Sham, Bo, or
Jan, Ken, Pon? How many places would you like to chase down the replicated instances of those words? It's not crucial to avoid duplication; we'll just call it a good habit to get into.

And let's not even talk about rock, paper, scissors, lizard, Spock.

duplicating the return type

This signature is lovely:

    def report_result(self) -> None:
        '''
        Print ...
        Returns: None

The "Returns:" remark is redundant with the signature, so you might prefer to elide it.

Worse, after a few months you might start returning some integer score, and mypy would notice the trouble but no automated tools will flag an inconsistent "Returns:" remark.

extra parens

    while(keep_going=='y'):

This isn't C or java -- we don't need ( ) parens there. As mentioned above, running black over this code base would tidy it up a bit.

extra flag

    first_try = True
    while keep_going == 'y':
        msg = game.welcome if first_try else "Type your new ... "

Certainly this works, you can keep it as-is.

Sometimes introducing such a flag is exactly the Right Thing to do. But here, it would have sufficed to initially assign msg = game.welcome, and then at bottom of loop unconditionally reassign "Type your new ... " on top of the existing msg.

\$\endgroup\$
0

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.