12
\$\begingroup\$

As I mentioned in my bio, I do not code for a living and too old to switch careers but I do enjoy it as a hobby and I am trying to get better. Also trying to get my teenage son more interested. Anyhow, the code below does want I want it to but I am looking for tips to improve it. Any advice would be much appreciated before I add more to it with High Score tables and more functionality.

import random
weapons = ["Sword", "Spell", "Fire"]
shields = ["Armour", "Magic", "Water"]

class Player:
    def __init__(self, name):
        self.name = name
        self.health = 100
        self.weapon = 0
        self.shield = 0

    def damage(self):
        points = random.randint(10, 35)
        self.health -= points

    def selectWeapon(self):
        choice = int(input("Choose your weapon 1-Sword, 2-Spell or 3-Fire:  "))
        self.weapon = choice - 1

    def selectShield(self):
        choice = int(input("Choose your shield 1-Armour, 2-Magic or 3-Water:  "))
        self.shield = choice - 1


# Child class of player with override methods for weapon
# and shield selection
class AiPlayer(Player):
    def __init__(self,name):
        super().__init__(name)

    def selectWeapon(self):
        choice = random.randint(1, 3)
        self.weapon = choice - 1

    def selectShield(self):
        choice = random.randint(1, 3)
        self.shield = choice - 1

class Game:
    def __init__(self):
        self.gameOver = False
        self.round = 0

    def newRound(self):
        self.round += 1
        print("\n***   Round: %d   ***\n" %(self.round))  

    # Check if either or both Players is below zero health
    def checkWin(self, player, opponent):
        if player.health < 1 and opponent.health > 0:
            self.gameOver = True
            print("You Lose")
        elif opponent.health < 1 and player.health > 0:
            self.gameOver = True
            print("You Win")
        elif player.health < 1 and ai.health < 1:
            self.gameOver = True
            print("*** Draw ***")


    def displayResult(self, player, opponent):
            print("%s used a %s, %s used a %s Shield\n" %(player.name, weapons[player.weapon], opponent.name, shields[opponent.shield]))
            print("%s caused damage to %s\n" %(player.name, opponent.name))

    def takeTurn(self, player, opponent):

        # Decision Array
        #
        #           Armour|  Magic |  Water
        #           ______|________|_______
        # Sword:    False |  True  |  True
        # Spell:    True  |  False |  True   
        # Fire :    True  |  True  |  False

        decisionArray = [[False, True, True], [True, False, True], [True, True, False]]
        if decisionArray[player.weapon][opponent.shield]:
            opponent.damage()
            currentGame.displayResult(player, opponent)
        else:
            print("\n%s used a %s, %s used a %s Shield" %(player.name, weapons[player.weapon], opponent.name, shields[opponent.shield]))
            print("%s blocked %s's attack - No Damage" %(opponent.name, player.name))


# Setup Game Objects
currentGame = Game()
human = Player("Mark")
ai = AiPlayer("Computer")

players = [human, ai]

# Main Game Loop
while not currentGame.gameOver:
    for player in players:
        player.selectWeapon()
        player.selectShield()
    currentGame.newRound()
    currentGame.takeTurn(human, ai)
    currentGame.takeTurn(ai, human)
    print("%s's health = %d" %(human.name, human.health))
    print("%s's health = %d" %(ai.name, ai.health))
    currentGame.checkWin(human, ai)
\$\endgroup\$
22
\$\begingroup\$

f-strings

Beginning with Python 3.6, there is a friendlier way of formatting strings. Instead of using "format string" % (tuple_of_args) where the argument and the format codes are separated by significant distance, the arguments are embedded in the string itself, surrounded by {}'s. The string itself is prefixed with an f.

For example, instead of:

    print("\n***   Round: %d   ***\n" %(self.round))

you can write:

    print(f"\n***   Round: {self.round}   ***\n")

Enumerations

This code doesn't convey a lot of meaning by itself. It needs to be described by your 7 comment lines.

    decisionArray = [[False, True, True], [True, False, True], [True, True, False]]

Some of the problem stems from indices having semantic meaning. For example, 1 is Magic. But 1 is also Spell. Confusing.

Let's make what is happening a lot clearer. Let's define enumerations for our weapons and shields:

from enum import Enum

Weapon = Enum("Weapon", "Sword, Spell, Fire")
Shield = Enum("Shield", "Armour, Magic, Water")

Now we have Weapon.Sword, Weapon.Spell, and Weapon.Fire, which conveniently have values 1, 2 and 3 respectively. We also have Shield.Armour, Shield.Magic and Shield.Water, also with values 1, 2, and 3.

When we have our player select their choice of weapon, we use to store it as the number 0, 1 or 2:

    self.weapon = choice - 1

but now we can store an actual enumeration object, like Weapon.Fire directly. Conveniently, passing the value 1, 2, or 3 to Weapon(...) will return the corresponding enumeration object:

    self.weapon = Weapon(choice)

Similarly, we can store our shield choice as a Shield enumeration object:

    self.shield = Shield(choice)

The AI is similar, but instead of randint, we can use choice and select a random Weapon and random Shield:

    self.weapon = random.choice(list(Weapon))
    self.shield = random.choice(list(Shield))

But how about that decisionArray? First of all, that is a lousy name. What does "decision array" mean? A Shield will block one (or perhaps more) Weapon attacks. Let's add a .blocks attribute to our Shield objects:

Shield.Armour.blocks = { Weapon.Sword }
Shield.Magic.blocks = { Weapon.Spell }
Shield.Water.blocks = { Weapon.Fire }

(I'm using a set(), so more that one Weapon can be added. For example, Shield.Magic could block both { Weapon.Spell, Weapon.Fire } ... after all, it's MAGIC! Feel free to experiment.)

What would the takeTurn method now look like?

def takeTurn(self, player, opponent):

    if player.weapon not in opponent.shield.blocks:
        # apply damage
    else:
        # damage was blocked

That's pretty straight forward; no table required. If the weapon is not in the set of things the shield blocks, we apply damage.

PEP-8

PEP 8 is a Style Guide for Python. It describes things all Python programs should conform to, such as:

  • variables and function names should be snake_case not mixedCase.

Updated Code

import random
from enum import Enum

Weapon = Enum("Weapon", "Sword, Spell, Fire")
Shield = Enum("Shield", "Armour, Magic, Water")

Shield.Armour.blocks = { Weapon.Sword }
Shield.Magic.blocks = { Weapon.Spell }
Shield.Water.blocks = { Weapon.Fire }

class Player:
    def __init__(self, name):
        self.name = name
        self.health = 100
        self.weapon = None
        self.shield = None

    def damage(self):
        points = random.randint(10, 35)
        self.health -= points

    def select_weapon(self):
        weapons = [f"{weapon.value}-{weapon.name}" for weapon in Weapon]
        weapons = ", ".join(weapons[:-1]) + " or " + weapons[-1]
        choice = int(input(f"Choose your weapon {weapons}:  "))
        self.weapon = Weapon(choice)

    def select_shield(self):
        shields = [f"{shield.value}-{shield.name}" for shield in Shield]
        shields = ", ".join(shields[:-1]) + " or " + shields[-1]
        choice = int(input(f"Choose your shield {shields}:  "))
        self.shield = Shield(choice)

class AiPlayer(Player):

    def select_weapon(self):
        self.weapon = random.choice(list(Weapon))

    def select_shield(self):
        self.shield = random.choice(list(Shield))

class Game:
    def __init__(self):
        self.game_over = False
        self.round = 0

    def new_round(self):
        self.round += 1
        print(f"\n***   Round: {self.round}   ***\n")  

    # Check if either or both Players is below zero health
    def check_win(self, player, opponent):
        if player.health < 1 and opponent.health > 0:
            self.game_over = True
            print("You Lose")
        elif opponent.health < 1 and player.health > 0:
            self.game_over = True
            print("You Win")
        elif player.health < 1 and ai.health < 1:
            self.game_over = True
            print("*** Draw ***")


    def display_result(self, player, opponent):
            print(f"{player.name} used a {player.weapon.name}, {opponent.name} used a {opponent.shield.name} Shield\n")
            print(f"{player.name} caused damage to {opponent.name}\n")

    def take_turn(self, player, opponent):

        if player.weapon not in opponent.shield.blocks:
            opponent.damage()
            current_game.display_result(player, opponent)
        else:
            print(f"{player.name} used a {player.weapon.name}, {opponent.name} used a {opponent.shield.name} Shield\n")
            print(f"{opponent.name} blocked {player.name}'s attack - No Damage")

# Setup Game Objects
current_game = Game()
human = Player("Mark")
ai = AiPlayer("Computer")

players = [human, ai]

# Main Game Loop
while not current_game.game_over:
    for player in players:
        player.select_weapon()
        player.select_shield()
    current_game.new_round()
    current_game.take_turn(human, ai)
    current_game.take_turn(ai, human)
    print(f"{human.name}'s health = {human.health}")
    print(f"{ai.name}'s health = {ai.health}")
    current_game.check_win(human, ai)

Notice in this code, if you added additional weapons and/or shields, nothing need be changed other than the Enum(...) declarations and the .blocks = {} lines. The code itself creates the "1-Armour, 2-Magic or 3-Water" type choice strings, and will properly work with more than 3 choices.

|improve this answer|||||
\$\endgroup\$
  • 3
    \$\begingroup\$ This is awesome thanks very much for all the feedback. It's funny..... The snake case mixed case thing I did read PEP-8, started changing everything to snake case then changed it back. I will stick with the PEP-8 recommendations for future though. \$\endgroup\$ – CelestialMark Feb 20 at 14:07
3
\$\begingroup\$

To add to @AJNeufeld's answer, select_weapon and select_shield should probably validate their input. Those two functions could be refactored to remove duplicate code.

def select_enum(enum_class):
    items = [f"{item.value}-{item.name}" for item in enum_class]
    items = ", ".join(items[:-1]) + " or " + items[-1]

    valid_values = set(item.value for item in enum_class)

    while True:
        choice = int(input(f"Choose your {enum_class.__name__.lower()}: {items}"))
        if choice in valid_values:
            break

        print(f"'{choice}' is not a valid selection. Please try again.")

    return enum_class(choice)


def select_weapon(self):
    self.weapon = self.select_enum(Weapon)


def select_shield(self):
    self.shield = self.select_enum(Shield)
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thank you for your suggestion, much appreciated. I will add it in once I get it straight in my head what each line is doing. \$\endgroup\$ – CelestialMark Feb 22 at 1:12
  • \$\begingroup\$ Okay I get it I was confused by the enum_class.__name__.lower(). I think it is returning the name of module so either Weapon or Shield so that the code works for both selections. Thank you. Learning some cool new code. \$\endgroup\$ – CelestialMark Feb 22 at 1:23
  • 1
    \$\begingroup\$ @CelestialMark, you got it. Every class has a __name__ attribute, that is the name of the class. There are several handy "dunder" attributes. \$\endgroup\$ – RootTwo Feb 22 at 1:28
  • \$\begingroup\$ Nice addition. If you are validating input, however, you should go further. Add a try: ... except ValueError: to catch non-integer input, like "quit". \$\endgroup\$ – AJNeufeld Feb 22 at 6:45

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.