17
\$\begingroup\$

I got some good feedback from the last code I posted which I have incorporated in to this project (enum, try, f string formatting), so again just looking for some feedback on how the code could be improved. My teenage son came home with a Computer Science project for Rock Paper Scissors with an advanced option to add Lizard and Spock, keep score, ask for users name and give meaningful feedback on result. Not sure what he has come up with yet, I'm guessing a lot of if / elif statements! However, this is my effort. Any suggestions greatly appreciated.

from enum import Enum
from random import choice

Weapon = Enum("Weapon", "Rock, Paper, Scissors, Lizard, Spock")

# Add a list attribute to each weapon of the weapons it can beat
Weapon.Rock.beats = [Weapon.Scissors, Weapon.Lizard]
Weapon.Paper.beats = [Weapon.Spock, Weapon.Rock]
Weapon.Scissors.beats = [Weapon.Paper, Weapon.Lizard]
Weapon.Lizard.beats = [Weapon.Spock, Weapon.Paper]
Weapon.Spock.beats = [Weapon.Scissors, Weapon.Rock]

# Add an dictionary attribute to each weapon of the action verbs it is capable of
Weapon.Rock.actions = {Weapon.Scissors: "blunts", Weapon.Lizard: "crushes"}
Weapon.Paper.actions = {Weapon.Spock: "disproves", Weapon.Rock: "covers"}
Weapon.Scissors.actions = {Weapon.Paper: "cut", Weapon.Lizard: "decapitates"}
Weapon.Lizard.actions = {Weapon.Spock: "poisons", Weapon.Paper: "eats"}
Weapon.Spock.actions = {Weapon.Scissors: "smashes", Weapon.Rock: "vapourizes"}


# Set up player class
class Player:
    def __init__(self, name):
        self.name = name
        self.weapon = None
        self.score = 0


    def display_results(self, opponent, message):
        print(f"{self.name} chose {self.weapon.name} and {opponent.name} chose {opponent.weapon.name}")
        print(f"{self.weapon.name} {self.weapon.actions[opponent.weapon]} {opponent.weapon.name}")
        print(message)


    def win(self):
        self.score += 1



def display_instructions():
    ''' Function to display instructions when game starts'''

    print("""
    ____________________________________________________________________
    First the human player choses a weapon, after that the computer
    will chose a weapon at random
    The object of the game is to pick a weapon that will beat the weapon
    the computer has chosen

    Rock - beats Sciccors and Lizard
    Paper - beats Spock and Rock
    Scissors - beats Paper and Lizard
    Lizard - beats Spock and Paper
    Spock - beats Scissors and Rock

    Have fun!\n
    _____________________________________________________________________
    """)  



def display_score(human, ai):
    """ Function to display score after each turn """

    print("------------------------------------------------------")
    print(f"{human.name} - {human.score}    -    Computer - {ai.score}\n")    



#Main Game

print("Welcome to Rock, Paper, Scissors, Lizard, Spock\n")
player_name = input("What is your first name: ").title()
yes_no = input(f"\nHello {player_name}, do you want to see the instructions (Y or N)? ")
if yes_no.upper() == "Y":
    display_instructions()


# Create player objects
human = Player(player_name)
ai = Player("Computer")


# Main Game loop
while True:

    # User choses weapon
    # Catches the error if user enters an invalid option and loops until valid or QUIT
    try:
        menu_options = [f"{weapon.value} - {weapon.name}" for weapon in Weapon]
        menu_options = "\n".join(menu_options)
        print(menu_options)
        user_choice = input("Make you selection (1 - 5) or type QUIT: ")
        human.weapon = Weapon(int(user_choice))
    except:
        if user_choice.upper() == "QUIT":
            print("Thank you for playing")
            exit()
        else:
            print("Sorry, that was not one of the options, try again!\n")    
            continue

    # Computer chooses weapon
    ai.weapon = choice(list(Weapon))

    # Decides who won, displays results and increases score of wining player
    if human.weapon == ai.weapon:
        print(f"You chose {human.weapon.name} and the computer chose {ai.weapon.name}")
        print("It was a DRAW\n")
    elif ai.weapon in human.weapon.beats:
        human.display_results(ai, "You WIN\n")
        human.win()
    else:
        ai.display_results(human, "You LOSE\n")
        ai.win()

    display_score(human, ai)
\$\endgroup\$
  • 1
    \$\begingroup\$ Standard advice not really worthy of a separate answer: enclose the logic in a function (i.e. main()) and call it inside an if __name__ == "__main__" block. This will allow you to import the module without executing it for both (unit-)tests and future extensions (e.g.: a GUI version might want to re-use the main game loop or some part of it). \$\endgroup\$ – Marco Capitani Mar 9 at 9:35
  • 1
    \$\begingroup\$ Not worth a full answer, but you misspelled "Scissors" in the instructions describing what Rock beats. "beats Sciccors and Lizard" \$\endgroup\$ – Malivil Mar 9 at 12:30
  • \$\begingroup\$ Your Weapon enum is being passed 2 arguments. Add quotes! \$\endgroup\$ – Varad Mahashabde Mar 9 at 12:46
  • 1
    \$\begingroup\$ Somehow, I feel obliged to answer this particular question! :) \$\endgroup\$ – RockPaperLz- Mask it or Casket Mar 9 at 13:22
  • 1
    \$\begingroup\$ Minor nitpick: I would textwrap.dedent(...) the string literal in display_instructions for left-aligned output while preserving the satisfying code layout. \$\endgroup\$ – ojdo Mar 9 at 13:48
12
\$\begingroup\$

One Source of Truth

Weapon.XXX.beats is redundant. Weapon.XXX.actions provides the same information.

elif ai.weapon in human.weapon.actions:
    # Human's weapon has an action -vs- ai's weapon, so human's weapon wins!

Therefore, you can remove all of the Weapon.XXX.beats = [YYY, ZZZ] code. Having only one source of truth avoids the possibility of contradictory information.

Alternatively, you can generate all of the .beats information:

for weapon in Weapon:
    weapon.beats = { inferior for inferior in Weapon if inferior in weapon.actions }

Note: I've used a set for .beats, for efficient in testing.

As Gábor Fekete points out in a comment below, this could be generated with slightly less code:

for weapon in Weapon:
    weapon.beats = set(weapon.actions.keys())

Enhanced Enums

An enums is a class, and like any other class, you can extend it to enhance its behaviour.

In this project and the last project, you used "# - name" as a choice option. We can add a __str__ method this to the Enum class:

class Weapon(Enum):
    Rock = 1
    Paper = 2
    Scissors = 3
    Lizard = 4
    Spock = 5

    def __str__(self):
        return f"{self.value} - {self.name}"

Now, str(weapon) or f"{weapon}" would would produce the "# - name" string. Of course, you'd still use weapon.name when you want just the weapon's name.

As Marco Capitani mentions in the comments, we can define a .beats() method (instead of a .beats data member) which gives us "a single source of truth AND the useful minimal abstraction "beats" provides".

    def beats(self, other_weapon):
        return other_weapon in self.actions

which allows a more natural looking:

    elif human.weapon.beats(ai.weapon):

Extending this: A weapon that beats another weapon is "better than" another weapon, so we could define the > and < comparison operators in the Weapon class.

    def __gt__(self, other_weapon):
        if isinstance(other_weapon, Weapon):
            return self.beats(other_weapon)
        return NotImplemented

    def __lt__(self, other_weapon):
        if isinstance(other_weapon, Weapon):
            return other_weapon.beats(self)
        return NotImplemented

which allows an even more natural looking:

    elif human.weapon > ai.weapon:

But use caution, restraint and common sense when it comes to defining these rich comparison operators. You should only do so if there is a strict ordering possible between the objects, or you can create strange, non-transitive relationships, like:

>>> Weapon.Rock > Weapon.Lizard > Weapon.Spock > Weapon.Rock
True

which means sorting, which relies on these comparison operations, will fail spectacularly. Again, use common sense. While the non-transitive nature of these weapons will break sorting, you aren't likely to be sorting, so I feel the clarity of the human.weapon > ai.weapon wins out in this case.

The isinstance(...) check ensures that things like Weapon.Rock > 0 will still return a TypeError, instead of a meaningless False value.

In this project and the last project, you have an AI choosing weapons at random. We can add a class method to do this into the Weapon class as well:

    @classmethod
    def random(cls):
        return choice(list(cls))

which you would use like ai.weapon = Weapon.random().

AI's name

You've made the AI a Player with the name "Computer". But then you display the score with:

    print(f"{human.name} - {human.score}    -    Computer - {ai.score}\n")    

You should probably use:

    print(f"{human.name} - {human.score}    -    {ai.name} - {ai.score}\n")    

in case you want to change the name of your AI to something else, like "HAL9000", "Deep Thought" or "GLaDOS".

Same issue here, but now you've got a problem with "the". You probably wouldn't want "... and the Deep Thought chose ...", but simply dropping the word "the" is awkward if the name is "Computer" (you wouldn't want "... and Computer chose ..."):

    print(f"You chose {human.weapon.name} and the computer chose {ai.weapon.name}")

Instructions

As Malivi points out, you spelt "scissors" incorrectly, which made me realize you still have multiple sources of truth: the Enum data & the instructions. If you modified the game to include more Weapons (Rock-Paper-Scissors-Lizard-Spock-Well-Plant), you'd have to update both areas!

Instead, you could change the code to display instructions based directly on the Enum data:

def display_instructions(): 
    print(f"""
    {'_'*68}
    First the human player chooses ...
    """)

    for weapon in Weapon:
        losers = ", ".join(w.name for w in weapon.actions)
        print(f"{weapon.name} - beats {losers}")

    print(f"""
    Have fun!

    {'_'*68}
    """)

Joining a list of weapons, with commas between each one, with " and " before the last weapon, possibly using an Oxford comma, left as exercise for the student.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Seems so obvious when you point it out. As usual awesome feedback. Thank you. \$\endgroup\$ – CelestialMark Mar 9 at 3:04
  • 1
    \$\begingroup\$ Also, combining point 1 + 3: you can have "beats" be a method on the Weapon class: def beats(self, otherWeapon):\n return otherWeapon in self.actions This lets you have a single source of truth AND the useful minimal abstraction "beats" provides \$\endgroup\$ – Marco Capitani Mar 9 at 9:30
  • 1
    \$\begingroup\$ The loop to set the .beats could use the weapon.actions.keys() instead of the comprehension. It would be still type dict_keys so converting it to a set will produce the same result. \$\endgroup\$ – Gábor Fekete Mar 9 at 10:27

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.