17
\$\begingroup\$

I haven't been learning Python for too long and I was just wondering how this Pokémon style battle looks? It's based off of this: Turn Based Pokémon Style Game. It's my first proper time using classes so I'd love advice or critique on the usage. Also, where I make the CPU more likely to use heal when under 35 health, there must surely be a better way to do that.

# Simple battle simulator in the style of Pokemon.
# author: Prendy

import random

moves = {"tackle": range(18, 26),
         "thundershock": range(10, 36),
         "heal": range(10, 20)}


class Character:
    """ Define our general Character which we base our player and enemy off """
    def __init__(self, health):
        self.health = health

    def attack(self, other):
        raise NotImplementedError


class Player(Character):
    """ The player, they start with 100 health and have the choice of three moves """
    def __init__(self, health=100):
        super().__init__(health)

    def attack(self, other):
        while True:
            choice = str.lower(input("\nWhat move would you like to make? (Tackle, Thundershock, or Heal)"))

            if choice == "heal":
                self.health += int(random.choice(moves[choice]))
                print("\nYour health is now {0.health}.".format(self))
                break
            if choice == "tackle" or choice == "thundershock":
                damage = int(random.choice(moves[choice]))
                other.health -= damage
                print("\nYou attack with {0}, dealing {1} damage.".format(choice, damage))
                break
            else:
                print("Not a valid move, try again!")


class Enemy(Character):
    """ The enemy, also starts with 100 health and chooses moves at random """
    def __init__(self, health=100):
        super().__init__(health)

    def attack(self, other):
        if self.health <= 35:
            # increasing probability of heal when under 35 health, bit janky
            moves_1 = ["tackle", "thundershock", "heal", "heal", "heal", "heal", "heal"]
            cpu_choice = random.choice(moves_1)
        else:
            cpu_choice = random.choice(list(moves))
        if cpu_choice == "tackle" or cpu_choice == "thundershock":
            damage = int(random.choice(moves[cpu_choice]))
            other.health -= damage
            print("\nThe CPU attacks with {0}, dealing {1} damage.".format(cpu_choice, damage))
        if cpu_choice == "heal":
            self.health += int(random.choice(moves[cpu_choice]))
            print("\nThe CPU uses heal and its health is now {0.health}.".format(self))


def battle(player, enemy):
    print("An enemy CPU enters...")
    while player.health > 0 and enemy.health > 0:
        player.attack(enemy)
        if enemy.health <= 0:
            break
        print("\nThe health of the CPU is now {0.health}.".format(enemy))
        enemy.attack(player)
        if player.health <= 0:
            break
        print("\nYour health is now {0.health}.".format(player))
    # outcome
    if player.health > 0:
        print("You defeated the CPU!")
    if enemy.health > 0:
        print("You were defeated by the CPU!")

if __name__ == '__main__':
    battle(Player(), Enemy())
\$\endgroup\$
  • 1
    \$\begingroup\$ Supereffective hits, special conditions (poison ...), critical hits, accuracy, stats personalized for each Pokemon etc.. coming in the next iterations? \$\endgroup\$ – Caridorc Aug 13 '15 at 21:34
  • 1
    \$\begingroup\$ Yeah, I definitely want to make it more complex. Just wanted to see how my basic code was as I'm not too experienced or confident in making complex things \$\endgroup\$ – Prendy Aug 13 '15 at 21:41
13
\$\begingroup\$

Magic numbers

Right off the bat I see some magic numbers

moves = {"tackle": range(18, 26),
         "thundershock": range(10, 36),
         "heal": range(10, 20)}

What does that mean? Being familiar with Pokemon I'd assume damage, or something, but that won't necessarily be apparent to the user.

ABCs

You have your base class, Character. It would benefit from being an abstract base class

For example,

import abc

class Character(metaclass=abc.ABCMeta):

    def __init__(self, starting_health):
        self.current_health = starting_health

    @abc.abstractmethod
    def attack(self, other):
        raise NotImplementedError

I've done a few things here. For one, I've used more clear variable names. The input parameter health is actually the starting_health of the character, while self.health is actually referring to the current_health of the character. Better variable names make code easier to read.

The meat of this is the abc stuff. By giving the Character class a metaclass of abc.ABCMeta (don't worry about what a metaclass is) we're saying that it cannot be instantiated directly if it has any abstract methods or properties. With this definition, if you then tried to do this

char = Character(100)

you would get the following error:

TypeError: Can't instantiate abstract class Character with abstract methods attack

This carries into sub-types as well. It is a way of guaranteeing that all classes you instantiate that should override a method do override it.

Moves

Your moves should probably be classes. This will make it much, much easier to extend this, and simplify some other behaviors. I'd look at something like this

from enum import Enum

DamageTypes = Enum('DamageTypes', 'DAMAGING HEALING STATUS')
Types = Enum('Types', 'ELECTRIC NORMAL')

class Move(metaclass=abc.ABCMeta):

    @abc.abstractproperty
    def damage_type(self):
        return NotImplemented

    @abc.abstractproperty
    def move_type(self):
        return NotImplemented

    @abc.abstractmethod
    def health_change(self, modifiers=None):
        return NotImplemented


class Thundershock(Move):
    _max = 36
    _min = 10

    @property
    def damage_type(self):
        return MoveTypes.DAMAGING

    @property
    def move_type(self):
        return Types.ELECTRIC

    def health_change(self, modifiers=None):
        if modifiers is None:
            return random.randint(self._min, self._max)
        else:
            # Do something here if they have some ability that reduces electric damage, or whatever

I did a few things here. The first was the Enums. Enums let you group related constants together. For example, now I can do something like

if move.move_type is Types.ELECTRIC:
    # Has lightningrod ability, immune to electric moves
    return 0

without having a magic number. I then gave the class some properties (i.e. attributes that I can get/set without using parentheses) as well as some methods (attributes that are functions). This gives you a slightly more well organized codebase, and is easy to extend. Just add a new subclass.

Now in your Player class you can do something like

def attack(self, opponent)
    while True:
        try:
            move = moves[str.lower(input("stuff"))]
        except KeyError:
            print("Not a valid move, try again!")
        else:
            if move.move_type is MoveTypes.HEAL:
                self.health += move.health_change(None)
            elif move.move_type is MoveTypes.DAMAGIN:
                opponent.health -= move.health_change(None)
            else:
                opponent.status = move.status_effect(None)
            break

which to me is much cleaner, and also easier to extend if you add move types. It doesn't rely on the specific strings of the names (can you imagine typing all ~600 moves that exist in Pokemon?) just on what sort of effect they have.

Weighted randomness

I won't repeat everything here, but Ned Batchelder has a good suggestion for how to handle weighted randomness here

Attack order

You should randomise this. Good guys don't always go first :)

String formatting

Instead of doing

print("The health of the CPu is now {0.health}.".format(enemy)

just do

print("The health of the CPU is now {}.".format(enemy.health)

Improving your base class

As per @200_success's answer below, you should expand the functionality of your Character base class. For example, attacking, healing, and taking damage are all shared between characters (mostly). What if you did this?

class Character(metaclass=abc.ABCMeta):

    def __init__(self, starting_health):
        self.current_health = starting_health

    def attack(self, other, modifiers):
        move = self.get_move()
        if move.move_type is MoveTypes.DAMAGING:
            other.damage(move.health_change(modifiers))
        elif move.move_type is MoveTypes.HEAL:
            self.heal(move.health_change(modifiers))
        elif move.move_type is MoveTypes.STATUS:
            other.status(move.status_effect(modifiers))
        else:
            raise NotImplementedError

    @abc.abstractmethod
    def get_move(self):
        raise NotImplementedError

I've consolidated the act of attacking and abstracted away the only part that changes - how they pick their move. Now you can do

 class Player(Character):

     def get_move(self):
         while True:
             move = moves[str.lower(input(""))]
         except KeyError:
             print("No such move")
         else:
             return move

 class Enemy(Character):

     def get_move(self):
         # do some pseudo random stuff
         return move

And you don't have to repeat the attacking logic, and new types of characters only need to override how they pick a move.

\$\endgroup\$
  • 1
    \$\begingroup\$ Bloody hell! Thanks for all this, its really helpful. I definitely forgot about the going first thing, although the CPU definitely doesn't deserve to go first. :) \$\endgroup\$ – Prendy Aug 13 '15 at 22:04
  • 1
    \$\begingroup\$ @Prendy I've updated my answer to address the issue mentioned in 200_success's answer. Please take a look and let me know what you think. \$\endgroup\$ – Dannnno Aug 14 '15 at 16:29
  • \$\begingroup\$ Looks good, I've been trying to understand the Enum thing, I can't seem to get it to work. Like if I try to reference Thundershock.damage_type it doesn't seem to exist. \$\endgroup\$ – Prendy Aug 14 '15 at 20:54
  • 1
    \$\begingroup\$ @Caridorc that isn't strictly true. It works with small integers, booleans, interned strings (in CPython), and things that are actually the same. It never "behaves weirdly" - it behaves exactly like it should. In the case of Enums it is referring to the same member variable, in which case is is appropriate \$\endgroup\$ – Dannnno Aug 15 '15 at 23:59
  • 1
    \$\begingroup\$ @Peter move_type is a property, meaning that while you access it as if it were an attribute, it's really a function. In this case we just return Types.ELECTRIC all the time - but what if one of them used a move/ability/something that changes a Pokemon's type? Then we wouldn't want this to be hardcoded - we'd rather do some computation to determine what the actual type is. In this simple example, you could just do what you asked about, but making it a property allows for more flexibility in the future (although tbh switching to a property is super easy in Python). \$\endgroup\$ – Dannnno Feb 5 '16 at 1:28
6
\$\begingroup\$

The Character base class doesn't really serve a purpose here. In Java, for example, an abstract base class would be essential. In Python, not so much. There are two approaches you could take.

The simple change would just be to eliminate the Character class and use duck typing. The two subclasses aren't inheriting any useful functionality. You could just set self.health in their respective constructors. The attack() method is just an unimplemented stub.

Alternatively, you could define a Character class that has all of the healing and attacking logic. (After all, that logic is shared between Player and Enemy by copy-and-pasting, which is not ideal.) The HumanPlayer and CPU classes would just be responsible for the decision-making process — by prompting for user input and by making random choices, respectively.

\$\endgroup\$

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.