Yhyjyk

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,35)
 return heal_points 

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

user_health = 100
mew_health = 100
battle_continue = True

while battle_continue == True:
 print("nATTACK CHOICESn1. Close range attackn2. Far range attackn3. 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!")

The heal method and the handling of health points strikes 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. 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
 # Or, to reuse existing methods
 #self.heal(-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. That can cause problems with testing since you can't force it to heal by a certain amount. Allow the user to pick how the data is supplied unless you have a good reason to restrict it. If they want to hurt the Pokemon by a random amount, they can generate the random data themselves to pass it in.

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:

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)!

Conceptual

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.

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 CHOICESn1. Close range attackn2. Far range attackn3. 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!

1) The doc comment in the class should give information for that class and not for the whole program.

2) I don't see why the Pokemon should be initialised with an attack choice if it can do multiple attacks. I think the choice should be passed as a parameter to the attack method. Instead it will make more sense for the class to hold the health and the logic for the damage which is currently in the while loop.

3) battle_continue == True can be simplified to only battle_continue and can be renamed to battle_in_progress.

4) The print statement: print("nATTACK CHOICESn1. Close range attackn2. Far range attackn3. Heal") can be separated into multiple lines for better readability.

5) The moves can be an Enum instead of magic numbers.

6) The move selection should have exception handling for invalid inputs.

7) The attack function can take as a parameter the target of the attack and deduce the life points.

8) Some of the other functionality is better suited as methods on the class e.g checking if the health is 0, applying damage to a Pokemon, healing.

Final Implementation:

import random
from enum import Enum


class Pokemon:
 """Class for representing a pokemon"""

 def __init__(self, name):
 self.name = name
 self.health = 100

 def make_move(self, move, target):

 if move == Moves.HEAL:
 self.heal()
 else:
 self.attack(move, target)

 def attack(self, move, target):

 attack_points = None

 if move == Moves.CLOSE_ATTACK:
 attack_points = random.randint(18, 25)

 if move == Moves.RANGE_ATTACK:
 attack_points = random.randint(10, 35)

 if attack_points:
 target.take_damage(attack_points)
 print(self.name, "dealt", attack_points, "damage.")
 else:
 print("That is not a selection. You lost your turn!")

 def heal(self):

 heal_points = random.randint(18, 35)
 self.health = self.health + heal_points

 print(self.name, "healed", heal_points, "health points.")

 if self.health > 100:
 self.health = 100

 def take_damage(self, points):

 self.health = self.health - points

 if self.health < 0:
 self.health = 0

 def is_dead(self):

 return self.health <= 0


class PokemonBot(Pokemon):

 def select_move(self):

 if self.health == 100:
 choice = Moves.get_random_attack()
 else:
 choice = Moves.get_random_move()

 return choice


class Moves(Enum):
 CLOSE_ATTACK = 1
 RANGE_ATTACK = 2
 HEAL = 3

 @staticmethod
 def get_random_attack():
 return random.choice([Moves.CLOSE_ATTACK, Moves.RANGE_ATTACK])

 @staticmethod
 def get_random_move():
 return random.choice([Moves.CLOSE_ATTACK, Moves.RANGE_ATTACK, Moves.HEAL])

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


battle_in_progress = True
mew = PokemonBot("Mew")

name = input("Name your pokemon: ")
user_pokemon = Pokemon(name)

while battle_in_progress:

 print("MOVE CHOICES")
 print("1. Close range attack")
 print("2. Far range attack")
 print("3. Heal")

 move_choice = None

 while move_choice is None:
 try:
 move_choice = Moves(int(input("Select a move: ")))
 except ValueError:
 print("Please enter a valid move!")

 mew_move = mew.select_move()
 user_pokemon.make_move(move_choice, mew)
 mew.make_move(mew_move, user_pokemon)

 print("Current health of", user_pokemon.name, "is", user_pokemon.health)
 print("Current health of", mew.name, "is", mew.health)

 if mew.is_dead() or user_pokemon.is_dead():
 battle_in_progress = False

print("Final health of", user_pokemon.name, "is", user_pokemon.health)
print("Final health of", mew.name, "is", mew.health)

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