10
\$\begingroup\$

To get a better understanding of objects/methods, I wrote a text-based, turn-based fighting game.

I want to expand by adding new characters with new moves (one of which can alter the way other moves works - I'm thinking of using Decorators for this).

Anyways, I'm open to suggestions, advice, constructive criticism, etc.

import random

class Characters:
    class Fighter:
        def __init__(self,hp=100,atk=75,defen=50,speed=60):
            self.hp = hp
            self.atk = atk
            self.defen = defen
            self.speed = speed

        class Options:
            def punch(self,target,modifier=1.00):
                base = 50/100.0
                damage = (self.atk - target.defen)*base
                target.hp = target.hp - damage
                print(self.Name + " punched " + target.Name + " for %d HP" % damage)
            punch.priority = 1

            def heal(self,target,modifier=1.00):
                self.hp += 15
                if self.hp > 100:
                    self.hp = 100
                print(self.Name + " healed 15 HP")
            heal.priority = 2

            def kick(self,target,modifier=1.00):
                base = 100/100.0
                damage = (self.atk-target.defen)*base
                target.hp = target.hp - damage
                self.hp = self.hp - 10
                print(self.Name + " kicked " + target.Name + " for %d HP" % damage)
            kick.priority = 2

# Declare Marco & Player                
Player = Characters.Fighter()
Player.Name = "You"
Marco = Characters.Fighter()
Marco.Name = "Marco"

# Define Turn Action
def play(Player1,Player2):
        # First - Check Options Priority
        if Player1.Turn < Player2.Turn:
            Player1.move(Player1,Player2)

            if Player2.hp > 0:
                Player2.move(Player2,Player1)
            else:
                Player2.hp = 0
                return Player2.hp

        elif Player1.Turn > Player2.Turn:
            Player2.move(Player2,Player1)

            if Player1.hp > 0:
                Player1.move(Player1,Player2)
            else:
                Player1.hp = 0
                return Player1.hp

        # Second - Check Player's Speed
        else:
            if Player1.speed > Player2.speed:
                Player1.move(Player1,Player2)

                if Player2.hp > 0:
                    Player2.move(Player2,Player1)
                else:
                    Player2.hp = 0
                    return Player2.hp

            elif Player1.speed < Player2.speed:
                Player1.move(Player1,Player2)

                if Player2.hp > 0:
                    Player2.move(Player2,Player1)
                else:
                    Player2.hp = 0
                    return Player2.hp
            # Third - Ranomdly Choose
            else:
                random.choice(range(2))
                if random.choice == 0:
                    Player1.Turn = Player2.Turn+1
                    play(Player1,Player2)
                else:
                    Player1.Turn = Player2.Turn-1
                    play(Player1,Player2)



### Fight Sequence ###
# Start
print("\n 3...2...1...")
print("FIGHT!")

# Begin Fight/Conditions for Ending
while Player.hp > 0 and Marco.hp > 0:

    # Situational Update
    print("\n \t You have %d health points." %Player.hp)
    print("\n \t Marco has %d health points." %Marco.hp)
    print("\n \t It is your move!!!")

    # Choosing the Options - Correctly
    # Place Holder let's the whole loop start/error check.
    playerchoice = 'place holder'

    while playerchoice not in [1,2,3]:
        playerchoice = input("\n Which move would you like to use: \
            \n 1) Punch -- Base Damage of 50  (Weak) \
            \n 2) Kick  -- Base Damage of 100 (Moderate), but lose 10 HP\
            \n 3) Heal  -- Heal for 15 HP. \
            \n -->")

    # Convert Player Choice to String for Dict-Search
    if playerchoice == 1:
        playerchoice = "punch"
    elif playerchoice == 2:
        playerchoice = "kick"
    else:
        playerchoice = "heal"       

    # Randomly Choose Marco's move from list of Options Options
    # Marco shouldn't be frequently healing at higher HP (Hence, if/else)
    if Marco.hp < random.choice( range(80,100) ):
        marcochoice = random.choice(Marco.Options.__dict__.keys()[1:4])
    else:
        marcochoice = random.choice(Marco.Options.__dict__.keys()[2:4])

    Marco.move = Marco.Options.__dict__[marcochoice]
    Player.move = Player.Options.__dict__[playerchoice]
    print(Marco.move)
    print (Player.move)

    ### Determine who goes first
    Player.Turn = Player.move.priority
    Marco.Turn = Marco.move.priority
    print Marco.Turn
    print Player.Turn
    # First - Check Options Priority:   
    play(Player,Marco)
\$\endgroup\$
0

3 Answers 3

3
\$\begingroup\$
  • General

    • PEP8 calls for snake_case for methods, and reserves Capitalized names for classes. Using Player1 for variable is not right.

    • Returning values is much more explicit than state mutations. I strongly recommend passing players' Turns to play as variables rather then Player's properties.

    For the rest of review I'll use original style.

  • Move selection

    belongs to the player, not the main loop. For example,

    class Human(Characters.Fighter):
        ....
        def chose(self):
            follow_the_logic_here()
            self.move = self.Options.__dict__[playerchoice]
            self.Turn = self.move.priority
    

    and similar for Marco (who is really Robot). The main loop simplifies to

        while (....):
            situation_update()
            Player.chose()
            Marco.chose()
            play(Player, Marco)
    
  • play looks a bit convoluted, especially the recursive call. I recommend to factor out the rules of move order into a separate function, as in

    def play(Player1, Player2):
        p1, p2 = order_of_move(Player1, Player2)
        p1.move(p1, p2)
        if p2.hp > 0:
            p2.move(p2, p1)
    

    and

    def order_of_move(p1, p2):
        if p1.Turn < p2.turn:
            return (p1, p2)
        if p1.Turn > p2.Turn:
            return (p2, p1)
    
        # Here p1.Turn == p2.Turn for sure
        if p1.speed > p2.speed:
            return (p1, p2)
        if p1.speed < p2.speed:
            return (p2, p1)
    
        # Here we must randomize
        ....
    
\$\endgroup\$
3
  • \$\begingroup\$ Thank you. For order_of_move(p1,p2), why did you choose to use several if statements instead of if/elif/else? \$\endgroup\$ Feb 12, 2016 at 7:18
  • \$\begingroup\$ @DmitriValentine I see it more clear. An immediate return supersedes else. \$\endgroup\$
    – vnp
    Feb 12, 2016 at 18:21
  • \$\begingroup\$ but won't it check each if statement anyways? This would end up giving the turn to the final if statements which depend on the random choice (or what you have written the speed and not the move's internal priority). \$\endgroup\$ Feb 12, 2016 at 18:27
2
\$\begingroup\$

So, I'm no Pythonista, but it's not often that you see nested classes. That's not to say there aren't uses that I don't know of. To change your class into what I would think is a more Pythonic layout, I might do something like:

class Fighter:
    def __init__(self, name, hp=100, attack=75, defense=50, speed=60):
        self.name = name
        self.hp = hp
        self.attack = attack
        self.defense = defense
        self.speed = speed

    def punch(self, target, modifier=1.00):
        base = 50/100.0
        damage = (self.attack - target.defense) * base
        target.hp = target.hp - damage
        print(self.name, "punched", target.name, "for %d HP" % damage)
    punch.priority = 1

    def heal(self, target, modifier=1.00):
        self.hp += 15
        if self.hp > 100:
            self.hp = 100
        print(self.name, "healed 15 HP")
    heal.priority = 2

    def kick(self, target, modifier=1.00):
        base = 100/100.0
        damage = (self.attack - target.defense) * base
        target.hp = target.hp - damage
        self.hp = self.hp - 10
        print(self.name, "kicked", target.name, "for %d HP" % damage)
    kick.priority = 2

Note that this uses the fact that, in Python, a function can have member data. The line punch.priority = 1 works perfectly fine. I also expanded your variable names like atk into attack as it's more readable and you won't have to remember "how did I abbreviate that" later on. Also, I added a "name" attribute. Finally, I changed the use of print to use comma-separated arguments. The default delimiter is a space, so this will work in the same way and reads better.

Next, we need to update the following few lines based on the previous changes. This is now more concise.

# Declare Marco & Player                
Player = Fighter("you")
Marco = Fighter("Marco")

Now, for you function play, we're going to need to make some structural changes. Manually binding the Turn class attribute, when it hasn't been defined, makes the book-keeping here more complicated than it needs to be. Maybe, we could add a class attribute like next_move to keep track of this.

class Fighter:
    def __init__(...):
        # ...
        self.next_move = None

Then, to also help things out, let's embed the hp check into the move definition. We can do this to all of the moves with minimal code using a construct called a decorator. Let's add a function called hp_check, which takes a function and returns a function, outside of our class. Then, let's update our class methods using @hp_check right before the def line.

def hp_check(function):
    def decorator(self, *args, **kwargs):
        if self.hp > 0:
            function(self, *args, **kwargs)
        else:
            print(self.name, "is dead and cannot perform actions")
    return decorator

class Fighter:
    def __init__(...):
        # method body

    @hp_check
    def punch(...):
        # method body

    @hp_check
    def heal(...):
        # method body

    @hp_check
    def kick(...):
        # method body

With this work done, we can make the play function easier to read.

def play(Player1,Player2):
    # First - Check Options Priority
    if Player1.next_move is None and Player2.next_move is None:
        pass

    elif Player1.next_move is None:
        Player2.next_move(Player1)

    elif Player2.next_move is None:
        Player1.next_move(Player2)

    elif Player1.next_move.priority > Player2.next_move.priority:
        Player1.next_move(Player2)
        Player2.next_move(Player1)

    elif Player1.next_move.priority < Player2.next_move.priority:
        Player2.next_move(Player1)
        Player1.next_move(Player2)

    # Second - Check Player's Speed
    elif Player1.speed > Player2.speed:
        Player1.next_move(Player2)
        Player2.next_move(Player1)

    elif Player1.speed < Player2.speed:
        Player2.next_move(Player1)
        Player1.next_move(Player2)

    # Third - Randomly Choose
    else:
        if random.choice(range(2)) == 0:
            Player1.next_move(Player2)
            Player2.next_move(Player1)
        else: 
            Player2.next_move(Player1)
            Player1.next_move(Player2)

Finally, we can do make the main fight sequence more concise by using an OrderedDict.

from collections import OrderedDict
move_choices = OrderedDict((('1', 'punch'), ('2', 'kick'), ('3', 'heal')))

while Player.hp > 0 and Marco.hp > 0:
    player_choice = 'place holder'
    while player_choice not in move_choices:
        player_choice = input('')
    player_method = getattr(Player, player_choice)       
    setattr(Player, 'next_move', player_method)

    if Marco.hp < random.choice(range(80,100)):
        marco_choice = random.choice(move_choices.values())
    else:
        marco_choice = random.choice(move_choices.values()[:-1])

    marco_method = getattr(Marco, marco_choice)
    setattr(Marco, 'next_move', marco_method)

    play(Player, Marco)

Now, this last snippet ramps things up a bit. Instead of referencing __dict__, which I would try to avoid, it uses the built-in functions getattr and setattr which can be used to get and set object attributes. It also uses an OrderDict in order to simplify the input process. I can comment further on anything you want, but hopefully you're at the point where you can go through line by line and figure out what's going on. It's usually helpful to open up an interpreter to test things out. I removed the comments and input lines for brevity, but the functionality should be the same.

In addition to what was mentioned above, I also made a lot of small style changes for readability. Let me know if you have any questions or curiosities. At the end, my updated version of your code looks like:

def hp_check(function):
    def decorator(self, *args, **kwargs):
        if self.hp > 0:
            function(self, *args, **kwargs)
        else:
            print(self.name, "is dead and cannot perform actions")
    return decorator

class Fighter:
    def __init__(self, name, hp=100, attack=75, defense=50, speed=60):
        self.name = name
        self.hp = hp
        self.attack = attack
        self.defense = defense
        self.speed = speed

    @hp_check
    def punch(self, target, modifier=1.00):
        base = 50/100.0
        damage = (self.attack - target.defense) * base
        target.hp = target.hp - damage
        print(self.name, "punched", target.name, "for %d HP" % damage)
    punch.priority = 1

    @hp_check
    def heal(self, target, modifier=1.00):
        self.hp += 15
        if self.hp > 100:
            self.hp = 100
        print(self.name, "healed 15 HP")
    heal.priority = 2

    @hp_check
    def kick(self, target, modifier=1.00):
        base = 100/100.0
        damage = (self.attack - target.defense) * base
        target.hp = target.hp - damage
        self.hp = self.hp - 10
        print(self.name, "kicked", target.name, "for %d HP" % damage)
    kick.priority = 2


def play(Player1,Player2):
    # First - Check Options Priority
    if Player1.next_move is None and Player2.next_move is None:
        pass

    elif Player1.next_move is None:
        Player2.next_move(Player1)

    elif Player2.next_move is None:
        Player1.next_move(Player2)

    elif Player1.next_move.priority > Player2.next_move.priority:
        Player1.next_move(Player2)
        Player2.next_move(Player1)

    elif Player1.next_move.priority < Player2.next_move.priority:
        Player2.next_move(Player1)
        Player1.next_move(Player2)

    # Second - Check Player's Speed
    elif Player1.speed > Player2.speed:
        Player1.next_move(Player2)
        Player2.next_move(Player1)

    elif Player1.speed < Player2.speed:
        Player2.next_move(Player1)
        Player1.next_move(Player2)

    # Third - Randomly Choose
    else:
        if random.choice(range(2)) == 0:
            Player1.next_move(Player2)
            Player2.next_move(Player1)
        else: 
            Player2.next_move(Player1)
            Player1.next_move(Player2)


Player = Fighter("you")
Marco = Fighter("Marco")

from collections import OrderedDict
move_choices = OrderedDict((('1', 'punch'), ('2', 'kick'), ('3', 'heal')))

while Player.hp > 0 and Marco.hp > 0:
    player_choice = 'place holder'
    while player_choice not in move_choices:
        player_choice = input('')
    player_method = getattr(Player, player_choice)       
    setattr(Player, 'next_move', player_method)

    if Marco.hp < random.choice(range(80,100)):
        marco_choice = random.choice(move_choices.values())
    else:
        marco_choice = random.choice(move_choices.values()[:-1])

    marco_method = getattr(Marco, marco_choice)
    setattr(Marco, 'next_move', marco_method)

    play(Player, Marco)
\$\endgroup\$
2
\$\begingroup\$

I think you have a few odd patterns to try and solve the problem of choosing an attack and discerning its priority, but there are simpler, more Pythonic approaches to doing this. I would instead make a function for Character called choose_attack.

(As an aside I don't know why you even have Character here. It seems like you'd want to set up proper inheritance down the line more than anything else.)

So in choose_attack, you can ask the player what attack to use, then based on the string you can return the relevant function and its priority. Python allows you to pass references to practically anything, and allows you to return multiple parameters at once, so it's trivial to do something like:

return self.punch, 1  # Attack, Priority

This makes it a lot easier to avoid some of your stranger choices. Now you don't need to attach parameters to functions, nor do you need a subclass of Options, and you've also added the attack choosing logic to the Character class instead of leaving it in the main game logic.

For the most robust solution, I'd store the priority values in a dictionary, paired with the string command the user enters, and then I can return that dictionary value.

Like this:

class Character:

    PRIORITY = {
                "punch": 1
                "heal": 2
                "kick": 2
               }

    def choose_attack():
        while playerchoice not in (1,2,3):
            playerchoice = input("\n Which move would you like to use: \
                \n 1) Punch -- Base Damage of 50  (Weak) \
                \n 2) Kick  -- Base Damage of 100 (Moderate), but lose 10 HP\
                \n 3) Heal  -- Heal for 15 HP. \
                \n -->")

        # Convert Player Choice to String for Dict-Search
        if playerchoice == 1:
            return self.punch, PRIORITY['punch']
        elif playerchoice == 2:
            return self.kick, PRIORITY['kick']
        else:
            return self.heal, PRIORITY['heal']

Now you can just call ie like this:

attack, priority = Player.choose_attack()

And you can just call attack like a normal function, but it will specifically refer to Player.

\$\endgroup\$

Your Answer

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

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