Creating a complex hierarchy for "Player Effects"

I'm coding a plugin for a game that has a Player class which has a boolean attribute frozen, which decides if the player is frozen or not. There are more "effects" like frozen, but I'm using it as an example, since solving one should solve them all.

The problem is, this frozen attribute only supports True/False, and if a player attacks an other player, freezing him for 5 seconds, and then someone else freezes the player permanently, the permanent freeze will go off after the other person's 5 second freeze ends (due to delay(5, setattr, (player, 'freeze', False)). This is why I need to implement a system that manages all the freezes applied to a player, and then unfreezes the player only when all of the freezes are gone.

Here's my original take on it, which I think is messy and I don't like the fact that I had to add the _effects dictionary to the Player class, when it's only used by the effect objects. I also have to rely on external _name attribute to identify the effect instances, and thus have to pass in an additional name parameter for the PlayerEffect decorator.

``
from ticks import delay, cancel_delay # For delayed function calls
import game.core # For the game's Player` class

class PlayerEffect(object):

def __init__(self, name, on_f=None, off_f=None, effect_cls=None):

if effect_cls is None:
class effect_cls(_PlayerEffect):
pass

effect_cls.name = name
effect_cls.on_f = on_f
effect_cls.off_f = off_f

self._name = name
self._on_f = on_f
self._off_f = off_f
self._effect_cls = effect_cls

def on(self, on_f):
return type(self)(self._name, on_f, self._off_f, self._effect_cls)

__call__ = on

def off(self, off_f):
return type(self)(self._name, self._on_f, off_f, self._effect_cls)

def __get__(self, instance, owner):
return self._effect_cls(instance)

class _PlayerEffect(object):

name = None
on_f

I'd like to suggest a completely different implementation strategy for the, er, effects of the effects. You have a thing with mutable state (Player) and operations that change that state (self.move_state = ...) based on timers. You've had to add a special system to make the timers coordinate with each other, and if you ever make a mistake in the ”off” methods you will have a broken effect that sticks around.

Instead, make the state of the player a function of the effects. Here's a simple implementation to illustrate (note: may be Python 2-ish because I'm not familiar with 3; also untested):

def apply_effects(effects, attribute, base_value):
    value = base_value
    for effect in effects:
        value = effect.modify(attribute, value)
    return value

class Player(game.core.Player):
    def get_frozen(self):
        return apply_effects(self._effects, 'frozen', False)

    def get_move_state(self):
        return apply_effects(self._effects, 'move_state', MoveStates.NORMAL)

class Effect(object):
    """Base class for effects."""
    def modify(self, attribute, value):
        return value

class FrozenEffect(Effect):
    def modify(self, attribute, value):
        if attribute == 'frozen':
            return True
        else:
            super(FrozenEffect, self).modify(attribute, value)

class HoverEffect(Effect):
    def modify(self, attribute, value):
        if attribute == 'move_state' && value != MoveStates.FLY:
            return MoveStates.HOVER
        else:
            super(HoverEffect, self).modify(attribute, value)

class FlyEffect(Effect):
    def modify(self, attribute, value):
        if attribute == 'move_state':
            return MoveStates.FLY
        else:
            super(FlyEffect, self).modify(attribute, value)

(Note: If all effects are this simple, then they could be just functions instead of classes. Making them classes allows adding more operations, for example ”get name and icon” to show the player what effects apply.)

Then you would still manage the lifetime of the effects much like you are now, but the effects do not need to know each others' names. The only special rule here is that the FLY move state takes priority over the HOVER move state, which is presumably a basic rule about that enumerated type. (An alternative implementation would be to define a sort order for effects, sorting the list of effects so that the fly effect is always applied after the hover effect, or divide effects into groups that are applied in a specific order.)

This architecture also allows you to easily implement additive or multiplicative effects that stack with each other (e.g. speed/damage boosts).

If you need to actually have something happen when an effect ends, then save the computed values of the attributes, and when an effect ends compute the new attributes and do the relevant thing if they're different. This means that the happening is completely dependent on the state of the player and not what effects just happened. (For example, suppose two hover effects wear off at exactly the same moment: you don't want to start a ”drowned because fell in water” event twice, only once. This strategy can't make that mistake.)

StackExchange Code Review Q#96486, answer score: 3