3
\$\begingroup\$

This is my first large project, a text based fighting simulator.

It is very clunky, do you have any ideas on how to make the code better? and how to make the code not get printed at once


from __future__ import print_function
#Zack Ennen 1/12/19
import random

import time


overheadmod = 3
slashmod = 0
jabmod = 1

def load():
    print("Loading: *000000000 10%")
    time.sleep(.2)
    print("Loading: **00000000 20%")
    time.sleep(.2)
    print("Loading: ***0000000 30%")
    time.sleep(.2)
    print("Loading: ****000000 40%")
    time.sleep(.2)
    print("Loading: *****00000 50%")
    time.sleep(.2)
    print("Loading: ******0000 60%")
    time.sleep(.2)
    print("Loading: *******000 70%")
    time.sleep(.2)
    print("Loading: ********00 80%")
    time.sleep(.2)
    print("Loading: *********0 90%")
    time.sleep(.2)
    print("Loading: ********** 100%")
    print("Loading Complete, Enjoy The Game")
    start()

def start():
    global hpenemy
    global name
    name = raw_input("Please Select Your Fighters Name " )
    global weapon
    weapon = raw_input("Please Select A Weapon, Axe, Sword, Or Dagger ")
    weapon = weapon.capitalize()
    global health
    global wepmod
    if weapon == "Axe":
        health = 30
        wepmod = -1
        genen()
    elif weapon == "Sword":
        health = 25
        wepmod = 0
        genen()
    elif weapon == "Dagger":
        health = 20
        wepmod = 1
        genen()
    else:
        start()
def genen():
    en = ["Mr,Stroop","Ork","Dragon","Goblin","Halfling","Wizard","Demon","DemiGod"]
    enwep = ["Club","Belt","Mace","Axe","Sword","Dagger"]
    secure_random = random.SystemRandom()
    global enemyweapon
    enemyweapon = secure_random.choice(enwep)
    global enemy
    enemy = secure_random.choice(en)
    global hpenemy
    hpenemy = random.randint(20,30)

    print(enemy + " Blocks Your Path Weilding A " + enemyweapon)
    attackphase1()

def attackphase1():
    print("*****Attack Phase*****")
    time.sleep(1)
    attack = raw_input("Would You Like To Attack " + enemy + " With Your " + weapon+ " Or Run (Attack Or Run) ")
    attack = attack.capitalize()
    if attack == "Attack":
        attack2()
    else:
        attackphase1()


def attack2():
    global power
    global hpenemy

    crit = random.randint(1,10)

    choice = raw_input("How Would You Like To Swing Your " + weapon + " Overhead, Slash, Or Jab ")
    choice = choice.capitalize()

    if choice == "Overhead":
        x = 1
    elif choice == "Jab":
        x=3
    elif choice == "Slash":
        x = 1
    else:
        attack2()

    print(name + " Readys Their " + weapon + " For A " + choice + " Attack")
    time.sleep(1)
    power = random.randint(1,3)
    power = power + wepmod
    if choice == "Overhead":
        power = power + overheadmod
    elif choice == "Jab":
        power = power + jabmod
    else:
        power = power + slashmod


    if crit == 10:
        power = power * 2
        print ("Critical Hit!")
    else:
        power = power + 0
    time.sleep(1)
    print(name + " Swings " + weapon + " At " + enemy + " " + choice + " Dealing ", end="")
    print(power, end="")
    print(" Damage")

    time.sleep(1)
    hpenemy = hpenemy - power

    time.sleep(1)
    print(enemy + " Takes ", end="")
    print(power, end="")
    print(" Damage From " + name + "'s " + choice)
    time.sleep(1)
    print(enemy + " Has ", end="")
    print(hpenemy, end="")
    print(" Health Remaining")
    global hpenemy
    if hpenemy <= 0:
        death()
    enemyattack()

def enemyattack():
    global health
    time.sleep(1)
    enpower = random.randint(1,7)
    print(enemy + " Swings At " + name + " With Their " + enemyweapon + " For ", end="")
    print(enpower)

    health = health - enpower
    time.sleep(1)
    print("You Have ", end="")
    print(health, end="")
    print(" Health Remaining")
    dead()

def dead():
    global health
    global hpenemy

    if hpenemy <= 0:
        print("*****You Win*****")
        restart = raw_input("Do You Want To Play Again? Yes Or No ")
        restart = restart.capitlize()
        if restart == "Yes":
            load()
        else:
            exit
    elif health <= 0:
        print("*****You Lose*****")
        restart = input("Do You Want To Play Again? Yes Or No ")
        restart = restart.capitlize()
        if restart == "Yes":
            load()
        else:
            exit
    else:
        x=2
        attack2()


load()

\$\endgroup\$
  • 2
    \$\begingroup\$ Welcome to Code Review! Please have a look at How do I ask a good question? and change your title in order to "[s]tate what your code does in your title, not your main concerns about it. Be descriptive and interesting, and you'll attract more views to your question." \$\endgroup\$ – AlexV Jul 15 at 11:53
  • \$\begingroup\$ This question is tagged "python-3.x", but the line from __future__ import print_function implies that this is actually Python 2 code. Can you confirm which version of Python you are using? \$\endgroup\$ – MJ713 Jul 15 at 16:06
  • \$\begingroup\$ in attack2() you call death() instead of dead(). Please fix this typo. \$\endgroup\$ – MJ713 Jul 15 at 16:23
  • \$\begingroup\$ Another typo: in dead(), the two .capitlize()s shoud be .capitalize()s \$\endgroup\$ – MJ713 Jul 15 at 16:47
2
\$\begingroup\$

I think the most important things to which you should pay attention are:

  • Avoid code duplication. Every time you find yourself writing the same or similar code even twice, think about using loops (hint: your load method) or extracting the code into a function (hint: dead method). The two most evident out of many existing reasons to avoid code duplication: 1) the code becomes shorter 2) if you need to change something in the logic that is repeated, you need to do it in only one place.

  • Do not use global variables. You should use them in very very rare cases. Instead, make functions exchange values via parameters. You may want to google why using global variables is so bad.

  • Dicts instead of multilevel elif statements. If you have something like:

    if x==1:
        y = 'a'
        z = 67
    elif x==2:
        y = 'b'
        z = 77
    elif x == 3:
        y = 'c'
        z = 87
    ...
    

    probably you would want to you a dictionary instead:

    dict_name = {1: ('a', 67), 2: ('b', 77), 3: ('c',87)}
    y,z = dict_name[x]
    
  • Regarding printing. First, format the string you want to print, then print it (do not call print 3 times to display one sentence). Also, avoid adding strings (with +)

    If you use python of version 3.6 and higher, you can you so-called f-strings instead:

    print(f'You have {health} remaining')
    

    Otherwise:

    print('You have {0} remaining'.format(health))
    
\$\endgroup\$
1
\$\begingroup\$

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:

  1. Lists are ordered. The third item in the list will always be the third item, unless the list is altered.
  2. 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:

  1. 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.
  2. 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()
\$\endgroup\$
  • \$\begingroup\$ @zack-ennen Sorry this got so long, I'm not good at summarizing. \$\endgroup\$ – MJ713 Jul 17 at 0:17

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.