Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've made a python script to properly organise D&D monster stats in a directory. The script brings up the stats as instance variables of a Monster (class) instance.

For those unfamiliar with the game, keeping track of monsters (to me at least) is cumbersome and prone to many dead trees. You have to flip through the Monster Manual to find the monster, write down any change done to it (like health) on a piece of paper, keep track of the initiative order, different encounters...

There is a lot to do, so I decided to use technology to my advantage. I have folders set up under the Monsters folder. In each folder is a subfolder (the same types across all of them). In each subfolder is a .txt document with the stats (as dictated by the book) for each monster.

  • Main Types = Animate, Beast, Humanoid, Magical Beast
  • Sub Types = Abberant Elemental Fey Immortal Natural Shadow

Each text document is set up like the following example:

Example Monster # This is up here, and skipped, so I keep track of each one easily without having to remember to look at the name of the file.
HP = 50
Initiative = 5
Senses = [5, 'Darkvision'] # In auto, this is Perception and Vision combined
Defence = {"AC" : 5, "Fortitude" : 4, "Reflex" : 6, "Will" : 7}
Saving_Throw = 2
Speed = 6
AP = 1 # Action Points
Attacks = {'An attack that hurts the opponent and does permanent damage to its armour' : 'Example.alterstat({"HP" : -5, "Defence[0] : -3})' 

Here is my entire Monster class, plus some unbound functions. What I want to know is if any of the functions can be improved (especially the __init__, addmonster, and LoadAll)

 import os
 types = 'Animate Beast Humanoid Magical_Beast'.split()
 sub_types = 'Abberant Elemental Fey Immortal Natural Shadow'.split()
 LoadedMonsters = []


 class Monster:
     def __init__(self, monster, LoadedMonsters = LoadedMonsters):
         '''Checks if monsters is defined. If it is defined, sets class attributes to be the monster's as decided in Monsters.txt. If it is already loaded in LoadedMonsters, makes a copy'''
         self.name = monster.capitalize()
         self.isdead = False
         monstercheck = monsterfind(self.name)
         if monstercheck != False:
             with open(monstercheck, 'r') as monstercheck:
                 print monstercheck.next()
                 for stat in monstercheck:
                     print stat[:-1] # Show what stats are loaded
                     var, value = stat.split('=')
                     var, value = val.strip(), value.strip()
                     try:
                         value = eval(value) # Fixes non-string values
                     except:
                         pass
                     setattr(self, var, value)
                 self.Bloodied = self.HP / 2
             AllMonsters = open('Monsters.txt', 'w+') # List (.txt) of all monsters
             AllMonsters.read() # Get to end of text
             AllMonsters.write(self.name)
             AllMonsters.close()
             monstercheck.close()
             LoadedMonsters += [self.name] # A list of all loaded monsters
             print 'Monster loaded'
         else: # If can't find the monster in the folders
             print '{} Not found, add it?'.format(self.name)
             if raw_input('Y/n\n').capitalize() == 'Y':
                 self.addmonster()
                 self = Monster(self)
             else:
                 self.name = 'UNKNOWN'
    def setMonsterLevel(self, monster, level):
        "Changes the given monster's level to the given value."
        if monster not in LoadedMonsters:
            print 'Monster not loaded'
        raise NotImplementedError # I'll get to it, it's harder then it seems

    def attack(self, monsters, changes, buff = None):
        '''Takes a list of targets [Variable names!],a dictionary of stats and values w/ the stats as the key, and applies it to the list of targets. Optional parametre, buff, can be applied at the end as a dictionary'''
        for monster in monsters:
            for change in changes:
                monster.alterstat( {change : changes.get(change)} )
        if buff is not None:
            self.alterstat(buff.keys()[0], buff.get(buff.keys()[0]))# Messy way of saying get key and value of dictionary

def monsterfind(name): # For use in __init__
    '''name = monster Return folder path if monster exists, False otherwise'''
    monster = name + '.txt'
    for folder in types:
        for subfolder in sub_types:
            path = os.path.abspath('Monsters' + '\\{}\\{}\\{}'.format(
            folder, subfolder, monster))
            if os.path.exists(path):
                return path
    return False

def addmonster(name): # Bit wordy
    '''Name = string representation of a monster name makes monster name'''
    print 'Checking if available'
    if os.path.exists(name): # Can't have names across sub folders
        print '{} is taken!'.format(name)
        name = raw_input('Choose a different name: ')
        addmonster() # Start again
    print 'Name available!'
    while True:
        option = raw_input(
        'Choose the main folder:{}\n'.format(types)).capitalize()
        if option in types:
            while True:
                sub_option = raw_input(
                'Choose a sub-folder {}\n'.format(sub_types)).capitalize()
                if sub_option in sub_types:
                    newmonster = open(
                    os.path.abspath(
                    'Monsters') + '\\{}\\{}\\{}.txt'.format(
                    option, sub_option, name), 'w')
                    break
        break
    if raw_input('Automatic data entry, or Manual? \n').capitalize() == ('Manual'):
        print '''Enter your information. (Press enter for every line, press enter on a blank line to finish)'''
        newmonster.write(name + ':\n')
        while True:
            newline = raw_input('>') + '\n' # Starts on newline, gives last
            if newline == '\n':             # a new line
                break
            newmonster.write(newline)
    else:
        while True:
            Information = []
            print '''Follow the steps below. If you do not want to add a value then leave it blank. This will NOT be saved until you confirm it at the end'''
            print "Enter a number for the monster's health"
            HP = 'HP = ' + raw_input('HP = ')
            Information.append(HP)
            print "Enter a # bonus (or penalty) for the monster's Initiative"
            print "0 if no bonus/penalty"
            Initiative = 'Initiative = ' + raw_input('Initiative = ')
            Information.append(Initiative)
            print "Enter a # bonus (or penalty) for the monster's perception"
            Senses = list(raw_input('Perception = '))
            print "Enter the vision type of the monster"
            print "I.E: Darkvision, Normal, Low-Light, etc."
            Senses.append(raw_input('Vision = '))
            Senses = 'Sense =' + str(Senses)
            Information.append(Senses)
            print 'Enter the following defence stats as numbers:'
            Defence = 'Defence = ' + str({
            "AC" : input('AC = '),
            "Fortitude": input('Fortitude = '),
            "Reflex" : input('Reflex = '),
            "Will" : input('Will = ')
            })
            Information.append(Defence)
            print '''Enter a # bonus (or penalty) for the monster's Saving Throw'''
            Saving_Throw = 'Saving Throw = ' + raw_input('Saving Throw = ')
            Information.append(Saving_Throw)            
            print "Enter the value (not bonus) for speed. It is usually 6"
            Speed = 'Speed = ' + raw_input('Speed = ')
            Information.append(Speed)
            print "Enter how many AP's (Action Points) the monster has"
            AP = 'AP = ' + raw_input('AP = ')
            Information.append(AP)
            print 'Monster Stats:'
            print
            # Attacks will be added in GUI (if I do that): Too complicated for user here
            for stat in Information:
                print stat
            print 'Verify that this is correct'
            while True:
                print 'Y/N\n'
                answer = raw_input('>').capitalize()
                if answer == 'Y':
                    newmonster.write(name + '\n')
                    AllMonsters = open('All Monsters.txt', 'w+')
                    AllMonsters.read()
                    AllMonsters.write(name + '\n')
                    AllMonsters.close()
                    for stat in Information:
                       newmonster.write(str(stat) + '\n')
                    newmonster.close()
                    return
                elif answer == 'N':
                    'Press enter to start over'
                    newmonster.close()
                    raw_input()
                continue

def LoadAll(): # This should probably do the last step too...
    '''Opens Monsters.txt, a list of all monster names, and returns a list of instances of Monster to be loaded by iterating exec() it'''
    AllMonsters = open(os.path.abspath('Monsters\\All Monsters.txt'), 'r')
    for monster in AllMonsters:
        yield '{0} = Monster("{0}")'.format(monster[:-1]) # -1 cause \n function
    # Returns generator to load all monsters as instances of Monster class

If there are any mistakes let me know, as I had to rewrite the whole thing in spaces during a class period (curse bad programming habits!)

share|improve this question
    
You may want to adopt a consistent code style: the obvious suggestion for this is PEP8. The pep8 - Python style guide checker can help you with that task. –  Pedro Romano Nov 16 '12 at 18:28
    
Have you thought about open sourcing this and putting it on github? –  Nick ODell Nov 17 '12 at 2:03
    
actually yes :P. Of course, I (might) take this further. I.E: actually simulate a battle. This would be useful (in my opinion) because I can 1) Make the (arguably) most interesting thing about a table-top game quicker and easier and 2) I can test exactly WHAT you need to defeat Orcus –  Preston Carpenter Nov 17 '12 at 2:16
    
@Nick by virtue of Preston placing it on this site, it's now released by its author under the creative commons license. That's a pretty open license. :) codereview.stackexchange.com/faq#editing –  corsiKa Nov 17 '12 at 4:51
2  
@PrestonCarpenter, my advice is release early and often. –  Nick ODell Nov 17 '12 at 5:12
show 2 more comments

1 Answer 1

up vote 7 down vote accepted

1. Comments on your code

  1. What you are doing here is creating and querying a persistent database, so it's a good idea to use an existing relational database rather than trying to implement your own in an ad-hoc fashion.

    In particular, with a relational database you can issue queries ("select a chaotic evil monster with challenge rating 2"). Queries are going to be a pain to write if you pursue your current path of having lots of small text files containing assignments.

    Another advantage of using a relational database is that you get to learn how to use and program relational databases. Time spent on this won't be wasted.

    See section 2 below for an example of how you might go about this, using Python's built-in sqlite3 module.

  2. Your constructor function (Monster.__init__) does two different jobs: if the monster already exists in your database, it loads it, but if it doesn't exist, it prompts the user interactively to create it. This kind of dual-use method is usually a bad idea: it's better to separate the two uses into two methods (this makes the code easier to read, and is more flexible since you can pick which method you actually want).

    There could be several reasons why a monster can't be found (e.g. the program is being run in the wrong directory) and you don't always want to be prompted to create it.

  3. Using eval(value) is a bad idea because value is untrusted. It could contain code that you'd really rather not run. See "Eval really is dangerous" by Ned Batchelder.

  4. You write self.addmonster() in one place. This should be addmonster(name) (or else addmonster should be made a method of the Monster class).

  5. The docstring for Monster.__init__ says "If it is already loaded in LoadedMonsters, makes a copy", but this isn't true.

  6. The pathname code is Windows-specific and might not work on other operating systems. Python provides the os.path module for manipulating pathnames in a platform-independent way.

  7. There's no input validation.

  8. You use setattr to update the attributes of the Monster object. This means that the attributes share the same namespace as the methods. So for example if the monster database contains an attack statistic, then this will overwrite the monster's attack method. It's probably best to keep the statistics in their own namespace, except for a few statistics that you refer to frequently.

2. Revised code

Here's some code that shows how you might start storing your data in table in a SQLite relational database, and how you might go about validating user input.

import collections
import itertools
import random
import re
import readline
import sqlite3

# Input validation functions.
def validate_integer(s):
    return int(s)

def validate_positive(s):
    s = int(s)
    if s <= 0: raise ValueError('not positive')
    return s

def validate_name(s):
    if not s: raise ValueError('empty string')
    return s

DICE_RE = re.compile(r'([1-9]\d*)d([1-9]\d*)([+-]\d+)?$')

def validate_dice(s):
    if not DICE_RE.match(s): raise ValueError('bad dice description')
    return s

def roll_dice(s):
    """
    Roll dice according to the description in `s` (e.g. "2d6+1") and
    return their sum.
    """
    m = DICE_RE.match(s)
    n, d, bonus = int(m.group(1)), int(m.group(2)), int(m.group(3) or '0')
    return sum(1 + random.randrange(d) for _ in range(n)) + bonus

ALIGNMENTS = [
    'lawful good',    'neutral good', 'chaotic good',
    'lawful neutral', 'neutral',      'chaotic neutral',
    'lawful evil',    'neutral evil', 'chaotic evil',
]

def validate_alignment(s):
    if not s in ALIGNMENTS: raise ValueError('bad alignment')
    return s

# Monster statistics.
Stat = collections.namedtuple('Stat', 'stat sql desc validator advice'.split())
STATS = [
    #    Stat         SQL            Desc                Validator           Advice
    Stat('name',      'TEXT UNIQUE', "name",             validate_name,      'a string'),
    Stat('hitdice',   'TEXT',        "hit dice",         validate_dice,      'like "2d6+1"'),
    Stat('armour',    'INTEGER',     "armour class",     validate_integer,   'an integer'),
    Stat('challenge', 'INTEGER',     "challenge rating", validate_positive,  'a positive integer'),
    Stat('alignment', 'TEXT',        "alignment",        validate_alignment, 'e.g. "chaotic evil"'),
]

class MonsterDB(object):
    """
    Interface to the monster database.
    """
    def __init__(self, database = 'monsters.db'):
        """
        Connect to `database`, creating it if it doesn't already exist.
        """
        self.conn = sqlite3.connect(database)
        self.conn.row_factory = sqlite3.Row
        self.execute('CREATE TABLE IF NOT EXISTS monster ({})'
                     .format(','.join('{0.stat} {0.sql}'.format(stat)
                                      for stat in STATS)))
        self.conn.commit()

    def all(self):
        """
        Generate the names of all monsters found in the database.
        """
        for name, in self.execute('SELECT name FROM monster').fetchall():
            yield name

    def execute(self, *args, **kwargs):
        """
        Execute a database query and return a cursor.
        """
        c = self.conn.cursor()
        c.execute(*args, **kwargs)
        return c

    def exists(self, name):
        """
        Return True iff a monster with the given name exists in the
        database.
        """
        c = self.execute('SELECT EXISTS(SELECT * FROM monster WHERE name=?)',
                         (name,))
        return bool(c.fetchone()[0])

    def load(self, name):
        """
        Load the monster with the given name from the database and
        return it as a new Monster object. If it does not exist, raise
        KeyError.
        """
        if not self.exists(name):
            raise KeyError("No such monster: {}".format(name))
        c = self.execute('SELECT * FROM monster WHERE name = ?', (name,))
        return Monster(**c.fetchone())

    def update(self, **kwargs):
        """
        Update some stats for a monster (creating it if it doesn't exist).
        """
        if 'name' not in kwargs:
            raise ValueError("missing name")
        keys, values = zip(*kwargs.items())
        query = ('INSERT OR REPLACE INTO monster ({}) VALUES ({})'
                 .format(','.join(keys), ','.join(('?',) * len(keys))))
        self.execute(query, values)
        self.conn.commit()

    def input(self, stat, value = None):
        """
        Input statistic `stat`, with default `value`.
        """
        if value:
            prompt = "{0.desc} ({0.advice}; default: {1}) > ".format(stat, value)
        else:
            prompt = "{0.desc} ({0.advice}) > ".format(stat)
        while True:
            try:
                v = raw_input(prompt)
                if not v and value:
                    return value
                return stat.validator(v)
            except ValueError as e:
                print(e)

    def update_interactively(self):
        """
        Update a monster interactively (creating it if doesn't exist).
        """
        action = 'create'
        assert STATS[0].stat == 'name'
        name = self.input(STATS[0])
        if self.exists(name):
            action = 'update'
            c = self.execute('SELECT * FROM monster WHERE name = ?', (name,))
            stats = dict(c.fetchone())
        else:
            stats = dict(name = name)
        for stat in STATS[1:]:
            stats[stat.stat] = self.input(stat, stats.get(stat.stat))
        desc_width = max(len(stat.desc) for stat in STATS)
        value_width = max(len(str(stats[stat.stat])) for stat in STATS)
        print('\n{:{}}  {}'.format('STAT', desc_width, 'VALUE'))
        print('{:-<{}}  {:-<{}}'.format('', desc_width, '', value_width))
        for stat in STATS:
            print('{:{}}  {}'.format(stat.desc, desc_width, stats[stat.stat]))
        if raw_input("OK to {}? > ".format(action))[0].upper() != 'Y':
            print('Abandoned.')
        else:
            self.update(**stats)

class Monster(object):
    def __init__(self, **kwargs):
        self.stat = kwargs
        self.name = kwargs['name']
        self.hp = roll_dice(kwargs['hitdice'])

    def __repr__(self):
        return '<Monster {} HP:{}>'.format(self.name, self.hp)

3. Example interaction

>>> db = MonsterDB()
>>> db.update_interactively()
name (a string) > iron golem
hit dice (like "2d6+1") > 18d10+30
armour class (an integer) > 30
challenge rating (a positive integer) > 13
alignment (e.g. "chaotic evil") > neutral

STAT              VALUE
----------------  ----------
name              iron golem
hit dice          18d10+30
armour class      30
challenge rating  13
alignment         neutral
OK to create? > yes
>>> db.load('iron golem')
<Monster iron golem HP:122>
>>> _.stat
{'hitdice': u'18d10+30', 'challenge': 13, 'armour': 30, 'alignment': u'neutral', 'name': u'iron golem'}

And in SQL you can issue queries:

sqlite> select * from monster where challenge = 1;
name          hitdice     armour      challenge   alignment   
------------  ----------  ----------  ----------  ------------
goblin        1d8+1       15          1           neutral evil
hobgoblin     1d8+2       15          1           lawful evil 
giant bee     3d8         14          1           neutral     
spider swarm  2d8         17          1           neutral     

4. Looking ahead

If you follow this implementation strategy, you'll find eventually that even with a relational database, it becomes fiddly to maintain the mapping between Python and the database. Each time you add a new stat you have to update the database schema. And the mapping will get more complex: for example you will probably want to put special powers/attacks into their own database table.

When you get to this point you'll probably want to look at Object-Relational Mapping (ORM) toolkits like SQLAlchemy. (But I think it's worth learning to use plain old SQL first: you can't properly understand ORM unless you understand both the Object and the Relational side of the mapping.)

share|improve this answer
    
Thank you very much for the time you put in to your answer. I feared that this 'question' would go unanswered as I want to do some fun things while learning in Python for a while before I move on to Java. I will look more in depth in changing these things tomorrow. Thank you again for such a great response. –  Preston Carpenter Nov 20 '12 at 21:54
    
+1, though I feel your revised Monster class has too many responsibilities. Perhaps Monster should be decoupled from database, and there could be a separate MonsterDb class that can store and retrieve Monsters to/from database? –  Janne Karila Nov 21 '12 at 19:55
    
@Janne: Good point! I re-organized the code along the lines you suggested. –  Gareth Rees Nov 22 '12 at 11:40
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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