13
\$\begingroup\$

I am trying to teach myself to code using Python. The following is the first real program I have written from scratch. I feel that it is messy and in need of improvement, but I am either unsure of what to improve next or don't know how to improve it. I am looking for suggestions about: turning my functions into class methods, implementing the inventory{} list, improving my ranmob function to work with a much larger (list?) of monsters, and possibly turning my while True loop into a gameLoop function. Any other general advice would be appreciated.

from random import randint
class Dice:    
    def die(num):
        die=randint(1,num)
        return die
class Character:
    def __init__(self,name,hp,thaco,ac,inventory,exp):
        self.name=name
        self.hp=hp
        self.thaco=thaco
        self.ac=ac
        self.inventory=inventory
        self.exp=exp



class Fighter(Character):
    def __init__(self):
        super().__init__(name=input("What is your characters name?"),thaco=20,ac=10,
                         hp=10,inventory={},exp=10)
    prof = "fighter"
    maxhp=10
    level=1
    hd=10
    level2=20

class Cleric(Character):
    def __init__(self):
        super().__init__(name=input("What is your characters name?"),thaco=20,ac=10,
                         hp=8,inventory={},exp=8)
    prof= "cleric"
    maxhp=8
    level=1
    hd=8
    level2=15
class Mage(Character):
    def __init__(self):
        super().__init__(name=input("What is your characters name?"),thaco=20,ac=10,
                         hp=4,inventory={},exp=4)
    prof= "mage"
    mana=1
    maxmana=1
    maxhp=4
    level=1
    hd=4
    level2=10
class Goblin(Character):
    def __init__(self):
        super().__init__(name="goblin",
                         hp=7,thaco=20,
                         ac=6,inventory={},
                         exp=7)


class Orc(Character):
    def __init__(self):
        super().__init__(name="orc",
                         hp=8,thaco=18,
                         ac=6,inventory={},
                         exp=8)

def profession():
    print("What is your class?",'\n',
          " press f for Fighter",'\n',
          " press c for Cleric",'\n',
          " press m for Mage")
    pclass=input(">>>")
    if pclass =="f":
        Prof = Fighter()
    elif pclass=="c":
        Prof = Cleric()
    elif pclass == "m":
        Prof = Mage()
    else:
        Prof=Fighter()
        #profession()
    return Prof
def ranmob():
    mob = Goblin() if Dice.die(2)<2 else Orc()
    return mob


def playerAttack():
    roll=Dice.die(20)   
    if roll>=hero.thaco-mob.ac:
        print("You hit")
        if hero.prof=="fighter":
            rollD=Dice.die(10)

        if hero.prof=="cleric":
            rollD=Dice.die(6)

        if hero.prof=="mage":
            rollD=Dice.die(4)
        print("for",rollD,"damage")
        mob.hp-=rollD
        print("the",mob.name,"has",mob.hp,"hp left")
    else:
        print("You miss")

def monsterAttack():
    roll=Dice.die(20)   
    if roll>=mob.thaco-hero.ac:
        print("Monster hit")
        if mob.name=="goblin":                    
            rollD=Dice.die(4)
        elif mob.name=="orc":
            rollD=Dice.die(6)
        print("for",rollD,"damage")
        hero.hp-=rollD
        print(hero.name,"has",hero.hp,"hp left")
    else:
        print("Monster misses")

def levelUp():


    while hero.exp>=hero.level2:
        levelGain=False
        hero.level+=1
        levelGain=True
        hero.level2=hero.level2*2
        if levelGain==True:
            hero.maxhp+=Dice.die(hero.hd)
            hero.hp=hero.maxhp
            if hero.prof=="mage":
                hero.maxmana+=1
                hero.mana=hero.maxmana

            print("You Gained a level","\n",'hp:',hero.hp,"\n",'level:',hero.level)
            levelGain=False
    while hero.level>=3:
        hero.level-=3
        hero.thaco-=1
        print("thaco:",hero.thaco)


def commands():
    if hero.prof=="fighter":
        print (" press f to fight",'\n',
               "press enter to pass")
        command=input("~~~~~~~~~Press a key to Continue.~~~~~~~")
        if command=="f":            
            playerAttack()
        if command=="":
            pass

    if hero.prof=="cleric":
        print (" press f to fight",'\n',
               "press h to heal",'\n',
               "press enter to pass")
        command=input("~~~~~~~~~Press a key to Continue.~~~~~~~")
        if command=="f":            
            playerAttack()
        elif command =="h":
            if hero.hp<hero.maxhp:
                hero.hp+=Dice.die(8)                
                if hero.hp>hero.maxhp:
                    hero.hp=hero.hp-(hero.hp-hero.maxhp)                    
                print("You now have:",hero.hp,"hp")
            else:
                print("Your hit points are full")
                commands()
        elif command=="":
            pass
    if hero.prof=="mage":
        print (" press f to fight",'\n',
               "press s for spells",'\n',
               "press m to generate mana",'\n',
               "press enter to pass")
        command=input("~~~~~~~~~Press a key to Continue.~~~~~~~")
        if command=="f":            
            playerAttack()
        elif command =="s":
            print("You have",hero.mana,"mana")
            if hero.mana>=1 and hero.mana<3:
                print("press s for sleep",'\n',
                      "press m for magic missile")
                command=input(">>>")
                if command =="s":
                    print("You put the monster to sleep it is easy to kill now")
                    mob.hp-=mob.hp
                    hero.mana-=1
                if command=="m":
                    if hero.mana<hero.maxmana:
                        hero.mana+=Dice.die(4)                
                        if hero.mana>hero.maxmana:
                            hero.mana-=(hero.mana-hero.maxmana)
                    dam =Dice.die(4)*hero.mana
                    mob.hp-=dam
                    print("You use all your mana! and do",dam,"damage!")
                    hero.mana-=hero.mana
            elif hero.mana>=3:
                print("press s for sleep",'\n',
                      "press m for magic missile",'\n',
                      "press f for fireball")
                command=input(">>>")
                if command =="s":
                    print("You put the monster to sleep it is easy to kill now")
                    mob.hp-=mob.hp
                    hero.mana-=1
                if command=="m":
                    dam=Dice.die(4)*hero.mana
                    mob.hp-=dam
                    print("You use all your mana! and do",dam,"damage!")
                    hero.mana-=hero.mana
                if command=="f":
                    print("You are temporarily blinded by a feiry flash of light.")
                    dam=0
                    dam+=Dice.die(6)
                    dam+=Dice.die(6)
                    dam+=Dice.die(6)
                    mob.hp-=dam
                    print("You did",dam,"points of damage")

                    hero.mana-=3
            else:
                print("Your mana is empty")
                commands()
        elif command =="m":
            if hero.mana<hero.maxmana:
                hero.mana+=1
                print("You have",hero.mana,"mana")
            elif hero.mana>=hero.maxmana:
                print("Your mana is full.")
                print("You have",hero.mana,"mana")
                commands()

        elif command=="":
            pass

mob=ranmob()
hero=profession()
print("name hp thaco ac inventory xp",'\n',
      hero.name,hero.hp,hero.thaco,hero.ac,hero.inventory,hero.exp)
while True:

    if mob.hp<=0:
        print('The',mob.name,'is dead!')        
        hero.exp+=mob.exp        
        print('hero xp',hero.exp)
        mob=ranmob()
    if hero.hp<=0:
        mob.exp+=hero.exp
        print("mob xp:",mob.exp)
        print(hero.name,'died!')
        #name=input("What is your characters name?")                
        hero=profession()
        print("name hp thaco ac inventory xp",'\n',
              hero.name,hero.hp,hero.thaco,hero.ac,hero.inventory,hero.exp)

    levelUp()

    print("You see",mob.name+",",mob.name,"has",mob.hp,"hp.")
    if hero.hp>0:
        commands()
    if mob.hp>0:                
        monsterAttack()
\$\endgroup\$

4 Answers 4

16
\$\begingroup\$
class Dice:    
    def die(num):
        die=randint(1,num)
        return die

A few points:

  1. I would call the method roll, and the class Die;
  2. It's typical to model a die by setting the number of sides in __init__, then calling roll without any arguments;
  3. Why bother assigning die?

I would have written:

class Die:
    """Represents a single die."""

    def __init__(self, sides=6):
        """Set the number of sides (defaults to six)."""
        self.sides = sides

    def roll(self):
        """Roll the die."""
        return random.randint(1, self.sides)

Note the use of docstrings to provide information about the class and its methods. Now e.g. Dice.die(2) becomes Die(2).roll(), which I think is much clearer about what's happening, and you can make a single die:

four_sided_die = Die(4)

and roll it repeatedly:

four_sided_die.roll()

I would be inclined to make the player classes separate to the monster classes, so you'd have an inheritance structure like:

                Character
               /         \
         Player           Monster
        /   |  \           /    \
 Fighter  Mage  Cleric  Goblin   Orc

This lets you factor out more of the duplication. For example:

class Player(Character):

    def __init__(self, hp, exp):
        super().__init__(input("What is your character's name? "),
                         20, 10, hp, {}, exp)


class Fighter(Character):

    # Note that constants should be UPPERCASE_WITH_UNDERSCORES
    HD = 10
    LEVEL = 1  # should this really be a class attribute?
    LEVEL_2 = 20
    MAX_HP = 10
    PROF = "fighter"

    def __init__(self):
        super().__init__(10, 10)

def ranmob():
    mob = Goblin() if Dice.die(2)<2 else Orc()
    return mob

A few points:

  1. random_mob would be a better name;
  2. This returns a single enemy, which I'm not sure I'd call a "mob";
  3. There's no need to use the Dice.

I'd use random.choice for this, and allow a size of mob to be specified:

ENEMIES = (Goblin, Orc)

def random_mob(size):
    return [random.choice(ENEMIES)() for _ in range(size)]

this uses a list comprehension to create a list of randomly-selected enemies from the tuple.


There is a vast amount of duplication in commands, and it contains logic (what the Players can do) that should be stored with the Player. For example, if each Player subclass had a dictionary of valid commands (each implemented as an instance method):

class Fighter(Player):

    ...

    def fight(self):
        super().fight()  # all players can fight

    def cast_spell(self):
        ...

    def generate_mana(self):
        ...

    COMMANDS = {
        'f': ('fight', fight),
        's': ('spells', cast_spell),
        'm': ('generate mana', generate_mana),
    }

Then commands starts:

# Show the valid commands
for command, action in hero.COMMANDS.items():
    print('press {} to {}'.format(command, action[0]))
print('press Enter to skip')

# Get validated user input
while True:
    command = input("~~~~~~~~~Press a key to Continue.~~~~~~~")
    if command and command not in hero.COMMANDS:
        print('Not a valid command')
        continue
    break

# Run the appropriate action
if command:
    hero.COMMANDS[command][1]()  # call the method 

See Asking the user for input until they give a valid response for more on input validation. You can extend this to define the appropriate parameters for each action, etc..

\$\endgroup\$
18
  • \$\begingroup\$ First thanks jon the changes to my dice class were really helpful. The ranmob function needs to eventually have D100 to parse through a list of mobs with different chances. e.g. 1-45=orc 46-50=goblin etc so your suggestion for that will not work. Maybe a dictionary but I am not exactly sure how to do that. I would like to implement a player and monster class to get rid of some of the repetitive code, but I am having trouble understanding how the inheritance works. I tried the code in your example it threw an error at ",20, 10, hp, {}, exp)" could you please elaborate with a more clear example? \$\endgroup\$
    – Medullan
    May 18, 2015 at 20:17
  • \$\begingroup\$ I'd recommend using dict.iteritems() over dict.items(). \$\endgroup\$ May 18, 2015 at 20:39
  • 1
    \$\begingroup\$ @Medullan ah yes, my fault; you can't have name= then positional arguments. I've removed the first keyword, or you can continue to pass all by keyword. \$\endgroup\$
    – jonrsharpe
    May 18, 2015 at 20:56
  • 2
    \$\begingroup\$ As a side note, 'mob' is just a piece game jargon that means 'monster'; Minecraft is the first example that pops to mind of where it's used commonly. \$\endgroup\$
    – anon
    May 19, 2015 at 3:39
  • 4
    \$\begingroup\$ "mob" comes from "Mobile" -- en.wikipedia.org/wiki/Mob_(video_gaming) \$\endgroup\$ May 19, 2015 at 10:31
10
\$\begingroup\$

There's a lot of code here, (258 lines), so I'm not going to give a full review, but rather, a few suggestions on what you can improve in general.

  • My first suggestion is to be consistent, and clear with your whitespace usage. For example, on some lines, you have something like this: self.thaco=thaco, and on other lines you have something like this: prof= "cleric". Both of these should be changed to something like variable = value. This makes it clearer to read. You should also have whitespace between operators, for example: x >= y, and two blank lines in between function and class declarations.
  • Another suggestion would be to add comments and docstrings explaining what your code does. For example:

def my_function(arguments):
    """
    This is a docstring. Put an explanation of
    your function or class here, and put your code below.
    """
    pass

  • When naming classes, variables, and functions, it's important to give them good names. For example, I have no idea what the variable self.thaco is used for from just looking at it. Names should be descriptive of what they are used for, but not too long either. After all, nobody wants to have to type an name like this_is_the_x_position_of_the_player.
  • You have quite a few functions which are just large, hard-to-read blocks of code. See if you can separate certain actions out into smaller, more easy to read functions. This is for the benefit of you, and someone who might want to read your code.
  • I'd also create a generalized function for choosing player decisions, rather than large if/elif/else blocks. Here's what I'd implement.

def player_decision(player_input, possible_choices):
    """
    Return a value based on the player's input.
    """
    if player_input in possible_choices:
        return possible_choices[player_input]
    else:
        print "Invalid choice!"

  • player_input would simply be the string the player inputs, and possible_choices is a dictionary of possible choices and their return values.
  • Finally, I'd recommend reading Python's official style guide, PEP8.

Anyway's, I hope this helps you with your project!

\$\endgroup\$
1
  • 5
    \$\begingroup\$ In defense of naming a variable thaco, "THAC0" is a term that shows up frequently in the domain of tabletop RPGs. It's short for "to hit armor class zero" and is used to calculate whether an attack hits. See dungeons.wikia.com/wiki/THAC0 \$\endgroup\$
    – Kevin
    May 19, 2015 at 1:12
7
\$\begingroup\$

The Official style guide

You have about 250 violations to PEP8, use autopep8 to make your code more readable

Dictionary

A long series of if and elif is considered bad style, a dictionary also allows easier adding of more classes and options.

def profession():
    letter_to_profession = {
        'f': Fighter,
        'c': Cleric,
        'm': Mage
        }
    print("What is your class?\n")
    for letter in letter_to_profession.keys():
        print("- Press {} for {}".format(
            letter, letter_to_profession[letter].__name__))
    pclass = input(">>>")
    return letter_to_profession[pclass]()

Also note that the printing is generated from the dictionary for maximum convenience.

Stand-alone functions

In Python functions can float freely in the global namespace an it is raccomended that they do so when it makes sense, so:

class Dice:    
    def die(num):
        die=randint(1,num)
        return die

Must become:

def die(sides):
     return randint(1,num)

also it is better to return the value directly (without and intermediate variable).

Extend the classes

def playerAttack():
    roll=Dice.die(20)   
    if roll>=hero.thaco-mob.ac:
        print("You hit")
        if hero.prof=="fighter":
            rollD=Dice.die(10)

        if hero.prof=="cleric":
            rollD=Dice.die(6)

        if hero.prof=="mage":
            rollD=Dice.die(4)
        print("for",rollD,"damage")
        mob.hp-=rollD
        print("the",mob.name,"has",mob.hp,"hp left")
    else:
        print("You miss")

This if chain is not nice and will become even less nice when more characters are added, you should implement the max_damage or max_power inside the Player Class so as to write:

def playerAttack():
    roll = Dice.die(20)
    if roll >= hero.thaco - mob.ac:
        print("You hit")
        roolD = die(hero.max_power)
        print("for", rollD, "damage")
        mob.hp -= rollD
        print("the", mob.name, "has", mob.hp, "hp left")
    else:
        print("You miss")

Put effort into pretty printing.

The following:

print("name hp thaco ac inventory xp", '\n',
      hero.name, hero.hp, hero.thaco, hero.ac, hero.inventory, hero.exp)

when printed looks like:

name hp thaco ac inventory xp 
 foobar 10 20 10 {} 10

Instead:

print("Name: {}, HP: {}, Thaco: {}, Ac:{}, Inventory:{}, XP:{}\n".format(
     hero.name, hero.hp, hero.thaco, hero.ac, hero.inventory, hero.exp))

Looks like:

Name: foobar, HP: 4, Thaco: 20, Ac:10, Inventory:{}, XP:4
\$\endgroup\$
3
  • \$\begingroup\$ I'd separate the pretty-print values by comma, like so: Name: foobar, HP: 4, .... Just looks nicer. :) \$\endgroup\$ May 18, 2015 at 20:36
  • \$\begingroup\$ You've got a little bit of wacky indenting going on (around your professions dictionary), and one chunk of code just plain ain't formatted as code (under "Extend the classes"). You've also got a couple of typos sprinkled in -- 'shouls' for 'should', that kind of thing. \$\endgroup\$
    – anon
    May 19, 2015 at 3:42
  • \$\begingroup\$ @QPaysTaxes fixed code blocka and typo, the indentation is more readable this way in the dictinoary IMO \$\endgroup\$
    – Caridorc
    May 19, 2015 at 13:02
3
\$\begingroup\$

The Dice class is superfluous as written. If you just want to be able to invoke a die roll, you just need the function

def roll(s):
    return random.randint(1, s)

However, the Dice class makes sense if you were storing it to be used for something. For example, if you make the modification suggested in this other answer, then you could modify your classes such as

class Fighter(Character):
    # ...
    damageDice = Die(10)

and then playerAttack would simplify to

def playerAttack():
    roll=Die(20).roll()
    if roll>=hero.thaco-mob.ac:
        print("You hit")
        rollD=hero.damageDice.roll()
        print("for",rollD,"damage")
        mob.hp-=rollD
        print("the",mob.name,"has",mob.hp,"hp left")
    else:
        print("You miss")

This change also opens up the path to make damageDice something more interesting in the future, and to make it an attribute of instances of a character rather than being tied to its class -- e.g. your fighter might upgrade to a +1 greatsword, and you'd want to change damageDice to an object that would roll 2d6+1.

\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.