6
\$\begingroup\$

Original inspiration was by this Python beginner, and it prompted me to rewrite a bunch of stuff with my flair and my Python experience: First attempt: Python Rock Paper Scissors

Okay, so I looked at the aforementioned post, and was bored and needed to kill an hour at work. So I killed an hour - I took their RPS game and turned it into a class, and made it look less evil/ugly.

While this is by no means a fully-fledged program that I've cleanly created and really thoroughly tested, this is something that I can at least ask for input on.

Runs pretty cleanly, and uses a lot of strings that the OP of the original inspiration post had. But, it has a lot of docstrings, too. And the entire game resides in a class, and calls via the class and such.

Because this version uses f-strings, you must have Python 3.6 or newer to use this program/code.

rps.py:

import random


class RockPaperScissors:
    """
    Class to handle an instance of a Rock-Paper-Scissors game
    with unlimited rounds.
    """

    def __init__(self):
        """
        Initialize the variables for the class
        """
        self.wins = 0
        self.losses = 0
        self.ties = 0
        self.options = {'rock': 0, 'paper': 1, 'scissors': 2}

    def random_choice(self):
        """
        Chooses a choice randomly from the keys in self.options.
        :returns: String containing the choice of the computer.
        """

        return random.choice(list(self.options.keys()))

    def check_win(self, player, opponent):
        """
        Check if the player wins or loses.
        :param player: Numeric representation of player choice from self.options
        :param opponent: Numeric representation of computer choice from self.options
        :return: Nothing, but will print whether win or lose.
        """

        result = (player - opponent) % 3
        if result == 0:
            self.ties += 1
            print("The game is a tie! You are a most worthy opponent!")
        elif result == 1:
            self.wins += 1
            print("You win! My honor demands a rematch!")
        elif result == 2:
            self.losses += 1
            print("Haha, I am victorious! Dare you challenge me again?")

    def print_score(self):
        """
        Prints a string reflecting the current player score.
        :return: Nothing, just prints current score.
        """
        print(f"You have {self.wins} wins, {self.losses} losses, and "
              f"{self.ties} ties.")

    def run_game(self):
        """
        Plays a round of Rock-Paper-Scissors with the computer.
        :return: Nothing
        """
        while True:
            userchoice = input("Choices are 'rock', 'paper', or 'scissors'.\n"
                               "Which do you choose? ").lower()
            if userchoice not in self.options.keys():
                print("Invalid input, try again!")
            else:
                break
        opponent_choice = self.random_choice()
        print(f"You've picked {userchoice}, and I picked {opponent_choice}.")
        self.check_win(self.options[userchoice], self.options[opponent_choice])


if __name__ == "__main__":
    game = RockPaperScissors()
    while True:
        game.run_game()
        game.print_score()

        while True:

            continue_prompt = input('\nDo you wish to play again? (y/n): ').lower()
            if continue_prompt == 'n':
                print("You are weak!")
                exit()
            elif continue_prompt == 'y':
                break
            else:
                print("Invalid input!\n")
                continue

Any suggestions and input are welcome, as this is a rough attempt. :)

\$\endgroup\$
3
\$\begingroup\$

I think it makes sense to use dictionaries to store values for win/tie/loss:

    def __init__(self):
        """
        Initialize the variables for the class
        """
        self.options = {'rock': 0, 'paper': 1, 'scissors': 2}
        self.outcome_count = {
            "tie": 0,
            "win": 0,
            "loss": 0,
        }

This makes check_win a bit more "mechanical" since you can now refer to the outcomes by name and by looking things up in static data, instead of needing a bunch of if/else:

    def check_win(self, player, opponent):
        """
        Check if the player wins or loses.
        :param player: Numeric representation of player choice from self.options
        :param opponent: Numeric representation of computer choice from self.options
        :return: Nothing, but will print whether win or lose.
        """

        result = ["tie", "win", "loss"][(player - opponent) % 3]

        self.outcome_count[result] += 1

        outcome_message = {
            "tie":  "The game is a tie! You are a most worthy opponent!",
            "win":  "You win! My honor demands a rematch!",
            "loss": "Haha, I am victorious! Dare you challenge me again?",
        }
        print(outcome_message[result])

though of course it ends up making print_score less self-explanatory:

    def print_score(self):
        """
        Prints a string reflecting the current player score.
        :return: Nothing, just prints current score.
        """
        wins   = self.outcome_count["win"]
        losses = self.outcome_count["loss"]
        ties   = self.outcome_count["tie"]
        print(f"You have {wins} wins, {losses} losses, and {ties} ties.")

Lastly, I think that the run_game loop can be made slightly clearer by writing instead

        while True:
            userchoice = input("Choices are 'rock', 'paper', or 'scissors'.\nWhich do you choose? ").lower()
            if userchoice in self.options.keys():
                break
            print("Invalid input, try again!")

I find an explicit "early-exit" with no else to be easier to follow [note that the condition is not inverted, which I think helps for clarity in this case] though this could be jarring if it's not conventional for the larger codebase.

|improve this answer|||||
\$\endgroup\$
3
\$\begingroup\$

Formatting & naming

  • According to PEP 8, all lines should be <= 79 characters.
  • userchoice -> user_choice (given that you have opponent_choice)
  • continue_prompt -> user_choice (in the context of where it's being used, it's actually the user's choice/response to the continue prompt, not the continue prompt itself)

Documentation

The docstring for random_choice could be improved. Instead of repeating verbatim what is happening in the code (implementation), prefer documenting it in a way so the reader doesn't need to read the implementation to know what the method will do:

def random_choice(self) -> str:
    """
    Randomly chooses rock, paper, or scissors.
    :return: 'rock', 'paper', or 'scissors'
    """

Clean/normalize user input

It's good that you are already calling lower() on the user input, but you should also be calling strip() on it as well. Otherwise, a user choice with leading or trailing whitespace is treated as invalid input (e.g. ' rock' or 'rock ', ' y' or 'y ').

Efficiency

Each call to random_choice calls list() on the self.options dictionary, which is re-creating the same list of choices on each call. Consider only creating the list once in __init__:

def __init__(self):
    ...
    self.options = {'rock': 0, 'paper': 1, 'scissors': 2}
    self.choices = list(self.options.keys())

Then we can use it in random_choice:

def random_choice(self):
    return random.choice(self.choices)

And when validating user input for choosing 'rock', 'paper', or 'scissors':

if user_choice in self.choices:
    ...

Class structure

Since your class is already handling interactive user input, I think the code where you prompt the user to play another round should live inside the class. Then anyone who wants to use your class to launch an interactive multi-round game of rock-paper-scissors need only do game.run_game().

For the same reason, the call to print_score() should be within the game coordination logic inside your class; a client of your class shouldn't need to call it directly.

I think it would be easier to read if you extracted the interactive prompting and retrieval of user input into their own methods, e.g.

def player_choice(self) -> str:
    """
    Prompts player for choice of rock, paper, or scissors.
    :return: 'rock', 'paper', or 'scissors'
    """
    while True:
        user_choice = input("Choices are 'rock', 'paper', or 'scissors'.\n"
                            "Which do you choose? ").lower().strip()
        if user_choice in self.choices:
            return user_choice

        print("Invalid input, try again!")
def player_wants_to_play_again(self) -> bool:
    """
    Prompts player to play again.
    :return: True if the player wants to play again.
    """
    prompt = "\nDo you wish to play again? (y/n): "
    valid_choices = {'y', 'n'}
    while True:
        user_choice = input(prompt).lower().strip()
        if user_choice in valid_choices:
            return user_choice == 'y'

        print("Invalid input!")

Then your main game methods could look something like this:

def run_one_round(self):
    user_choice = self.player_choice()
    opponent_choice = self.random_choice()
    print(f"You've picked {user_choice}, and I picked {opponent_choice}.")
    self.check_win(self.options[user_choice],
                   self.options[opponent_choice])
    self.print_score()

def run_game(self):
    while True:
        self.run_one_round()
        if not self.player_wants_to_play_again():
            print("You are weak!")
            break

By structuring things like this, we no longer need to call exit() (which exits the Python interpreter) to break out of the main game loop. Note that it's generally considered bad form to use exit() for handling non-exceptional scenarios in your program flow, i.e. if it's possible to allow your program to terminate normally without having to resort to exit(), you should do that.

Bonus: Improving clarity with a custom Enum

In the original program, the implicit contract is that the exact strings rock, paper, and scissors represent the choices each player can make, and are thus special. One can observe this by looking at the dictionary self.options, which has the above strings mapped to integers so we can compare them later on using modular arithmetic in check_win. This sounds like a case where having a custom enum.Enum type might help make things more explicit.

Let's define an Enum called Choice which can take one of three values: ROCK, PAPER, or SCISSORS. What's cool is that we can have Choice be responsible for all of the following:

  • conversion from str to Choice (if the provided string cannot be converted, throw an exception)
  • define a canonical string representation for each Choice, e.g. "rock", "paper", and "scissors" (conversion from Choice to str)
  • make Choices comparable, such that if you have two Choices X and Y, you can compare them to determine which one would win

The code:

from enum import Enum


class Choice(Enum):
    ROCK = 0
    PAPER = 1
    SCISSORS = 2

    @classmethod
    def from_str(cls, s: str) -> "Choice":
        try:
            return {
                "r": cls.ROCK,
                "rock": cls.ROCK,
                "p": cls.PAPER,
                "paper": cls.PAPER,
                "s": cls.SCISSORS,
                "scissors": cls.SCISSORS
            }[s.strip().lower()]
        except KeyError:
            raise ValueError(f"{s!r} is not a valid {cls.__name__}")

    def __str__(self) -> str:
        return self.name.lower()

    def beats(self, other: "Choice") -> bool:
        return (self.value - other.value) % 3 == 1

Interactive session showing it in action:

>>> list(Choice)
[<Choice.ROCK: 0>, <Choice.PAPER: 1>, <Choice.SCISSORS: 2>]

>>> Choice.from_str('rock')
<Choice.ROCK: 0>

>>> Choice.from_str('paper')
<Choice.PAPER: 1>

>>> Choice.from_str('scissors')
<Choice.SCISSORS: 2>

>>> print(Choice.ROCK)
rock

>>> print(Choice.PAPER)
paper

>>> print(Choice.SCISSORS)
scissors

>>> Choice.ROCK == Choice.ROCK
True

>>> Choice.ROCK.beats(Choice.SCISSORS)
True

>>> Choice.PAPER.beats(Choice.ROCK)
True

>>> Choice.SCISSORS.beats(Choice.PAPER)
True

Let's use it in RockPaperScissors to see how it looks. Here's __init__:

def __init__(self):
    self.wins = 0
    self.losses = 0
    self.ties = 0
    self.choices = list(Choice)

Now random_choice and player_choice both return a Choice instead of a str, making the type signatures of these methods much more expressive:

def random_choice(self) -> Choice:
    return random.choice(self.choices)

def player_choice(self) -> Choice:
    prompt = ("\nChoices are 'rock', 'paper', or 'scissors'.\n"
              "Which do you choose? ")
    while True:
        try:
            return Choice.from_str(input(prompt))
        except ValueError:
            print("Invalid input, try again!")

When we were returning strings from the above two methods, it was necessary to clarify in the documentation that only one of three strings would be returned: 'rock', 'paper', or 'scissors'. With Choice, we no longer need to do that since all of that information is explicitly laid out in its definition.

Similarly, check_win now takes in as parameters two Choices instead of two ints. The code is practically self-documenting at this point:

def check_win(self, player_choice: Choice, opponent_choice: Choice):
    if player_choice == opponent_choice:
        self.ties += 1
        print("The game is a tie! You are a most worthy opponent!")
    elif player_choice.beats(opponent_choice):
        self.wins += 1
        print("You win! My honor demands a rematch!")
    else:
        self.losses += 1
        print("Haha, I am victorious! Dare you challenge me again?")

The full code using Choice can be found in this gist.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Partially wrong on PEP8 - PEP8 allows <100 if the sole/exclusive maintainer/team maintaining it agrees. "For code maintained exclusively or primarily by a team that can reach agreement on this issue, it is okay to increase the line length limit up to 99 characters, provided that comments and docstrings are still wrapped at 72 characters." \$\endgroup\$ – Thomas Ward Nov 3 '19 at 4:17
  • \$\begingroup\$ @ThomasWard I don't believe I am misrepresenting PEP8. My wording of "should" (and not "must") is consistent with PEP8's standard recommendation of 79 characters or less for line length. It is also consistent with linters that check code against PEP8's style conventions, e.g. pycodestyle flags any lines longer than 79 characters by default. As a code reviewer, I see no issue in supplying the standard recommendation, given that I don't have any other context to base my review on. I also do not disagree that teams can have their own project-specific conventions -- and neither does PEP8. \$\endgroup\$ – Setris Nov 3 '19 at 4:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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