Structure and flow
Add levels of abstraction
Your overall program structure is hard to read. You have a bunch of different functions calling each other in a loop/chain, which means you have to look at all of the code just to understand the basic sequence of events.
One way to fix this problem is to add a main() function that defines the core sequence of your program (load, define player, define enemy, choose attack or run, exchange attacks, declare winner, restart-or-exit), and uses your other, more-specific functions as "helpers" to get the job done. Then the overall structure of the program is easy to understand, and you know which function to read to get further details on any particular step.
def main():
restart = True
while restart:
load()
generate_player()
generate_enemy()
enter_attack_phase()
# exchange attacks
global player_hp
global enemy_hp
while True:
player_attack()
if enemy_hp <= 0:
break
enemy_attack()
if player_hp <= 0:
break
declare_winner()
restart = ask_for_restart()
if __name__ == "__main__":
main()
In fact, I would separate that "exchange attacks" code into its own function.
def exchange_attacks():
global player_hp
global enemy_hp
while True:
player_attack()
if enemy_hp <= 0:
break
enemy_attack()
if player_hp <= 0:
break
def main():
restart = True
while restart:
load()
generate_player()
generate_enemy()
enter_attack_phase()
exchange_attacks()
declare_winner()
restart = ask_for_restart()
if __name__ == "__main__":
main()
You can think of this kind of code as having levels, where each level further down handles more and more fine detail, while each level further up binds those more-detailed functions together into a sensible structure. Each lower-level function acts as an abstraction: a generic idea that the higher levels can use without knowing all the messy details (like "Attack each other until somebody dies").
Levels of abstraction not only make your code easier to read and understand, but they also make it easier to change. For example, in the code above, if I decided to sometimes randomly give the enemy two attacks in a row instead of one, I could just edit exchange_attacks() to make that happen. I wouldn't have to make any changes to main() (higher level) or to enemy_attack() (lower level). The change is "contained" within its own level.
Tip: If you find yourself writing near-identical code in multiple places, you probably need to add a level of abstraction. Write a function to handle the lower-level messy details and swap that in for the duplicate code.
def capitalized_input(prompt):
user_input = raw_input(prompt)
return user_input.capitalize()
Remove unnecessary recursive functions
You have several functions (start(), attackphase1(), attack2()) that call themselves, as a way of "restarting" if the user input is not valid. This is inefficient (since you have to re-do some work that is not related to accepting the user input) and hard to read.
More generally, functions that call themselves (called recursive functions) can have some very weird and cool and hard-to-understand behaviors, so you shouldn't use them unless you really need them.
Instead of recursive functions, use a while loop to retry until the user input is acceptable.
def attackphase1():
print("*****Attack Phase*****")
time.sleep(1)
attack = ""
while attack != "Attack":
attack = capitalized_input("Would You Like To Attack " + enemy + " With Your " + weapon+ " Or Run (Attack Or Run) ")
attack2()
Data and data structures
Remove unused variables
This is pretty basic. There are a few places in your code where you set a variable named x, but you never use it for anything. So we can remove it.
Use the right data structure for the job
For example, you have the enemy names and enemy weapon names stored in lists. Lists in python have two key properties:
- Lists are ordered. The third item in the list will always be the third item, unless the list is altered.
- Lists are mutable; they can be changed. You can add an item, remove an item, sort the entire list, and so on.
But in this case, you don't care about ordering (because you are just picking one item randomly), and you don't care about mutability either (because you never update the list after it is created). You never use the features that set lists apart from other data structures. So we have to ask, is a list is the best choice?
Looking at the other basic data structures:
- We don't want a dict because we're not dealing with key-value pairs.
- A set has some interesting properties: it's unordered, and comes with a uniqueness guarantee to make sure you can't add the same item twice. It even has an immutable version called frozenset. Unfortunately,
random.choice() doesn't work with sets.
- Tuples are ordered like lists, but immutable.
In this case the tuple is the basic data structure that best fits your needs. For more on Python's basic data structures, see the documentation.
enemies = ("Mr,Stroop","Ork","Dragon","Goblin","Halfling","Wizard","Demon","DemiGod")
enemy_weapons = ("Club","Belt","Mace","Axe","Sword","Dagger")
Use data structures to store related values together
For example, the user's choice of weapon determines both their health and a modifier to their attack, but these values are connected to each other through a series of if-statements, which is awkward. Storing the values together in some kind of data structure, like a dictionary of tuples, makes the logic more compact (and therefore easier to read) and provides a handy central place to add new values or edit existing values in the future.
# structure of dictionary is { weapon name: (player HP, weapon mod) }
weapon_choice_to_player_stats = {
"Axe": (30, -1),
"Sword": (25, 0),
"Dagger": (20, 1)
}
global weapon
while weapon not in weapon_choice_to_player_stats:
raw_weapon = raw_input("Please Select A Weapon, Axe, Sword, Or Dagger ")
weapon = raw_weapon.capitalize()
global health
global wepmod
health, wepmod = weapon_choice_to_player_stats[weapon]
Granted, it's a bit annoying to have to remember which number in the tuple is player health and which one is the weapons mod. What if you get them backwards? Well, you could create a more advanced nested structure of dictionaries-within-dictionaries...
weapon_choice_to_player_stats = {
"Axe": { "health": 30, "wepmod": -1 },
"Sword": { "health": 25, "wepmod": 0 },
"Dagger": { "health": 20, "wepmod": 1 }
}
stats = weapon_choice_to_player_stats[weapon]
global health
global wepmod
health = stats["health"]
wepmod = stats["wepmod"]
... or you could go so far as to create a PlayerType class and add the stats as properties of PlayerType objects. But that would open us up to a much deeper restructuring of other parts of the code, so let's set that possibility aside for now...
Another example: overheadmod, slashmod, and jabmod are all used in the same specific place in one function, but are currently floating out there unattached to each other. It would make sense to group them together into a dictionary.
I'll let you think of other instances.
Be careful with globals
You use the global keyword a lot. Global variables in general are considered "risky" in programming. Since they can be accessed from anywhere, it's harder to keep track of when and where they were modified or not modified, which makes it harder to write correct logic, which leads to bugs. You can also run into problems with scope, where you think you're using the global variable, but you're actually creating or calling on a local variable with the same name. Or vice versa. More bugs.
So using all these globals is not ideal, but I think you can get away with it in this specific case for two reasons:
global variables in Python aren't truly "global" in the traditional sense; they are restricted to a specific module (i.e. they are not visible outside this file). So the amount of stuff that can go wrong is limited by that.- This program is small and simple enough that you can hold the entire structure of the program in your head at once, so you're unlikely to lose track of which values are modified where. But if the program became much more complicated then
globals would definitely be a bad idea.
A related problem: instead of declaring all these module-wide / global variables in one place, you scatter the declarations throughout the code. This is hard to read and understand. A reader has to go on a scavenger hunt to understand what values are shared by different functions. It also triggers a warning message when you run the code (SyntaxWarning: name 'hpenemy' is assigned to before global declaration).
So, at minimum, you should declare all the module-wide / global variables at the top of the module. That way you can see all the shared values in one place. Since it's unclear what values the variables should have at that point, give them "empty" values: 0, "" (the empty string), None, etc.
enemy = ""
enemyweapon = ""
health = 0
hpenemy = 0
name = ""
power = 0
weapon = 0
wepmod = 0
def load():
...
Looking at the code, I think power doesn't even need to be a global, and I would change some of the variable names to make them clearer and make them better match each other. E.g. player_hp and enemy_hp instead of health and hpenemy.
But wait! If we remember our lesson from "Use data structures to store related values together", we can bundle the player-related values and the enemy-related values together, reducing the number of global variables to 2.
player = None
enemy = None
...
def generate_enemy():
...
global enemy
enemy = {
"name": name,
"hp": hp,
"weapon_name": weapon_name
}
print("{} Blocks Your Path Wielding A {}".format(enemy["name"], enemy.["weapon_name"]))
Better yet: instead of using dicts for our player and enemy global variables, we can create some lightweight classes that serve the same role. The advantage of a class is reusability: if for some reason you had two different functions that could both create enemies, you could just call the Enemy class constructor twice, instead of having to type out the whole enemy dict structure in two different places in the code.
player = None
enemy = None
class Player(object):
def __init__(self, name, hp, weapon_name, weapon_attack_mod):
self.name = name
self.hp = hp
self.weapon_name = weapon_name
self.weapon_attack_mod = weapon_attack_mod
class Enemy(object):
def __init__(self, name, hp, weapon_name):
self.name = name
self.hp = hp
self.weapon_name = weapon_name
...
def generate_enemy():
...
global enemy
enemy = Enemy(name, hp, weapon_name)
print("{} Blocks Your Path Wielding A {}".format(enemy.name, enemy.weapon_name))
Of course, now that we only have two variables to deal with instead of 8, it becomes a lot easier to just avoid using globals altogether, and just pass them back and forth between functions as parameters:
def exchange_attacks(player, enemy):
while True:
player_attack(player, enemy)
if enemy.hp <= 0:
break
enemy_attack(player, enemy)
if player.hp <= 0:
break
def main():
restart = True
while restart:
load()
player = generate_player()
enemy = generate_enemy()
enter_attack_phase(player, enemy)
exchange_attacks(player, enemy)
declare_winner(player, enemy)
restart = ask_for_restart()
if __name__ == "__main__":
main()
So in the end we didn't need globals after all.
Miscellaneous
Code Style
The developers of Python have a number of recommendations for how to make your code look nice: where to use line breaks, how long your lines should be, whether or not to put spaces before or after certain symbols, etc. You have a few minor violations here (no spaces after commas, etc.). It's not a huge deal, but abiding by the guidelines can help make your code more readable. See the PEP 8 document for further details. A text editor plugin like Pylint can help you keep track of this as you write code (and detect actual code errors too, which is nice).
Putting it all together
Here's my revision of your code using the principles I've discussed. There might still be some room for improvement; for instance, I didn't look to see if there's a cleaner way of separating the "UI logic" (the code that prints to the screen and gets use inputs back) from the "business logic" (the code that actually computes the attacks and hit points). Right now everything is kind of mixed together. Then again, it might not be worth your time to separate them unless the program gets bigger and more complicated.
from __future__ import print_function
import random
from time import sleep
class Player(object):
def __init__(self, name, hp, weapon, weapon_attack_mod):
self.name = name
self.hp = hp
self.weapon = weapon
self.weapon_attack_mod = weapon_attack_mod
class Enemy(object):
def __init__(self, name, hp, weapon):
self.name = name
self.hp = hp
self.weapon = weapon
def capitalized_input(prompt):
user_input = raw_input(prompt)
return user_input.capitalize()
def print_and_sleep(msg, time_to_sleep=1):
print(msg)
sleep(time_to_sleep)
def build_loading_message(percent_loaded, bar_length,
loaded_char='*', unloaded_char='0'):
num_loaded_chars = percent_loaded // bar_length
num_unloaded_chars = bar_length - num_loaded_chars
return "Loading: {}{} {}%".format(loaded_char * num_loaded_chars,
unloaded_char * num_unloaded_chars,
percent_loaded)
def load():
bar_length = 10
for percent in range(10, 100, 10):
msg = build_loading_message(percent, bar_length)
print_and_sleep(msg, .2)
print(build_loading_message(100, bar_length))
print("Loading Complete, Enjoy The Game")
def generate_player():
weapon_choice_to_player_stats = {
"Axe": {"base_hp": 30, "weapon_attack_mod": -1},
"Sword": {"base_hp": 25, "weapon_attack_mod": 0},
"Dagger": {"base_hp": 20, "weapon_attack_mod": 1}
}
name = raw_input("Please Select Your Fighter's Name: ")
weapon = ""
while weapon not in weapon_choice_to_player_stats:
weapon = capitalized_input(
"Please Select A Weapon (Axe, Sword, Or Dagger): ")
chosen_stats = weapon_choice_to_player_stats[weapon]
hp = chosen_stats["base_hp"]
weapon_attack_mod = chosen_stats["weapon_attack_mod"]
player = Player(name, hp, weapon, weapon_attack_mod)
return player
def generate_enemy():
enemy_names = ("Mr. Stroop", "Ork", "Dragon", "Goblin",
"Halfling", "Wizard", "Demon", "DemiGod")
enemy_weapons = ("Club", "Belt", "Mace", "Axe", "Sword", "Dagger")
secure_random = random.SystemRandom()
name = secure_random.choice(enemy_names)
hp = secure_random.randint(20, 30)
weapon = secure_random.choice(enemy_weapons)
enemy = Enemy(name, hp, weapon)
print("{} Blocks Your Path Wielding A {}".format(enemy.name, enemy.weapon))
return enemy
def enter_attack_phase(player, enemy):
print_and_sleep("*****Attack Phase*****")
action = ""
while action != "Attack":
action = capitalized_input(
"Would You Like To Attack {} With Your "
"{} Or Run (Attack Or Run) ".format(enemy.name, player.weapon))
def player_attack(player, enemy):
attack_type_modifiers = {
"Overhead": 3,
"Slash": 0,
"Jab": 1
}
attack_type = ""
while attack_type not in attack_type_modifiers:
attack_type = capitalized_input(
"How Would You Like To Swing Your "
"{} Overhead, Slash, Or Jab ".format(player.weapon))
print_and_sleep("{} Readies Their {} For A {} Attack".format(
player.name, player.weapon, attack_type))
base_power = random.randint(1, 3)
power = (base_power
+ player.weapon_attack_mod
+ attack_type_modifiers[attack_type])
crit = random.randint(1, 10)
if crit == 10:
power = power * 2
print_and_sleep("Critical Hit!")
print_and_sleep("{} Swings {} At {} {} Dealing {} Damage".format(
player.name, player.weapon, enemy.name, attack_type, power))
enemy.hp = enemy.hp - power
print_and_sleep("{} Takes {} Damage From {}'s {}".format(
enemy.name, power, player.name, attack_type))
print_and_sleep("{} Has {} Health Remaining".format(enemy.name, enemy.hp))
def enemy_attack(player, enemy):
power = random.randint(1, 7)
print_and_sleep("{} Swings At {} With Their {} For {}".format(
enemy.name, player.name, enemy.weapon, power))
player.hp = player.hp - power
print_and_sleep("You Have {} Health Remaining".format(player.hp))
def decide_winner(player, enemy):
if enemy.hp <= 0:
print("*****You Win*****")
elif player.hp <= 0:
print("*****You Lose*****")
else:
raise ValueError("Cannot decide winner while "
"both sides have health remaining")
def ask_for_restart():
acceptable_responses = ("Yes", "No")
response = ""
while response not in acceptable_responses:
response = capitalized_input("Do You Want To Play Again? Yes Or No ")
return response == "Yes"
def exchange_attacks(player, enemy):
while True:
player_attack(player, enemy)
if enemy.hp <= 0:
break
enemy_attack(player, enemy)
if player.hp <= 0:
break
def main():
restart = True
while restart:
load()
player = generate_player()
enemy = generate_enemy()
enter_attack_phase(player, enemy)
exchange_attacks(player, enemy)
decide_winner(player, enemy)
restart = ask_for_restart()
if __name__ == "__main__":
main()
from __future__ import print_functionimplies that this is actually Python 2 code. Can you confirm which version of Python you are using? \$\endgroup\$attack2()you calldeath()instead ofdead(). Please fix this typo. \$\endgroup\$dead(), the two.capitlize()s shoud be.capitalize()s \$\endgroup\$