Revised version of my Python text game

Question

Following all the good notes I've received from my question, I've edited my code quite a lot, so there are probably even more problems than there was before. Anyway, here are some details of the changes I've made:

Changes

• Added Hero and Map class
• Added Warrior, Mage, Healer subclasses of Hero
• Saved all story/game related text into separate files
• Battles are no longer automated. Characters have options now
• Changed some variables into more appropriate data types
• Removed all global variables

The code is even longer than before (sorry):

import time
import yaml
import pickle
import os
import random
hero_races = ("Human", "Elf", "Orc")
hero_classes = ("Warrior", "Mage", "Healer")
dungeon_map = ("Training Room", "Slime Room", "Chimera Room", "Demon Room")
save_file = "hero_information.txt"
if os.path.exists("TextGameDetails.yaml"):
 with open("TextGameDetails.yaml") as story_file:
 story_info = yaml.load(story_file)
 dungeon_description = story_info["dungeon_description"]
 level_complete = story_info["level_complete"]
else:
 print("Game files are missing!\nCannot run game")
 exit()
class Monster:
 def __init__(self, level):
 self.health = 100 * level
 self.attack = 20 * level
 def battle_action(self, hero):
 damage = random.randrange(self.attack / 2, self.attack)
 if hero.block_stance:
 print("Monster attacks you for {} damage"
 .format(damage / 2))
 hero.health -= damage / 2
 else:
 print("Monster attacks you for {} damage"
 .format(damage))
 hero.health -= damage
class Hero:
 def __init__(self, hero_name, hero_health, hero_attack, race_choice,
 class_choice):
 self.health = hero_health
 self.attack = hero_attack
 self.block_stance = False
 self.name = hero_name
 self.race = race_choice
 self.h_class = class_choice
 def attack_enemy(self, monster):
 damage = random.randrange(int(self.attack / 2), self.attack)
 print(f"You attack the monster for {damage} damage!")
 monster.health -= damage
 print(f"Monster has only {monster.health} health remaining")
 def block(self):
 print("You switch to a defensive stance!")
 self.block_stance = True
 def battle_action(self, monster):
 self.block_stance = False
 print("1- Attack\t2- Defend")
 action_option = int(input("Action: "))
 if action_option == 1:
 self.attack_enemy(monster)
 elif action_option == 2:
 self.block()
 def display_status(self):
 print(f"Welcome back {self.name} the {hero_races[self.race - 1]} "
 f"{hero_classes[self.h_class -1]}, you have {self.health} health")
class Warrior(Hero):
 def __init__(self, hero_name, hero_health, hero_attack, race_choice,
 class_choice):
 super(Warrior, self).__init__(hero_name, hero_health, hero_attack,
 race_choice, class_choice)
class Mage(Hero):
 def __init__(self, hero_name, hero_health, hero_attack, race_choice,
 class_choice):
 super(Mage, self).__init__(hero_name, hero_health, hero_attack,
 race_choice, class_choice)
 self.spell = self.attack * 2
 def cast_spell(self, monster):
 damage = random.randrange(self.spell / 2, self.spell)
 print("You attack the monster with magic for {} damage!"
 .format(damage))
 monster.health -= damage
 print("Monster has only {} health remaining"
 .format(monster.health))
 def battle_action(self, monster):
 self.block_stance = False
 print("1- Attack\t2- Defend\t3- Cast Spell")
 action_option = int(input("Action: "))
 if action_option == 1:
 self.attack_enemy(monster)
 elif action_option == 2:
 self.block()
 elif action_option == 3:
 self.cast_spell(monster)
class Healer(Hero):
 def __init__(self, hero_name, hero_health, hero_attack, race_choice,
 class_choice):
 super(Healer, self).__init__(hero_name, hero_health, hero_attack,
 race_choice, class_choice)
 def battle_action(self, monster):
 self.block_stance = False
 print("1- Attack\t2- Defend\t3- Heal Self")
 action_option = int(input("Action: "))
 if action_option == 1:
 self.attack_enemy(monster)
 elif action_option == 2:
 self.block()
 elif action_option == 3:
 print("You have restored 50 health points")
 self.health += 50
class Map:
 def __init__(self, level):
 self.level = level
 def display_map(self):
 print(f"You are at the {dungeon_map[self.level]}")
 def map_navigation(self, hero):
 while True:
 print("1- Proceed\t2- Status\t3- Map\t4- Save\t5- Exit to Main Screen")
 choice = int(input("What should I do?... "))
 if choice == 1:
 self.level += 1
 return 0
 elif choice == 2:
 hero.display_status()
 elif choice == 3:
 self.display_map()
 elif choice == 4:
 save_game(hero, self)
 elif choice == 5:
 return 1
def create_hero():
 print("What is your name?")
 hero_name = input("My name is: ")
 print("Choose a race")
 print("1- Human\t\t2- Elf\t\t3- Orc")
 race_choice = int(input("My race is: "))
 while True:
 print("Choose a class.")
 print("1- Warrior\t\t2- Mage\t\t3- Healer")
 class_choice = int(input("My class is: "))
 if class_choice == 1:
 hero = Warrior(hero_name, 200, 100, race_choice, class_choice)
 break
 elif class_choice == 2:
 hero = Mage(hero_name, 100, 50, race_choice, class_choice)
 break
 elif class_choice == 3:
 hero = Healer(hero_name, 200, 25, race_choice, class_choice)
 break
 return hero
def save_game(hero, game_map):
 print("Saving Game...\nPlease don't turn off the application.\n...")
 time.sleep(3)
 with open(save_file, 'wb') as info:
 pickle.dump(hero, info)
 pickle.dump(game_map, info)
 print("Your game has been saved successfully!")
def load_game():
 if not os.path.exists(save_file):
 return 1, 1
 with open(save_file, 'rb') as info:
 hero = pickle.load(info)
 game_map = pickle.load(info)
 return hero, game_map
def encounter(hero, game_map):
 print(dungeon_description[game_map.level])
 monster = Monster(game_map.level + 1)
 battle(hero, monster)
 print(level_complete[game_map.level])
def battle(hero, monster):
 print("Prepare to battle!\n...")
 time.sleep(3)
 while monster.health > 0:
 print("Prepare for monster attack! ....")
 time.sleep(2)
 monster.battle_action(hero)
 time.sleep(1)
 print("Your turn to fight back!\n...")
 hero.battle_action(monster)
def main(choices=("New Game", "Load Game", "Exit")):
 while True:
 active_character = False
 for index, choice in enumerate(choices, start=1):
 print(f'{index}- {choice}')
 start_choice = int(input())
 if start_choice == 3:
 print("Game Over.")
 break
 elif start_choice == 1:
 hero = create_hero()
 active_character = True
 game_map = Map(0)
 encounter(hero, game_map)
 elif start_choice == 2:
 hero, game_map = load_game()
 if hero == 1 or game_map == 1: # 1 is the return value if there is no save file
 print("File save not found")
 continue
 active_character = True
 if active_character:
 max_level = len(dungeon_map) - 1
 while game_map.level < max_level:
 if game_map.map_navigation(hero) == 1:
 break
 encounter(hero, game_map)
if __name__ == "__main__":
 main()

Here's my TextGameDetails.yaml file (copy/paste this into a notepad++ file, file extension should be .yaml, not .yml):

--- 
dungeon_description: 
 - "You are standing in the knight's training chambers. In front of you lies your master-at-arms."
 - |-
 You enter the room. You can barely make out anything from this dark area. However you see a glowing blue light straight ahead of you.
 The light burns greater... it begins to make the shape of a creature... a slime!
 - |-
 As you proceed into the room you hear eerie noises from all across the room followed by a dark shape that moves too fast for you to catch. You stand in the middle of the round room you hear a loud growl behind you.
 You turn around to fight a lion no... it's a two-headed creature, both a lion and a giant serpent.
 - |-
 As you step into the room you find it hard to stand as if an unknown force is pushing your body down into the ground. You start shaking your teeth grinding.
 It takes you a moment but you are finally able to regain control of your body. Already tired from your previous trials you push ahead and find an empty throne.
 You blink and all of a sudden there is a dark figure sitting on the chair. He no... It smiles and stands up then walks toward you.
 .... This is your final challenge.
level_complete: 
 - |-
 After several excruciating blows, your sparring session is over.
 Your teacher guides you to the gate where your first true challenge begins to become a full-pledged knight
 - |-
 you fight it off and you (surprisingly) beat your first real enemy.
 Behind the gooey carcass of the now-defeated slime lies another room with the doorknob of a lion
 - |-
 You fight it off and you barely beat this vicious creature. You reach the edge of the room to find a dark door. Red mist flows through the gaps of the door.
 Do you proceed? This may be your final choice
 - "With great struggle, you defeat your final challenge. Now you are ready to become a full-fledged knight."

• Georgy: When I try to run it, it says: Game files are missing! Cannot run game Could you provide your TextGameDetails.yaml as well?
• Axel1212: Sorry about that, I felt like I forgot something when I submitted this. Edited the question.

Answer 1

Congratulations! Your code looks pretty good given the background, and you have done solid work incorporating the suggestions from the previous review cycle.

Decompose More

With that said, I'd like to see you making more use of functions to decompose the activities you are performing. For example, you have some in-line code for loading the game data. Move that to a function that returns an object or data structure. Call that function from inside main.

Likewise, main is a bit too complex and too low-level. You also have some code that looks like it is left over from a previous iteration: active_character. As far as I can tell, active_character is always true when you do the if active_character test, which suggests that you intended to use it for something and changed your mind, or changed the code, or went and had lunch and came back, or something.

I'd suggest you push your load-or-create-player decision down into a lower level function, break the loop if it returns None (meaning "Exit Game"), and otherwise treat the two (load/create) cases as the same: you loaded and are ready to go, or you created and are ready to go.

Centralize UI Actions

You do a lot of work handling and testing user input. You implement menu selection in several places, and print messages to the user all over. I've suggested this in other reviews, and I'll suggest it here: create a TextUI class that has methods for common operations. Then call those methods! They should return whatever-you-need-them-to-return, correctly formatted and checked. If you write a menu, then the result should be an integer or a string (your call). If you ask for a number, the result should be a number. If you ask for a choice, you can pass in key=value pairs to define the choices and return values.

The point of this would be to shrink the code you have, and eliminate any use of print or input statements, replacing them with calls to specific methods that always work and don't need to be checked.

Keep Your Hands to Yourself!

Finally, I'd like to point out this code from your Monster class:

def battle_action(self, hero):
 damage = random.randrange(self.attack / 2, self.attack)
 if hero.block_stance:
 print("Monster attacks you for {} damage"
 .format(damage / 2))
 hero.health -= damage / 2
 else:
 print("Monster attacks you for {} damage"
 .format(damage))
 hero.health -= damage

Count the number of times self appears in the code. (Two times.) Now count the number of times hero appears in the code. (Three times!) Whose method is this?

Your code is violating the principle of encapsulation. You are "operating on the hero" instead of "interacting with the hero." There is another maxim of OO programming: "Tell, don't ask!" Instead of asking for properties and asking for .block_stance and reducing the hero's hit points, just tell the hero you hit her, and tell her how much damage to take:

def battle_action(self, hero):
 damage = random.randrange(self.attack / 2, self.attack)
 hero.attacked_by(self, damage)

Then the Hero can handle updating the fields of the hero, and mitigating the damage with block_stance:

class Hero:
 def attacked_by(self, attacker, damage):
 """Apply damage taken from attacker."""
 if self.block_stance:
 damage //= 2 # // is "floor" or "integer" division
 print("Monster attacks you for {} damage.".format(damage))
 self.health -= damage

The important thing here is that the method which deals with the Hero fields and properties is a Hero method, not a Monster method. That means that if you decide to change how blocking works the changes will be in one place: on the Hero.

Likewise, when you do damage to a monster, the Monster class should handle applying the damage. That way, if you invent a monster subclass that is immune to swords (or whatever), you can implement that inside the subclass, rather than having to code it up in the various Hero subclasses.

This is not to say you cannot use the attacker's info. For example, if class StoneGolem(Monster) is immune to swords, then the .attacked_by method could check the attacker's weapon. (Or maybe there needs to be some kind of attack details object...) But the part where it is immune is definitely internal to the StoneGolem class. And the part where it updates health, and divides by 2 if blocking, and ...

Bottom line: don't touch another object's private data, let the other object do it.

EDIT

This is in response to your (first) comment.

Consider what you are trying to do in main: "Ask the user how they want to play, or if they want to quit. Then either quit, or set up the game the way the user requested, and play."

First, the presence/absence of a save-game file is something you can detect. So you should only ask the user if they want to load a saved game, if there actually is a save-game file available. Otherwise, the menu should just be "New Game or Quit".

This is something your code can check for:

if saved_game_exists():
 choices = (... "Load Saved Game" ...)
else:
 choices = (...)

In this circumstance, I'd suggest you write a TextUI.choose_from() function that takes kwargs (or you can splat a dict):

def main():
 while True:
 choices = {'new': "Play New Game", 'quit': "Quit"}
 if saved_game_exists():
 choices.update(load="Load Saved Game")
 choice = TextUI.choose_from(**choices)
 if choice == 'exit': 
 break
 elif choice == 'load':
 hero, game_map = load_saved_game()
 else:
 hero, game_map = create_new_game()
 play_game(hero, game_map)

This is a lot clearer, IMO. It shows what you are trying to do, and the conditions under which you will do it. It pushes every other consideration down into lower level functions. Of course, you have to actually write those functions. But, again, focus on doing just the right amount at the right level, and it should be straightforward.

• Axel1212: Thanks for the feedback. The active_character part was meant so that if the user chooses new game or load game, they can proceed with the map_navigation and so on. However, if the user chooses the "Exit to main menu" inmap_navigation(), we go back to the start of the loop in the main() function and reset active_character to false. I'm aware it's confusing a bit, an alternative option I just thought of would be to remove everything about active_character and replace the condition with if start_choice == 1 or start_choice == 2

Answer 2

Names:

1. dungeon_map should be named as dungeon_maps as it consists of multiple elements.
2. save_file should be named as save_file_path.
3. I understand that you chose name h_class for a hero class because class is a reserved word in Python. But a single letter prefix h_ is not a very good choice. And hero_ would be redundant. To avoid conflicts in these cases PEP8 advices to use trailing underscores. So in your case you will have a class_.

DRY (don't repeat yourself):

4. TextGameDetails.yaml is used twice in your code. You should create a variable for it called, for example, as game_details_path.

5. In battle_action method of a Monster you repeat the same actions but for a different amount of damage. Why not write something like:

   def battle_action(self, hero):
    damage = random.randrange(self.attack / 2, self.attack)
    if hero.block_stance:
    damage /= 2
    print(f"Monster attacks you for {damage} damage")
    hero.health -= damage
   

6. Asking user to choose something. See the comment on choose_from() from @Austin Hastings

Be consistent:

7. In some places you followed my advice and used f-strings, but in other ones you left the other way of formatting with .format(). Choose one way.

8. In battle_action the minimum attack value is calculated as self.attack / 2 but in attack_enemy as int(self.attack / 2)

9. In main you create hero and map separately but when you load them, you return them as a tuple. See example with load_saved_game and create_new_game from @Austin

10. Choose one way to keep multiline text in your yaml file.

Principle of least astonishment:

11. Your h_class and race are integers when they clearly should be strings.

Avoid magic numbers:

12. You have a lot of them. For example, in create_hero you keep health and attack points for every subclass of Hero when they should be kept as default parameters of __init__:

    class Warrior(Hero):
     def __init__(self,
     *,
     hero_name: str,
     race_choice: int,
     class_choice: int,
     hero_health: int = 200,
     hero_attack: int = 100):
     # The rest of the code
    

    After you do this you will have something like:

    if class_choice == 1:
     hero = Warrior(hero_name=hero_name,
     race_choice=race_choice,
     class_choice=class_choice)
     break
    elif class_choice == 2:
     hero = Mage(hero_name=hero_name,
     race_choice=race_choice,
     class_choice=class_choice)
     break
    elif class_choice == 3:
     hero = Healer(hero_name=hero_name,
     race_choice=race_choice,
     class_choice=class_choice)
     break
    

    Which can be reduced to:

    if class_choice == 1:
     class_ = Warrior
    elif class_choice == 2:
     class_ = Mage
    elif class_choice == 3:
     class_ = Healer
    return class_(hero_name=hero_name,
     race_choice=race_choice,
     class_choice=class_choice)
    

    And if you will return string instead of integers for class_choices than you could keep a dict with classes which you could use here like this:

    class_choices = {'Warrior': Warrior,
     'Mage': Mage,
     'Healer': Healer}
    class_ = class_choices[class_choice]
    return class_(hero_name=hero_name,
     race_choice=race_choice,
     class_choice=class_choice)