18
\$\begingroup\$

This is my first attempt at OOP/classes: a game where you will select a character and use attacks and abilities to defeat the enemy character. I have a few specific questions:

I was told that having the "while" statement in the Character doesn't make any sense but I don't know where I should put those. Also, notable right away, is what seems to me like a scissors and tape way of taking turns... I feel like there's a better way but I could not think of it.

import random

class Character:
    def __init__(self, health, strength, mobility, energy):
        self.health = health
        self.strength = strength
        self.mobility = mobility
        self.energy = energy

    turn = False

    is_target = True

    is_dazed = False
    is_stunned = False
    is_debilitated = False
    is_confused = False
    is_terrified = False

    while is_debilitated:
        mobility = 0

    def success_check(self):
        defense = roll_d_x(attack_dice)
        if defense >= defender.mobility:
            success = True
        else:
            success = False
        return success

    def skip_turn(self):
        print("Skipping turn.")
        self.turn = False


player1 = None
player2 = None
defender = None
attack_dice = 100

nec_starting_health = 80
nec_starting_strength = 10
nec_starting_mobility = 10
nec_starting_energy = 20

ghost_starting_health = 10
ghost_starting_strength = 10
ghost_starting_mobility = 30
ghost_starting_terr = 5
skel_starting_health = 30
skel_starting_strength = 30
skel_starting_mobility = 10
skel_starting_terr = 10
mons_starting_health = 50
mons_starting_strength = 20
mons_starting_mobility = 20
mons_starting_terr = 20

My idea for minions was to give the Necromancer a list and append these minions to it...good idea/bad idea?

class Necromancer(Character):
    def __init__(self):
        self.health = nec_starting_health
        self.strength = nec_starting_strength
        self.mobility = nec_starting_mobility
        self.energy = nec_starting_energy

        self.friendly_minions = list()

    def gain_health(self, amount):
        self.health += amount
        print(f"Health gained: {amount}")
        if self.health > nec_starting_health:
            self.health = nec_starting_health

    def lose_health(self, amount):
        self.health -= amount
        print(f"Health lost: {amount}")
        self.energy += amount
        print(f"Life Force gained: {amount}")
        if self.health <= 0:
            game_over = True

    def gain_strength(self, amount):
        self.strength += amount
        if self.strength > nec_starting_strength:
            self.strength = nec_starting_strength

    def lose_strength(self, amount):
        self.strength -= amount
        if self.strength < 0:
            self.strength = 0

    def gain_mobility(self, amount):
        self.mobility += amount
        if self.mobility > nec_starting_mobility:
            self.mobility = nec_starting_mobility

    def lose_mobility(self, amount):
        self.mobility -= amount
        if self.mobility < 0:
            self.mobility = 0

    def gain_energy(self, amount):
        self.energy += amount
        print(f"Life Force gained: {amount}")
        if self.energy > (nec_starting_energy + nec_starting_energy):
            self.energy = (nec_starting_energy + nec_starting_energy)

    def lose_energy(self, amount):
        self.energy -= amount
        print(f"Life Force lost: {amount}")
        if self.energy < 0:
            self.energy = 0

    def sacrifice_random_minion(self):
        if len(self.friendly_minions) == 0:
            print("No Minion to sacrifice.")
        else:
            ran_minion = random.randint(0, (len(self.friendly_minions) - 1))
            sacrificed_minion = self.friendly_minions[ran_minion]
            print("Minion sacrificed.")
            sacrificed_minion.show_attributes()
            self.friendly_minions.remove(sacrificed_minion)

    def attack(self):
        if self.is_dazed:
            print("Dazed...attack unsuccessful.")
        elif self.is_confused:
            confused_test = roll_d_x(2)
            if confused_test == 1:
                print("Confused...attack unsuccessful.")
        else:
            success = self.success_check()
            if success:
                defender.lose_health(self.strength)
                self.gain_health(20)
            else:
                print(f"Attack missed.")


    def cull(self):
        if self.is_stunned:
            print("Stunned...ability unsuccessful.")
        elif self.is_confused:
            confused_test = roll_d_x(2)
            if confused_test == 1:
                print("Confused...ability unsuccessful.")
        else:
            success = self.success_check()
            if success:
                if len(self.friendly_minions) == 0:
                    print("Ability failed. No Minion to sacrifice.")
                else:
                    ran_minion = random.randint(0, (len(self.friendly_minions) - 1))
                    sacrificed_minion = self.friendly_minions[ran_minion]
                    print("Minion sacrificed.")
                    sacrificed_minion.show_attributes()
                    self.gain_health(sacrificed_minion.health)
                    self.friendly_minions.remove(sacrificed_minion)
            else:
                print("Ability failed.")

    def call_ghost(self):
        if self.is_stunned:
            print("Stunned...ability unsuccessful.")
        elif self.is_confused:
            confused_test = roll_d_x(2)
            if confused_test == 1:
                print("Confused...ability unsuccessful.")
        else:
            success = self.success_check()
            if success:
                self.lose_health(5)
                self.lose_energy(10)
                ghost = Ghost()
                self.friendly_minions.append(ghost)
                print("Ghost Called.")
            else:
                print("Ability failed.")

    def raise_skeleton(self):
        if self.is_stunned:
            print("Stunned...ability unsuccessful.")
        elif self.is_confused:
            confused_test = roll_d_x(2)
            if confused_test == 1:
                print("Confused...ability unsuccessful.")
        else:
            success = self.success_check()
            if success:
                self.lose_health(10)
                self.lose_energy(25)
                skeleton = Skeleton()
                self.friendly_minions.append(skeleton)
                print("Skeleton Raised.")
            else:
                print("Ability failed")

    def summon_monster(self):
        if self.is_stunned:
            print("Stunned...ability unsuccessful.")
        elif self.is_confused:
            confused_test = roll_d_x(2)
            if confused_test == 1:
                print("Confused...ability unsuccessful.")
        else:
            success = self.success_check()
            if success:
                self.lose_health(20)
                self.lose_energy(40)
                monster = Monster()
                self.friendly_minions.append(monster)
                print("Monster Created.")
            else:
                print("Ability failed.")


    def start_turn(self):
        self.gain_energy(((100 - self.health) / 2) + 10)

        for minion in self.friendly_minions:
            minion.attack()

    def end_of_turn(self):
        self.is_dazed = False
        self.is_stunned = False
        self.is_debilitated = False
        self.is_confused = False
        self.is_terrified = False

        self.gain_energy((100 - self.health) / 2 + 10)

        if (self.health + self.energy) > 100:
            self.energy = (100 - self.health)


class Minion:
    def __init__(self, health, strength, mobility, chance_terrify):
        self.health = health
        self.strength = strength
        self.mobility = mobility
        self.chance_terrify = chance_terrify

    is_target = True

    def show_attributes(self):
        print(f"Minion:\n{self.health} Health\n{self.strength} Strength\n{self.mobility} Mobility")

    def attack(self):
        flip = roll_d_x(2)
        if flip == 1:
            print("Minion attack failed.")
        else:
            defense = roll_d_x(100)
            if defense >= defender.mobility:
                defender.lose_health(self.strength)
                print(f"The minion attack is successful.")
            else:
                print(f"Attack missed.\n(Defense Roll, {defense}. Defender's Mobility, {defender.mobility})")


class Ghost(Minion):
    def __init__(self):
        self.health = ghost_starting_health
        self.strength = ghost_starting_strength
        self.mobility = ghost_starting_mobility
        self.chance_terrify = ghost_starting_terr


class Skeleton(Minion):
    def __init__(self):
        self.health = skel_starting_health
        self.strength = skel_starting_strength
        self.mobility = skel_starting_mobility
        self.chance_terrify = skel_starting_terr


class Monster(Minion):
    def __init__(self):
        self.health = mons_starting_health
        self.strength = mons_starting_strength
        self.mobility = mons_starting_mobility
        self.chance_terrify = mons_starting_terr


def roll_d_x(max_num):
    roll = random.randint(1, max_num)
    return roll

This, again, is a but of scissors and tape. I'm not sure of a better way to do this.

def pick_starting_player():
    flip = roll_d_x(2)
    if flip is 1:
        player1.turn = True
    else:
        player2.turn = True
    if player1.turn:
        print("Player 1 goes first.")
    else:
        print("Player 2 goes first.")


def end_turn_p1():
    player1.turn = False
    player2.turn = True


def end_turn_p2():
    player2.turn = False
    player1.turn = True

This is for testing ... I need to learn how to make a main I think... Eventually, I would like to get this into pygame(?), have clickable buttons for abilities, visual monsters and heroes, etc...

player1 = Necromancer()
player2 = Necromancer()

pick_starting_player()

game_over = False
while not game_over:

    while player1.turn is True:
        defender = player2
        player1.start_turn()
        print("***\nPlayer1")
        player1.show_skills()
        while True:
            user_skill = input("Enter the skill you wish to use: ")
            if user_skill.lower() == 'attack':
                player1.attack()
                break
            elif user_skill.lower() == 'cull':
                player1.cull()
                break
            elif user_skill.lower() == 'call ghost':
                player1.call_ghost()
                break
            elif user_skill.lower() == 'raise skeleton':
                player1.raise_skeleton()
                break
            elif user_skill.lower() == 'summon monster':
                player1.summon_monster()
                break
            else:
                print("Invalid skill")
                continue
        player1.end_of_turn()
        end_turn_p1()

    while player2.turn is True:
        defender = player1
        player2.start_turn()
        print("Player2 does nothing on his turn.")
        end_turn_p2()
\$\endgroup\$

4 Answers 4

19
\$\begingroup\$

Welcome back! I'm going to take a moment to talk about modules and classes before I start reviewing your code.

Modules and Classes

Python code is stored in source files with a .py extension. These are modules. If you write a "program" in python -- something meant to be run from the command line, like python mygame.py -- your program is still a module, although it has some unusual properties (like __name__ is set to "__main__" instead of the module name).

Modules

Regardless, the python execution process starts by parsing all of your code, from the top of the file to the bottom. It produces a parse tree, which converts to an AST (except for possibly 3.8+ where the parser might emit the AST directly), which is then compiled into byte code. The byte code is then either stored in a .pyc file (non-main modules) or just executed directly from memory.

Each module when loaded into memory is made up of nothing but executable statements. The executable statements are all the code that you put "up against the wall" on the left margin, and also class definitions and function definitions.

Again: class definitions are executable code.

Also: function definitions are executable code

Classes

A class definition is "executed" when you reach the point in the code flow that it appears. That means if you have a module like:

A = 1

class Dog:
    pass

B = 1

then the class definition is executed after A = 1 and before B = 1. This is also true for function definitions: they are executed where they appear in the code flow.

A class definition consists of some book-keeping code for creating the class and linking it with its parents and metaclass, plus creating a new entry in the module's namespace, plus all the code you type inside the class definition.

Most of the time, the "code you type inside the class definition" consists of class-level constants, class-level variable initialization, and function (method) definitions, which we already mentioned as being executable code.

If you type code that is "actual code" -- that is, call functions, print results, whatever -- it will be executed in the order in which it appears in the class (top to bottom) whenever the class definition is executed. As we have discussed, that means in the order in which the class appears in the file.

Your code

Your code looks good. It conforms to a lot of PEP-8, modulo a few nits like the number of blank lines between things.

There is too much code up against the wall. You have skipped over the if __name__ == '__main__': check. Please correct that, even if you do naught but move everything into one big main().

Too many names

One problem you suffer from is too many names. Worse, the names are formatted incorrectly.

nec_starting_health = 80
nec_starting_strength = 10
nec_starting_mobility = 10
nec_starting_energy = 20

These are supposed to be constants -- you only use them to set other values. But constants should be in UPPER_SNAKE_CASE.

But rather than change that, I'd suggest you just make these numbers part of the appropriate class. You could have something like:

class Necromancer(...):
    STARTING_ENERGY = 20
    STARTING_HEALTH = 80

Instead, I suggest that you just hard-code the numbers into the Necromancer.__init__ function, which is the only place they'll be used. Use named parameters for clarity:

class Necromancer(Character):
    """ ... """
    def __init__(self):
        super().__init__(health=80, strength=10, mobility=10, energy=20)

This does the same things, with less typing: it conveys that these are magic numbers specific to the initial setup of the Necromancer class, it documents what number is what attribute, and it lets you change them in one place. Admittedly, it doesn't let you change them from outside the module, but I'd call that a feature ;-).

One side effect of this would be creating max_XXX attributes, for attributes where you wanted to be able to gain them back. This seems obvious to me, and you could move these functions into the Character base class if you made the attributes explicit, instead of trying to sneak references to those outside names in your subclass methods.

Extra code in class Character

As you mentioned yourself, all this code is extra:

turn = False

is_target = True

is_dazed = False
is_stunned = False
is_debilitated = False
is_confused = False
is_terrified = False

while is_debilitated:
    mobility = 0

It might be that you planned to indent that under the def __init__() that appears above it. In which case you probably meant something like self.turn = False. Otherwise, you are defining a class variable turn which will be shared by everything that is an instance or subclass-instance of Character.

The is_... attributes clearly should be instance attributes -- move them into your __init__ method. The while is_debilitated is not executed, since you defined is_debilitated = False just a few lines higher. But since you don't do anything to change is_debilitated inside your loop, that will be an infinite loop and hang your program if you ever actually run the code. I think you meant if instead of while, and you meant for it to be located someplace else.

DRY: Missing class Attribute

You have four attributes, and you have written gain and lose methods for all four. Why not just have your attributes be of class Attribute and write gain and lose methods once, for the class:

player1.gain_energy(1) --> player1.energy.gain(1)

DRY: Missing function: use_ability()

Your class Necromancer has a bunch of "ability" methods. They all share the same common code:

def call_ghost(self):
    if self.is_stunned:
        print("Stunned...ability unsuccessful.")
    elif self.is_confused:
        confused_test = roll_d_x(2)
        if confused_test == 1:
            print("Confused...ability unsuccessful.")
    else:
        success = self.success_check()
        if success:

            ### NOTE: This part is different

        else:
            print("Ability failed.")

So why not write a function (method) for that:

def use_ability(self, ability):
    """Common framework for ability methods."""
    # ...
    # as before
    # ...
    if success:
        ability()
    # ...
    # as before ...

Then you can code the individual ability methods as _methodname to indicate they're tricky, and drop all the boilerplate code at the beginning and end, just assuming you succeeded.

Class Minion

This seems like it should be a subclass of Character, but perhaps there should be a common parent class Combatant or something that both Minion and Character can derive from? Or perhaps Minion should be the parent of Character?

Missing attribute: name

Instead of Minion attack failed print f"{self.name}'s attack failed" or perhaps type(self).__name__ to get the class name ("Character's attack failed", "Ghost's attack failed", etc.)

Bogus attribute: turn.

The turn is a game concept, not a character concept. Don't store it in the character or minion. Store it at the game level.

Missing function: player turn

You have a lot of code inside the game while loop for executing the player1 and player2 turns. Move that into a function.

That's it for this pass. Good luck!

\$\endgroup\$
7
  • \$\begingroup\$ I'm struggling to understand creating class Attribute. Thank you for everything so far... \$\endgroup\$ Commented Oct 8, 2019 at 23:59
  • \$\begingroup\$ An Attribute, as you are using it, has a name, a value, a maxvalue. The minvalue is 0, I assume. It should support gain() and lose() operations. Then you could say self.energy = Attribute('energy', 20) to create one, and say self.energy.lose(2) when you want to spend points on something. \$\endgroup\$
    – aghast
    Commented Oct 9, 2019 at 3:32
  • \$\begingroup\$ As for the use_ability, you could also turn this into a decorator. \$\endgroup\$
    – JAD
    Commented Oct 9, 2019 at 12:30
  • \$\begingroup\$ Procedural question: When I want to post a revised/expanded code of the same game, does it belong as a new post entirely? \$\endgroup\$ Commented Oct 10, 2019 at 15:40
  • \$\begingroup\$ @ClaudeFried You can add an Update: to the bottom if you are just incorporating suggested changes. If you extend the game by adding more unreviewed code, I'd say it's time for a new review (but include a link to this one so that people can see the evolution, and nag you about changes you haven't made yet. ;-) \$\endgroup\$
    – aghast
    Commented Oct 10, 2019 at 20:32
7
\$\begingroup\$

Character.success_check

This method can be reduced to two lines

def success_check(self):
    defense = roll_d_x(attack_dice)
    return defense >= defender.mobility

Since defense >= defender.mobility evaluates to a boolean expression, you can simply return the statement.

roll_d_x()

This method can be reduced to one line

def roll_d_x(max_num):
    return random.randint(1, max_num)

Expression vs is True or == True

Instead of checking if an expression is equal to True or False, simply check the expression. It evaluates to a boolean result, and you can go from there:

while player2.turn is True:

to

while player2.turn:

For numbers, use == instead of is. is will return True if two variables point to the same object, == if the objects referred to by the variables are equal.

Character Class

Not really sure what the while loop is doing there. If this were to be run, and the loops expression be True, then it'd be an infinite loop, as there's no way to change the state of is_debilitated with the code that's written.

Reduce Method Calls

Instead of calling .lower() every time when you check for matches, simply call it on the input() function itself:

user_skill = input("Enter the skill you wish to use: ").lower()

Type Hints

Use type hints to display what types of parameters are accepted, and what type(s) of value(s) are being returned from the function/method.

From this

gain_health(self, amount):
success_check(self):

to this

gain_health(self, amount: int) -> None:
success_check(self) -> bool:

Docstrings

You should include a docstring at the beginning of every method, class and module you write. This will allow you to express what these are supposed to do, and allows other programmers to more easily understand what the purpose and "function" of the functions are.

"Main" Method

There isn't really a main method in python. There is a main guard, however. It's a simple if statement that prevents code from being run if you decide to import this module from other programs. It works like so

if __name__ == '__main__':
    # code here

"name" will only be "main" in the module that the program is run from.

\$\endgroup\$
2
  • \$\begingroup\$ Are type hints used for readability only? \$\endgroup\$ Commented Oct 9, 2019 at 0:05
  • \$\begingroup\$ @ClaudeFried Yes, and to let programmers know what types of data to pass, instead of having to validate the data inside the function. \$\endgroup\$
    – Linny
    Commented Oct 9, 2019 at 0:34
3
\$\begingroup\$

Re:

    while is_debilitated:
        mobility = 0

I'm guessing you were hoping that this would set mobility to zero whenever is_debilitated was true? (All that can do is either nothing at all or create an infinite loop.)

I'd suggest something like

def get_mobility(self):
        if self.is_debilitated:
            return 0
        else:
            return self.mobility

And any other effects that temporarily modify mobility could go there. Then elsewhere you'd put

if defense >= defender.get_mobility():

in place of

if defense >= defender.mobility:

(Apologies if I got any of the Python grammar wrong - I'm more of a C++ guy...)

\$\endgroup\$
2
  • 2
    \$\begingroup\$ Although as far as I'm aware it's often a good idea to use getters and setters, it's also worth noting that we could do this using property and pretend that mobility is just a normal attribute even though everything you do with it is actually calls to custom getters and setters. \$\endgroup\$ Commented Oct 9, 2019 at 7:37
  • \$\begingroup\$ I feel this could use a more expanded explanation of why that while loop isn't doing anything. It doesn't do anything because it's only run on class instantiation, when is_debilitated is False. A Pythonic solution would be @property over getters. \$\endgroup\$
    – Gloweye
    Commented Oct 28, 2019 at 12:48
2
\$\begingroup\$

When creating a parent class your attributes can be inherited to a subclass. A conventional way to create characters:

class Char:
    def __init__(self, name, hp, strength, gold):
        self.name = name
        self.hp = hp
        self.strength = strength
        self.gold = gold


class Enemy:
    def __init__(self, name, hp, strength, gold):
        self.name = name
        self.hp = hp
        self.strength = strength
        self.gold = gold

#Create player and enemy
player = Char('John Smith', 100, 100, 100)
goblin = Enemy('Goblin', 100, 100, 200)

You can also use the super() method to create subclass that automatically inherits the attributes of the parent class without affecting the parent class.

class Goblin(Char):
    def __init__(self):
        super().__init__(name='Goblin', hp=100, strength=100, gold=200)

This is using the attributes from the parent class Char. You can also add attributes to this class if you want to include any other variables

@Linny

Yes you can, the extending the attributes would just be included in the class your making however you might not want the rest of the functions from the main class. This is why I usually separate the classes. For instance:

class Char:
    def __init__(self, name, hp, strength, gold):
        self.name = name
        self.hp = hp
        self.strength = strength
        self.gold = gold

    def __str__(self): # You may not want to return any values but this will run
      return 'name: {} , hp: {} , strength: {} , gold: {}'.format(self.name, self.hp, self.strength, self.gold)


class Enemy(Char): # You can extend attributes here without changing the parent class
  pass


player = Char('John Smith', 100, 100, 100)
enemy_1 = Enemy('Giant', 1000, 1000, 1000)
print(player)
print(enemy_1)

This gives:

name: John Smith , hp: 100 , strength: 100 , gold: 100
name: Giant , hp: 1000 , strength: 1000 , gold: 1000

Your Subclass may not want the Parent class functions so the first method is a good way to achieve full flexibility in your code.

\$\endgroup\$
2
  • \$\begingroup\$ Wouldn't creating a base Entity class with the starting values, then extending from that class Character(Entity) and Enemy(Entity) be better? Then you wouldn't have to write the same class body two times. \$\endgroup\$
    – Linny
    Commented Oct 28, 2019 at 4:32
  • 1
    \$\begingroup\$ Thats a good question . I have amended the answer to include the reason why it is done different ways \$\endgroup\$
    – Barb
    Commented Oct 28, 2019 at 8:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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