12
\$\begingroup\$

This is based off the popular Pokemon Turn Based project (here).

GOAL

Write a simple game that allows the user and the computer to take turns selecting moves to use against each other. Both the computer and the player should start out at the same amount of health (such as 100), and should be able to choose between the three moves:

  • The first move should do moderate damage and has a small range (such as 18-25).

  • The second move should have a large range of damage and can deal high or low damage (such as 10-35).

  • The third move should heal whoever casts it a moderate amount, similar to the first move.

After each move, a message should be printed out that tells the user what just happened, and how much health the user and computer have. Once the user or the computer's health reaches 0, the game should end.

SUBGOALS

  • When someone is defeated, make sure the game prints out that their health has reached 0, and not a negative number.

  • When the computer's health reaches a set amount (such as 35%), increase it's chance to cast heal.

  • Give each move a name.

I am new to Python and I am trying to use OOP. Did I use my classes properly and is there a way to reduce the number of if statements in my code? Since this is my first code review, how is the formatting, number of comments, etc?

import random

class Pokemon:

    """Blueprint for turn based Pokemon battle"""

    def __init__(self, attack_choice):

        self.__attack_choice = attack_choice


    def attack(self):

        if self.__attack_choice == 1:
            attack_points = random.randint(18,25)
            return attack_points

        elif self.__attack_choice == 2:
            attack_points = random.randint(10,35)
            return attack_points

        else:
            print("That is not a selection. You lost your turn!")

    def heal(self):

        heal_points = random.randint(18,25)
        return heal_points    

###########################################################################

user_health = 100
mew_health = 100
battle_continue = True

while battle_continue == True:
    print("\nATTACK CHOICES\n1. Close range attack\n2. Far range attack\n3. Heal")
    attack_choice = eval(input("\nSelect an attack: "))

    # Mew selects an attack, but focuses on attacking if health is full.  
    if mew_health == 100:
        mew_choice = random.randint(1,2)

    else:
        mew_choice = random.randint(1,3)

    mew = Pokemon(mew_choice)
    user_pokemon = Pokemon(attack_choice)

    # Attacks by user and Mew are done simultaneously.
    if attack_choice == 1 or attack_choice == 2:
        damage_to_mew = user_pokemon.attack()
        heal_self = 0
        print("You dealt",damage_to_mew,"damage.")

    if mew_choice == 1 or mew_choice ==2:
        damage_to_user = mew.attack()
        heal_mew = 0
        print("Mew dealt", damage_to_user, "damage.")

    if attack_choice == 3:
        heal_self = user_pokemon.heal()
        damage_to_mew = 0
        print("You healed",heal_self,"health points.")

    if mew_choice == 3:
        heal_mew = mew.heal()
        damage_to_user = 0
        print("Mew healed", heal_mew, "health points.")

    user_health = user_health - damage_to_user + heal_self
    mew_health = mew_health - damage_to_mew + heal_mew

    # Pokemon health points are limited by a min of 0 and a max of 100.
    if user_health > 100:
        user_health = 100

    elif user_health <= 0:
        user_health = 0
        battle_continue = False

    if mew_health > 100:
        mew_health = 100

    elif mew_health <= 0:
        mew_health = 0
        battle_continue = False

    print("Your current health is", user_health)
    print("Mew's current health is", mew_health)

print("Your final health is", user_health)
print("Mew's final health is", mew_health)

if user_health < mew_health:

    print("\nYou lost! Better luck next time!")

else:

    print("\nYou won against Mew!")
\$\endgroup\$
  • 4
    \$\begingroup\$ Welcome to Code Review! I hope you get some great answers. Please include a (possible shortened) description of the task you want to accomplish in case the external description is somehow no longer available. \$\endgroup\$ – AlexV Apr 10 at 20:55
  • \$\begingroup\$ I suspect you have a typo, as the code presented doesn't follow the spec: heal_points = random.randint(18,35) does not have the same range as attack option one, topping out at 35 instead of 25 \$\endgroup\$ – TemporalWolf Apr 10 at 22:48
  • \$\begingroup\$ OOP, as it is typically practiced in languages like C# and Java, is not strongly encouraged in Python. Python's late binding on all names (include modules, classes, and unattached methods) negates many of the benefits of pervasive object use in other languages. More procedurally or functionally minded approaches are more common and often (or maybe usually) simpler. \$\endgroup\$ – jpmc26 Apr 10 at 23:30
  • \$\begingroup\$ Have you checked Pokemon Showdown's server code? In particular how various things are structured. \$\endgroup\$ – Voile Apr 11 at 4:53
  • \$\begingroup\$ @Voile I will take a look at the server code. I am still new to programming and Github in general so code is fairly foreign to me. Is there something specific that you would like me to look at? \$\endgroup\$ – Citrus-Code Apr 11 at 13:44
7
\$\begingroup\$

You have chosen a great problem to begin with, however there are a few things you get wrong about OOP. OOP is not simply about using a class for "everything", it's a kind of way to think about a problem.

To begin with, it helped me a lot to think about a class as a prototype of a "thing". In your case the "thing" would be a Pokemon. A Pokemon can do certain things. In your simplified versions that would be 1. attack another Pokemon and 2. heal itself. Often these actions are reflected in the classes's methods. I think you mostly understood that. What else is there about the Pokemon/"thing" it has certain properties that describe it. I would say that's an aspect you did not think about. A property could be name, color, ... or it's health status. We also learn that the health can only be between 0 and 100.

So with this on our mind, let's think of a new design for class Pokemon:

class Pokemon:
    """Blueprint for a new pokemon"""

    def __init__(self):
        self._health = 100
        #    ^--- the leading _ is a convention to mark internal values

    @property
    def health(self):
        """The health of the Pokemon which is between 0 and 100"""
        return self._health

    @health.setter
    def health(self, new_health):
        """Set the new heath value"""
        # here the health limits are enforced
        self._health = min(100, max(0, new_health))

    def attack(self, other, choice):
        """Attack another Pokemon with the chosen attack (1 or 2)

        This function also returns the raw amount of random damage dealt. The
        amount of damage dealt depends on the attack type.
        """
        if choice == 1:
            attack_points = random.randint(18, 25)
        elif choice == 2:
            attack_points = random.randint(10, 35)
        else:
            print("That is not a selection. You lost your turn!")
            attack_points = 0
        other.health -= attack_points
        return attack_points

    def heal(self):
        """Heal the Pokemon"""
        heal_points = random.randint(18, 35)
        self.health += heal_points
        return heal_points

A lot of this should look familiar to you, since the main work is still done by the code you've written. What is new that the Pokemon now has health. If your unfamiliar with properties in Python just think of them as synthatic sugar on what would normally be done using getter and setter functions in other languages. There is a great SO post with a nice explanation on properties. In addition to that, attack and heal now handle the update of the health value for the Pokemon, as well as the opponent. This allows us to write up a simple battle in a very concise way:

mew = Pokemon()
user = Pokemon()
mew.attack(user, 1)
print(f"User health after attack: {user.health}")
user.heal()
print(f"User health after heal: {user.health}")
mew.heal() # health should still only be 100
print(f"Mew's health after 'illegal' heal: {user.health}")

which prints for example:

User health after attack: 75
User health after heal: 100
Mew's health after 'illegal' heal: 100

No additional variables that need to track health status, no checking on the health limits. Everything is nicely encapsulated into the Pokemon class. As DaveMongoose pointed out in his comment, a drawback of this approach is that the Pokemon can not be defeated as long as it heals after an attack, no matter how much damage it took.


Short break on style and other conventions
Another thing that has changed in contrast to your solution is the documentation I added to each function. Python has an "official" Style Guide (which is worth a read on its own) with a section on how to write these so called docstrings here.

It also features guidelines on where to use blank lines and where not to use them. In my oppinion the excessive use of blank lines in your code hinders readability more than it does help to structure the code.

I also used only a single leading underscore for my internal value instead of two as you did. The Style Guide also has you covered on this topic. In general you should always use a single leading underscore to mark functions and variables/members as internal. For more details on what happens if you use two leading underscores, follow the link above.


After that short intermezzo, let's look on how the battle simulation looks with the new class:

def battle_simulation():
    """Run a simple interactive Pokemon battle simulation"""
    mew = Pokemon()
    user_pokemon = Pokemon()
    while True:
        print("\nATTACK CHOICES\n1. Close range attack\n2. Far range attack\n3. Heal")
        attack_choice = int(input("\nSelect an attack: "))
        # DON'T use eval on user input, this can be dangerous!

        # Mew selects an attack, but focuses on attacking if health is full.
        mew_choice = random.randint(1, 2 if mew.health == 100 else 3)
        # this is your original distinction just condensed into a single line

        # Attacks by user and Mew are done simultaneously
        # with the changes to Pokemon, there is no need to save all the
        # intermediate damage/heal values -> nice and short code
        if attack_choice != 3:
            print(f"You dealt {user_pokemon.attack(mew, attack_choice)} damage.")

        if mew_choice != 3:
            print(f"Mew dealt {mew.attack(user_pokemon, mew_choice)} damage.")

        if attack_choice == 3:
            print(f"You healed {user_pokemon.heal()} health points.")

        if mew_choice == 3:
            print(f"Mew healed {mew.heal()} health points.")

        if mew.health == 0 or user_pokemon.health == 0:
            break

        print(f"Your current health is {user_pokemon.health}")
        print(f"Mew's current health is {mew.health}")

    print(f"Your final health is {user_pokemon.health}")
    print(f"Mew's final health is {mew.health}")

    if user_pokemon.health < mew.health:
        print("\nYou lost! Better luck next time!")
    else:
        print("\nYou won against Mew!")


if __name__ == "__main__":
    battle_simulation()

As you can see, we got rid of all the variables needed to store damage and heal values since everything is no done internally in the Pokemon class. With this, the code becomes quite a bit shorter, clearer and easier to read. Cool, isn't it?

A thing you will see me use quite often are the so called f-strings. They take arbitrary Python expressions (function calls, variables, ...) and allow to incorporate them into a formatted string. If you want to know more about them, I would recommend this blog post.

Happy Coding!

\$\endgroup\$
  • \$\begingroup\$ An excellent answer, but there's a possibly unwanted side effect of how you're constraining health in the setter and resolving healing after damage - it's impossible for either player to lose on a turn when they heal. It might be best to allow health to exceed the limits during a turn and then constrain it at the end - this way healing and damage are effectively resolved at the same time, and it would still be possible for one side to lose because the damage exceeded their healing + remaining health. \$\endgroup\$ – DaveMongoose Apr 11 at 16:38
  • \$\begingroup\$ Excellent catch. I will add a note that this is something one would have to consider. \$\endgroup\$ – AlexV Apr 11 at 17:42
  • \$\begingroup\$ Thanks for the resources and improvements. Is there a way to avoid a simultaneous attack? Considering that this is more turn based, is there a way that I can the User can go first, see damage on Mew then Mew does its attack? Would I have to consider subclasses for this to work? \$\endgroup\$ – Citrus-Code Apr 16 at 17:23
  • 1
    \$\begingroup\$ I see no need for subclasses here. You would have to rearrange the ifs and check if Mew is down to 0 health before allowing it to attack. Then after Mew's attack check if the player is down to 0 before going to the next round. \$\endgroup\$ – AlexV Apr 16 at 17:55
14
\$\begingroup\$

The heal method and the handling of health points strike me as odd. In the current setup,

  • heal does not heal; it simply returns a random number. It also currently has no direct relation to any part of the Pokemon class, so it doesn't really make sense as a method of the class.

  • The health points of each Pokemon are "loose" in the script, along with instances of the Pokemon class. You have a Pokemon class representing a Pokemon, and it makes sense that health would be an attribute of a Pokemon itself.

The design can be improved by doing something like (stripped down):

class Pokemon:
    def __init__(self, start_health):
        self.hp = start_health

    def heal(self, heal_amount):
        self.hp += heal_amount

    def hurt(self, damage):
        self.hp -= damage

Now, you can do something like:

mew = Pokemon(100)
mew.hurt(50) # Ow
mew.heal(49) # Back to almost full
print(mew.hp) # Prints 99

And you don't need to have loose health values floating around in the code for each Pokemon in use. Using the method like this also lets you check the health after healing to ensure that you didn't exceed the maximum health allowed. As long as you're going to make use of a class, you can benefit by having it encapsulate all the data (like health) that's directly relevant to it.

I also decided to not have heal generate the random amount to heal for the following reasons:

  • Are you sure that every time you want to heal you want it to be a random value, and are you sure you will always want it to be a random value in the range (10, 25]? As you mentioned, what about potions? Are you sure you want potion healing to be random, and always in that range? What about when Pokemon level up and have more health? Are you sure that you'll only want to heal them in that narrow range? The Pokemon class simply does not have enough information to always decide how much Pokemon should be healed by, and that shouldn't be its responsibility in the first place anyways.

  • Using random data complicates testing. Say you want to add tests to ensure the correctness of the hurt and heal methods. You would expect this to always pass:

    start_health = 100
    poke = Pokemon(start_health)
    poke.hurt(50)
    poke.heal(50)
    
    # A simplified test
    print("Test Passed?", poke.hp == start_health)
    

    If the methods are correct, you would expect this test to always pass. With random data though, your tests can't be as definitive. You may know that it was hurt by some value, then healed by some value, but that isn't enough information to ensure correct functionality (unless you're only testing that it produces results in a certain range).

    For small toy projects like this, testing isn't really necessary (although it's always good to practice). When you start dealing with larger projects split across multiple files, written by multiple people, and changing code that you haven't looked at in potentially months, you need tests to ensure that code stays correct from one change to the next. Making testing easier helps ensure the validity of the tests, and prevents you from wasting your time trying to Jerry-rig some test in place after the fact.

If some code wants to hurt the Pokemon by a random amount, they can generate the random data themselves to pass in to hurt.


while battle_continue == True: has a redundant condition. while (and if) just check if their condition is truthy. You can just write:

while battle_continue:
\$\endgroup\$
  • \$\begingroup\$ Thanks for your response. Could you help me clarify what you mean by the issues caused by the random values for heal? Would there be a constant value for healing? Like how potions work? \$\endgroup\$ – Citrus-Code Apr 11 at 15:20
  • 3
    \$\begingroup\$ @citruslipbalm What if, in the future, you wanted to add different healing moves or items that healed for different amounts? Or that healed for a constant amount. Pokemon.heal shouldn't decide how much to heal for, it should take an argument and add that amount to the health, then moves or items that heal can specify how much they heal by, and you can, for example, use Pokemon.heal(potion.get_heal_amount). get_heal_amount can then return a constant number, or generate a random number within a range, or get its data elsewhere. \$\endgroup\$ – Skidsdev Apr 11 at 15:25
  • \$\begingroup\$ @citruslipbalm On top of Skidsdev's points, see my new section at the bottom of the answer that elaborates. \$\endgroup\$ – Carcigenicate Apr 11 at 16:32
  • \$\begingroup\$ I would never use heal(-damage). The method name does not allow for this. I would rather insert assert heal_amount >= 0. The alternative would be to use a change_health method (possibly called from heal and hurt). \$\endgroup\$ – allo Apr 12 at 9:54
  • 1
    \$\begingroup\$ @allo Ya, I'm not sure why I added that originally. Removed. \$\endgroup\$ – Carcigenicate Apr 12 at 14:04
12
\$\begingroup\$

Hello and welcome to CodeReview. Congratulations on writing code that is well-formatted and compliant with the basics of the generally-agreed-upon Python coding style.

You have made some errors in organization and structure, however. Let's look at those first:

Organization & Structure

Organization

Your code is "against the wall." This means that if I import your module, the code is going to be run, not just loaded. That's bad because it means you can't import it into the python command line REPL to play around with it, and you can load it into a debugger or a test framework.

The basic solution is to find all your "left-edge" statements, like these:

user_health = 100
mew_health = 100
battle_continue = True

while battle_continue == True:

And move them into a function. Call it main until you have a better name (if you have a lot of code you might break it into more than one function, but main is a good place to start!). Then at the bottom of your module, do:

if __name__ == '__main__:
    main()

That "hook" means that if you run python myfile.py the code will run and the game will play, but if you start the REPL using just python and then type >>> from myfile import * you will have all the functions available and can call them to see what they do in different circumstances. And it also means you can use one of the many Python unit testing frameworks to check your code.

Structure

You have one class, and you don't really use it. The class is syntactically correct, but you aren't treating it as a class because you don't "trust" it. You're still treating it as a collection of functions that you can call.

Let's look at the first part of the problem statement. It's nice and small, so let's just play a game of find the nouns and see if that gives us any ideas about objects:

Write a simple game that allows the user and the computer to take turns selecting moves to use against each other. Both the computer and the player should start out at the same amount of health (such as 100), and should be able to choose between the three moves:

  • The first move should do moderate damage and has a small range (such as 18-25).

  • The second move should have a large range of damage and can deal high or low damage (such as 10-35).

  • The third move should heal whoever casts it a moderate amount, similar to the first move.

After each move, a message should be printed out that tells the user what just happened, and how much health the user and computer have. Once the user or the computer's health reaches 0, the game should end.

(I flagged "heal" as a noun because it's really the opposite of "damage".)

The general rule for OOP is that objects are nouns and methods are verbs. So our set of potential objects includes:

  1. Game
  2. User
  3. Computer
  4. Turn
  5. Move
  6. Health
  7. Damage
  8. Range
  9. Heal
  10. Message

With those in mind, it seems like User and Computer are likely to be either different parts (methods or functions) of the Game object, or they will be two different implementations of a Player interface (or subclasses of a Player class).

The Turn might be an object, but it seems more likely that "take turns" is a verb on the Game. A Move seems very likely to be an object, since there are so many details and requirements about them.

Health seems like an attribute, since we get a starting number provided, and both Heal and Damage both seem like verbs affecting health more than separate attributes or objects. The Range seems like an attribute of the various Move objects, but an internal attribute - when you invoke a move, it will adjust a player's health by some internally-computed amount.

Message is probably just a string. It's an object, but of a type that comes built-in. There is one key question, though: are turns sequential or simultaneous? If turns are sequential, messages can just be printed as they happen. If turns are simultaneous, messages might have to be computed after the results of both moves are understood. (What happens if both players kill each other during the same turn?)

Pro-Tips

Move class

Having Move as a class would be an example of the Command pattern, and might look something like this:

from abc import ABC, abstractmethod

class Move(ABC):
    ''' Abstract game move. (Command pattern.) Perform an action when
        invoked.
    '''
    def __init__(self, name):
        self.name = name

    @abstractmethod
    def execute(self, us: 'Player', them: 'Player') -> str:
        ''' Perform an operation like damaging them, or healing us. '''
        return f"{us.name} uses {self.name}! It's super effective!"

(Note: You could also construct the Move objects with the us/them attributes configured during __init__, instead of passed as parameters.)

Random choices

You should eliminate all uses of if that involve random choices. You are randomly choosing a move: use a list or tuple of Move objects and compute the index of the object, then pass that object around. You are randomly generating damage or healing: compute the number, then apply that number to the appropriate player health attribute. The only if statement you need is to guard against going above or below the maximum or minimum health values (and those aren't random)!

\$\endgroup\$
  • \$\begingroup\$ Interesting idea. Would have never thought of this. \$\endgroup\$ – AlexV Apr 10 at 22:34
  • 2
    \$\begingroup\$ TIL "code is against the wall" = a lot of left-adjusted statements / scripting \$\endgroup\$ – aaaaaa Apr 11 at 0:21
  • \$\begingroup\$ "This means that if I import your module, the code is going to be run, not just loaded" Hard disagree that this is a bad thing. If I write a module that executes some stuff, I would imagine it's not to be imported, or it's doing some necessary set up. If I find that I want to import it for some reason, I refactor the portion that I want to import into its own module, and import that. Adding 4 spaces of indentation to protect people importing something ridiculous is an anti-pattern in my opinion. \$\endgroup\$ – Adam Barnes Apr 11 at 23:42
  • \$\begingroup\$ Thank you for your response. I wanted to answer your comment re: if the attacks are simultaneous or turn based/random. If I wanted more turn based, are subclasses more appropriate? The way I am currently seeing it is assigning the user or Mew 1 or 2 and have a random number generated which will activate the appropriate subclass. Please let me know if I am totally off with this. \$\endgroup\$ – Citrus-Code Apr 16 at 17:27
  • \$\begingroup\$ Subclasses are orthogonal to messages. But what subclasses would you assign? \$\endgroup\$ – Austin Hastings Apr 16 at 21:34

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.