In an RPG game I'm developing, after building a base Weapon
class, instead of having many subclasses of Weapon
, I want the weapons to be built upon dictionary parameters. Many of them will be simple static information (weight, value, etc.), but others will have conditionally defined parameters, with not only numeric/textual values: they may have to call external methods, such as spell-like effect, feat, skill, combat maneuvers, etc.).
In this example I'm using strings, like "unholy", to represent what will become method calls (with a given name) at some point.
The number of items (that will share most of this Weapon example's design, once I'm done with it), eventually, should reach the hundreds (the PRD rules I'm adapting to this game already provide a few hundred "basic items"), and allow the player to build new ones, both at runtime based on existing models (crafting) and pre-runtime (modding).
One weapon, for example, should change its stats and associated functions (spell-like effects) when equipped according to the wielder's class. But many more different conditions should be needed/used as I add fancy magic weapons, armors and items.
By managing to add new weapons without having to mess with the class itself, but instead with a dictionary/JSON (once the core code is built, hopefully with some great advice from this community), the user would benefit from accessible code and even the developer would gain in productivity by handling only dictionaries and relying on an existing core logic.
This currently relies on a few setattr
and eval
calls to handle strings from the dictionary. I would like to hear suggestions on preferable approaches, concerning performance, security, etc.. Can it be significantly improved somehow? Readability (always) matters.
# A design test for dynamic class building.
# > attributes and conditional logic are readed from a dictionary.
# This dictionary will actually reside in another file, maybe a json or
# gzipped pickled json...
WEAPONS = {
"bastard's sting": {
"equipped": False,
"magic": 2,
"on_turn_actions": [],
"on_hit_actions": [],
"on_equip": [
{
"type": "check",
"condition": (
"self.owner.is_of_class",
["antipaladin"]
),
True: [
{
"type": "action",
"action": "self.add_on_hit",
"args": ["unholy"]
},
{
"type": "action",
"action": "self.add_on_turn",
"args": ["unholy aurea"]
},
{
"type": "action",
"action": "self.set_attribute",
"args": ["magic", 5]
}
],
False: [
{
"type": "action",
"action": "self.set_attribute",
"args": ["magic", 2]
}
]
}
],
"on_unequip": [
{
"type": "action",
"action": "self.rem_on_hit",
"args": ["unholy"]
},
{
"type": "action",
"action": "self.rem_on_turn",
"args": ["unholy_aurea"]
},
{
"type": "action",
"action": "self.set_attribute",
"args": ["self.magic", 2]
}
]
}
}
class Player:
inventory = []
def __init__(self, _class):
self._class = _class
def pick_up(self, item):
"""Pick an object, put in inventory, set its owner."""
self.inventory.append(item)
item.owner = self
def is_of_class(self, _class):
"""Checks for the Character _class, not a python `class`"""
return self._class == _class
class Weapon:
"""An type of item that can be equipped/used to attack."""
def __init__(self, template):
"""Set the parameters based on a template."""
self.__dict__.update(WEAPONS[template])
def equip(self):
"""Set item status and call its on equip functions."""
self.equipped = True
for action in self.on_equip:
if action['type'] == "check":
self.check(action)
elif action['type'] == "action":
self.action(action)
def unequip(self):
"""Unset item dynamic status, call its on unequip functions."""
self.equipped = False
for action in self.on_unequip:
if action['type'] == "check":
self.check(action)
elif action['type'] == "action":
self.action(action)
def check(self, dic):
"""Check a condition and call an action according to it."""
check_act = eval(dic['condition'][0])
args = dic['condition'][1]
result = check_act(*args)
self.action(*dic[result])
def action(self, *dicts):
"""Perform action with args, both specified on dicts."""
for dic in dicts:
act = eval(dic['action'])
act(dic['args'])
def set_attribute(self, args):
name, value = args
setattr(self, name, value)
def add_on_hit(self, actions):
for action in actions:
if action not in self.on_hit_actions:
self.on_hit_actions.append(action)
def add_on_turn(self, actions):
for action in actions:
if action not in self.on_turn_actions:
self.on_turn_actions.append(action)
def rem_on_hit(self, actions):
for action in actions:
try:
self.on_hit_actions.remove(action)
except ValueError:
# We never had that but unequip tries to clean it anyway.
pass
def rem_on_turn(self, actions):
for action in actions:
try:
self.on_turn_actions.remove(action)
except ValueError:
pass
if __name__ == '__main__':
"""Let's test it!"""
weapon = Weapon("bastard's sting")
player1 = Player("paladin")
player1.pick_up(weapon)
weapon.equip()
print("Enhancement: {}, Hit effects: {}, Other effects: {}".format(
weapon.magic, weapon.on_hit_actions, weapon.on_turn_actions))
weapon.unequip()
player2 = Player("antipaladin")
player2.pick_up(weapon)
weapon.equip()
print("Enhancement: {}, Hit effects: {}, Other effects: {}".format(
weapon.magic, weapon.on_hit_actions, weapon.on_turn_actions))
Explaining the example:
The "bastard's sting" is a weapon that, in the hands of an "antipaladin", acts like an +5 unholy sword, with some fancy passive and active effects; for other classes, however, it behaves like a plain +2 sword without effects (and, in the future, will probably do some nasty stuffs in the hands of the good ones).
It should print:
For the non-antipaladin
Enhancement: 2, Hit effects: [], Other effects: []
For the antipaladin
Enhancement: 5, Hit effects: ['unholy'], Other effects: ['unholy aurea']
Notes:
As the size of the dictionary grows I intend to keep use a bzipped and pickled version of it, created once and updated in case of changes to the JSON/dictionary.
This code does not reflect the actual main structure of the game but tries to illustrate, through a working example, an approach specifically on how could weapons (and other items) be created based on dictionaries in a way that this data could provide conditional verifications and actions to interact dynamically with a huge combinations of race x attributes x features x classes behavior.
What is a weapon here is actually a component of a game object in the main code, and so is the player and other creatures. With that said, any specific comment on the main structure is still appreciated, but the main focus should be in the interaction between the
Weapon
class and the dictionary.The action "unholy area" of the weapon, in this specific example, would affect every neighbor square of the "owner" (I don't update the item position after someone picks it up, only when the item is dropped); other passive effects would do the same (heal the owner every turn, etc.). I could pass the owner to its equipment at every
on_turn
call but it sounded like such a waste;The
_class
variable holds a character class (such as Cleric, Rogue, etc.), not a Python class.
3 Answers 3
Nice code! Clean, and pretty Pythonic IMO. Without further ado...
General stuff
# This dictionary will actually reside in another file, maybe a json or
# gzipped pickled json...
WEAPONS = {
}
The first time I read this post, I was like "this guy is out of his mind" for the ridiculous nesting abuse of the dictionary syntax. Now I understand :)
item.owner = self
This is interesting to me. item
is already privately initialised for a given instance; why the redundancy?
It reads well, but I don't understand why it makes sense.
def is_of_class(self, _class):
return self._class == _class
Nitpick: Python already has two ways to check if something is a subclass / instance of something.
I think the name of this method should be is_instanceof
or so -- I know I'd be more likely to remember that, anyways. (Even if it contains a string not a Python object, I say the same.)
Code duplication
Let's take a look at this:
def equip(self):
"""Set item status and call its on equip functions."""
self.equipped = True
for action in self.on_equip:
if action['type'] == "check":
self.check(action)
elif action['type'] == "action":
self.action(action)
def unequip(self):
"""Unset item dynamic status, call its on unequip functions."""
self.equipped = False
for action in self.on_unequip:
if action['type'] == "check":
self.check(action)
elif action['type'] == "action":
self.action(action)
# --------------
def check(self, dic):
"""Check a condition and call an action according to it."""
check_act = eval(dic['condition'][0])
args = dic['condition'][1]
result = check_act(*args)
self.action(*dic[result])
def action(self, *dicts):
"""Perform action with args, both specified on dicts."""
for dic in dicts:
act = eval(dic['action'])
act(dic['args'])
# --------------
<snip></snip>
def add_on_hit(self, actions):
for action in actions:
if action not in self.on_hit_actions:
self.on_hit_actions.append(action)
def add_on_turn(self, actions):
for action in actions:
if action not in self.on_turn_actions:
self.on_turn_actions.append(action)
# --------------
def rem_on_hit(self, actions):
for action in actions:
try:
self.on_hit_actions.remove(action)
except ValueError:
pass
def rem_on_turn(self, actions):
for action in actions:
try:
self.on_turn_actions.remove(action)
except ValueError:
pass
Notice anything... macro? To me, what stands out is that each of the methods in the groups shares a literally identical shape to the eye when glancing over it, and upon closer inspection they each do the exact same thing in some different semantic context.
Whenever I come across cases of this all-but-semantic-duplication in my own code, my solution (typically) is to write one method that takes arguments of do.what
, to.what
, how
, etc. Generally, I think code that actually does something good and minimises duplication, even if that one method does get lengthy, is overall better than duplicated methods.
I could go into my spiel about why I think code deduplication is important, but you've probably heard it before and if you haven't, you can just google "why code duplication is evil" or so.
Q & A With your friendly neighbourhood static analysis tool: pylint
pylint3
has a lot of miscellaneous comments to make about your code, most of which I ignore because they are really noise when we Humans know what the code actually does.
However, pylint
did bring up some good points in the discussion we had. A synopsis:
W:109,20: Use of eval (eval-used)
W:111,17: Used * or ** magic (star-args)
W:118,18: Use of eval (eval-used)
R:115, 4: Method could be a function (no-self-use)
Okay, interesting. Let's talk about these!
Eval.
<rant class="eval" onload="javascript:eval('alert(\'HAAAAAAAAAAACKED\')')">
Eval... is cool. It's interesting, it's the fun you get to have with an interpreted language.
Having said that, it's evil in almost every case, and I would argue that this is possibly one of those cases. For instance, were this game MMORPG, I can put whatever junk I want into a JSON doc, pickle
it, hand it to this code, and boom: your game logic's hacked. I can even put arbitrary code execution in, let alone arbitrary data!
Eval. is. evil.
On a more serious note, I would recommend against eval
for real implementations in all cases unless your code, and only strings generated within Python's memory space, are what's being eval
'd.
</rant>
"Magical" stars
Actually, pylint
, get with the times, because *
and **
unzipping is totally awesome and there's no reason to avoid it. Sometimes, static analysis is, well, static. Like the kind on the radio :)
"Method could be a function"
Yeah, the first time I saw this it took me a minute too. What it's saying is that this method could be refactored into a function rather than cluttering this class's namespace because it doesn't reference self
.
This is an accurate statement: The method on line 115
does not contain the self
keyword.
Of course, we all know that with eval
, that method can do whatever anyone wants it to anyone/anything ever, but it also makes it harder to read and harder to use static analysis on.
Sometimes, static analysis tools like pylint
just spew static. But sometimes, more often than not, they contain good advice, or at least let you learn something by googling it, even if you disregard it.
Other than that stuff, I would just add writing to some log when the ValueError
catches, rather than just pass -- even if you want to ignore it now, you may want to refactor your code for speed in the future: EAFP is not necessarily faster or more efficient than LBYL, and if that time comes you will appreciate having logs about the behvaiour. Aside from refactoring, there could be an unexpected reason that throws ValueError
, the cause of bugs, and logs never hurt debugging.
-
1\$\begingroup\$ Thanks for the great code analysis. Style credits go to SublimeText3 and its linters :) -
item.owner = self
: added note #3 to the main post; -is_of_class
: added note #4 to the main post; - about the duplication: perfect, that piece of code needs a clean up! - about EAFP x LBYL: although the discussion about which is preferred usually takes (too) long, you log suggestion will be adopted!. I'm going to wait a bit more before I accept an answer, to see if something specific about the dictionary its conditions comes up. But thanks again. \$\endgroup\$Lucas Siqueira– Lucas Siqueira2016年02月03日 00:50:39 +00:00Commented Feb 3, 2016 at 0:50 -
1\$\begingroup\$ @LucasSiqueira I now understand your code more thoroughly from the notes you added :) Also, see the edit to my answer, as I added some content I forgot to earlier. \$\endgroup\$cat– cat2016年02月03日 01:11:44 +00:00Commented Feb 3, 2016 at 1:11
-
1\$\begingroup\$ Edited the code to reflect most of what you suggested above. Looks good enough to be used now. Time to split the dictionary apart, insert as many entries as I can fit in the logic, then pickle it, pack it and do some performance testings. Also I've installed 'pylint' here. Good stuff. Thanks a lot. Cheers. \$\endgroup\$Lucas Siqueira– Lucas Siqueira2016年02月03日 03:56:08 +00:00Commented Feb 3, 2016 at 3:56
Obtained a cleaner code using YAML, with serialization as a bonus. Things look a bit different, but the logic is basically the same:
- !!python/object:__main__.Weapon
base: bastard sword
masterwork: true
features:
magic_bonus:
- !!python/object:__main__.Feature
_type: magic_bonus
condition: !!python/object:__main__.Condition
condition: !!python/tuple
- !!python/tuple [owner, _class]
- ==
- antipaladin
result: {false: 2, true: 5}
passive:
- !!python/object:__main__.Feature
_type: passive
condition: !!python/object:__main__.Condition
condition: !!python/tuple
- !!python/tuple [owner, _class]
- ==
- antipaladin
result: {false: null, true: unholy aurea}
name: bastard's sting
- !!python/object:__main__.Weapon
base: bastard sword
masterwork: true
features:
critical_dmg:
- !!python/object:__main__.Feature
_type: critical_dmg
condition: !!python/object:__main__.Condition
condition: null
result: {false: null, true: 3}
name: bastard sword of dismembering
To use this yaml new classes and methods were created:
"""Build weapons from a yaml file."""
import yaml
FEATURE_TYPES = {"hit_bonus", "hit_effect", "critical_chance", "critical_dmg",
"critical_effect", "dmg_bonus", "kill_effect", "passive",
"magic_bonus", "masterwork"}
class Weapon:
"""Equipable weapon with possible features."""
base = None
masterwork = False
def __init__(self, name, base, features):
"""..."""
self.name = name
self.base = base
self.features = {}
self.features.__get
for feature in features:
_type = feature._type
self.features.setdefault(_type, [])
self.features[_type].append(feature)
def get_feature(self, key, target):
"""A workaround to handle expected key errors."""
try:
features = self.features.__getitem__(key)
return [feat.check(self.owner, target)
for feat in features if feat]
except KeyError as k:
if key in FEATURE_TYPES:
# Expected key error, weapon don't have this feature.
return None
else:
raise k
def hit_bonus(self, target=None):
"""..."""
mb = self.magic_bonus(target)
if mb:
return mb
else:
return 1 if self.masterwork else 0
def magic_bonus(self, target=None):
"""..."""
features = self.get_feature('magic_bonus', target)
return None if features is None else sum(features)
def hit_effect(self, target=None):
"""..."""
return self.get_feature("hit_effect", target)
def critical_chance(self, target=None):
"""..."""
return self.get_feature("critical_chance", target)
def critical_dmg(self, target=None):
"""..."""
features = self.get_feature('critical_dmg', target)
return None if features is None else max(features)
def critical_effect(self):
"""..."""
return self.get_feature("critical_effect", target)
def dmg_bonus(self):
"""..."""
return self.get_feature("dmg_bonus", target)
def kill_effect(self):
"""..."""
return self.get_feature("kill_effect", target)
def passive(self):
"""..."""
return self.get_feature("passive", target)
def __str__(self):
"""..."""
main = "Weapon=> {}, base: {}, features: [\n ".format(
self.name, self.base)
feat = ";\n ".join(v.__str__()
for k in self.features.values()
for v in k)
return main + feat + "]"
class Feature:
"""..."""
name = None
description = None
def __init__(self, _type, condition):
"""..."""
self._type = _type
self.condition = condition
def check(self, owner, target):
"""Ask the condition to check itself."""
return self.condition.check(owner, target)
def __str__(self):
"""..."""
return "Feature=> {}[{}]".format(self._type, self.condition)
class Condition:
"""..."""
operations = {"<": "__lt__", "<=": "__le__", "==": "__eq__",
"!=": "__ne__", ">=": "__ge__", ">": "__gt__",
"=~": "__contains__"}
def __init__(self, condition, true_value, false_value):
"""..."""
self.condition = condition
self.result = {True: true_value, False: false_value}
def check(self, owner, target):
"""Return a value according to a check."""
self.owner = owner
self.target = target
v = self.eval_condition()
# print(v)
self.owner = self.target = None
return self.result[v]
def eval_condition(self):
"""Evaluate the condition, returning the result as a boolean.
Sample conditions:
[("owner", "_class"), "==", "antipaladin"]
[("owner", "str"), ">", ("target", "str")]
[("target", "alignment"), "=~", "evil"]
Return: (boolean)
"""
if self.condition is None:
return True
v1, op, v2 = self.condition
if v1 is None:
return True
if isinstance(v1, tuple):
for level, name in enumerate(v1):
if level == 0:
tmp1 = getattr(self, name)
else:
tmp1 = getattr(tmp1, name)
v1 = tmp1
op = getattr(v1, self.operations[op])
# print(op)
if isinstance(v2, tuple):
for level, name in enumerate(v2):
if level == 0:
tmp2 = getattr(self, name)
else:
tmp2 = getattr(tmp2, name)
v2 = tmp2
return op(v2)
def __str__(self):
"""..."""
return "{} ? {} : {}".format(self.condition, self.result[True],
self.result[False])
def save_name():
"""..."""
return __file__.replace(".py", ".yaml")
class Person:
"""Dummy class representing a Person."""
def __init__(self, _class, race):
"""Dummy initialization."""
self._class = _class
self.race = race
def equip(self, weapon):
"""Dummy equip."""
self.weapon = weapon
weapon.owner = self
def attack(self, target):
"""Dummy attack."""
return self.weapon.hit_bonus(target=target)
def passive(self):
"""Dummy check for passive features."""
return self.weapon.passive
def __str__(self):
"""Print this dummy class nicely."""
return "Person=> class: {}, race:{}".format(self._class, self.race)
if __name__ == '__main__':
with open('weapons.yaml') as f:
weapons = yaml.load(f.read())
target = Person("fighter", "orc")
owner = Person("antipaladin", "human")
for w in weapons:
print()
print(w)
owner.equip(w)
hit_bonus = w.hit_bonus()
if hit_bonus:
print("hit bonus:", hit_bonus)
critical_dmg = w.critical_dmg()
if critical_dmg:
print("critical dmg:", critical_dmg)
Wikified answer to reflect the changes made after the suggestions received.
Changes:
Changed the arguments for each action/check in the dictionary to be keyword arguments (dictionaries themselves), but it's still possible to pass a list of arguments as one of those keywords.
Also changed the verification to be done directly to a property (char_class
) instead of calling a function for it.
WEAPONS = {
...
"on_equip": [
{
"type": "check",
"condition": {
'object': 'owner',
'attribute': 'char_class',
'value': "antipaladin"
},
True: [
{
"type": "action",
"action": "add_to",
"args": {
"category": "on_hit",
"actions": ["unholy"]
}
},
...
{
"type": "action",
"action": "set_attribute",
"args": {
"field": "magic",
"value": 5
}
}
...
For this example, changed the variable name from _class
to char_class
to avoid confusion with the Python class
.
class Player:
"""Represents the player character."""
inventory = []
def __init__(self, char_class):
"""For this example, we just store the class on the instance."""
self.char_class = char_class
Removed the duplicated code appointed above:
def toggle_equip(self):
"""Set item status and call its equip/unequip functions."""
if self.equipped:
self.equipped = False
actions = self.on_unequip
else:
self.equipped = True
actions = self.on_equip
for action in actions:
if action['type'] == "check":
self.check(action)
elif action['type'] == "action":
self.action(action)
...
def add_to(self, category, actions):
"""Add one or more actions to the category's list."""
action_list = getattr(self, self.action_lists[category])
for action in actions:
if action not in action_list:
action_list.append(action)
def remove_from(self, category, actions):
"""Remove one or more actions from the category's list."""
action_list = self.action_lists[category]
for action in actions:
if action in action_list:
action_list.remove(action)
And created a required definition of actions_lists
to be used by add_to
& remove_from
:
class Weapon:
"""An type of item that can be equipped/used to attack."""
equipped = False
action_lists = {
"on_hit": "on_hit_actions",
"on_turn": "on_turn_actions",
}
Replaced eval
with yet another getattr
in the check
method
def check(self, dic):
"""Check a condition and call an action according to it."""
obj = getattr(self, dic['condition']['object'])
compared_att = getattr(obj, dic['condition']['attribute'])
value = dic['condition']['value']
result = compared_att == value
self.action(*dic[result])
Adjusted the code to fit the new keyword arguments;
Final Code:
"""A design test for dynamic class building."""
# This dictionary will actually reside in another file, maybe a json or
# gzipped pickled json...
# Attributes and conditional logic relative to the items are readed from it.
WEAPONS = {
"bastard's sting": {
"magic": 2,
"on_turn_actions": [],
"on_hit_actions": [],
"on_equip": [
{
"type": "check",
"condition": {
'object': 'owner',
'attribute': 'char_class',
'value': "antipaladin"
},
True: [
{
"type": "action",
"action": "add_to",
"args": {
"category": "on_hit",
"actions": ["unholy"]
}
},
{
"type": "action",
"action": "add_to",
"args": {
"category": "on_turn",
"actions": ["unholy aurea"]
}
},
{
"type": "action",
"action": "set_attribute",
"args": {
"field": "magic",
"value": 5
}
}
],
False: [
{
"type": "action",
"action": "set_attribute",
"args": {
"field": "magic",
"value": 2
}
}
]
}
],
"on_unequip": [
{
"type": "action",
"action": "remove_from",
"args": {
"category": "on_hit",
"actions": ["unholy"]
},
},
{
"type": "action",
"action": "remove_from",
"args": {
"category": "on_turn",
"actions": ["unholy aurea"]
},
},
{
"type": "action",
"action": "set_attribute",
"args": ["magic", 2]
}
]
}
}
class Player:
"""Represents the player character."""
inventory = []
def __init__(self, char_class):
"""For this example, we just store the class on the instance."""
self.char_class = char_class
def pick_up(self, item):
"""Pick an object, put in inventory, set its owner."""
self.inventory.append(item)
item.owner = self
class Weapon:
"""An type of item that can be equipped/used to attack."""
equipped = False
action_lists = {
"on_hit": "on_hit_actions",
"on_turn": "on_turn_actions",
}
def __init__(self, template):
"""Set the parameters based on a template."""
self.__dict__.update(WEAPONS[template])
def toggle_equip(self):
"""Set item status and call its equip/unequip functions."""
if self.equipped:
self.equipped = False
actions = self.on_unequip
else:
self.equipped = True
actions = self.on_equip
for action in actions:
if action['type'] == "check":
self.check(action)
elif action['type'] == "action":
self.action(action)
def check(self, dic):
"""Check a condition and call an action according to it."""
obj = getattr(self, dic['condition']['object'])
compared_att = getattr(obj, dic['condition']['attribute'])
value = dic['condition']['value']
result = compared_att == value
self.action(*dic[result])
def action(self, *dicts):
"""Perform action with args, both specified on dicts."""
for dic in dicts:
act = getattr(self, dic['action'])
args = dic['args']
if isinstance(args, list):
act(*args)
elif isinstance(args, dict):
act(**args)
def set_attribute(self, field, value):
"""Set the specified field with the given value."""
setattr(self, field, value)
def add_to(self, category, actions):
"""Add one or more actions to the category's list."""
action_list = getattr(self, self.action_lists[category])
for action in actions:
if action not in action_list:
action_list.append(action)
def remove_from(self, category, actions):
"""Remove one or more actions from the category's list."""
action_list = self.action_lists[category]
for action in actions:
if action in action_list:
action_list.remove(action)
def test():
"""A simple test.
Item features should be printed differently for each player.
"""
weapon = Weapon("bastard's sting")
player1 = Player("paladin")
player1.pick_up(weapon)
weapon.toggle_equip()
print("Enhancement: {}, Hit effects: {}, Other effects: {}".format(
weapon.magic, weapon.on_hit_actions, weapon.on_turn_actions))
weapon.toggle_equip()
player2 = Player("antipaladin")
player2.pick_up(weapon)
weapon.toggle_equip()
print("Enhancement: {}, Hit effects: {}, Other effects: {}".format(
weapon.magic, weapon.on_hit_actions, weapon.on_turn_actions))
if __name__ == '__main__':
test()
Explore related questions
See similar questions with these tags.
creature.combat is not None
For (character) class identification, however, I'm going to use at least 32 different classes so a binary identification wouldn't work. \$\endgroup\$