Single Responsibility Principle(SRP) and class structure of my rpg looks "weird"

Question

I'm making a role playing game just for fun and to learn more about the SOLID principles. One of the first things I'm focusing on is SRP. I have a "Character" class that represents a character in the game. It has things like Name, Health, Mana, AbilityScores, etc.

Now, normally I would also put methods in my Character class so it would look something like this...

   public class Character
   {
      public string Name { get; set; }
      public int Health { get; set; }
      public int Mana { get; set; }
      public Dictionary<AbilityScoreEnum, int>AbilityScores { get; set; }

      // base attack bonus depends on character level, attribute bonuses, etc
      public static void GetBaseAttackBonus();  
      public static int RollDamage();
      public static TakeDamage(int amount);
   }

But because of SRP I've decided to move all the methods out into a separate class. I named that class "CharacterActions" and now the method signatures look like this...

public class CharacterActions
{
    public static void GetBaseAttackBonus(Character character);
    public static int RollDamage(Character character);
    public static TakeDamage(Character character, int amount);
}

Notice that I now have to include the Character object I am using in all my CharacterActions methods. Is this the right way to go about leveraging SRP? It seems to go completely against the OOP concept of encapsulation.

Or am I doing something completely wrong here?

ONE thing I do like about this is that my Character class is very clear on what it does, it simply represents a Character object.

Answer 1

Update - I've redone my answer because, after a half-night's sleep, I really didn't feel that my previous answer was very good.

To see an example of the SRP in action, let's consider a very simple character:

public abstract class Character
{
    public virtual void Attack(Character target)
    {
        int damage = Random.Next(1, 20);
        target.TakeDamage(damage);
    }

    public virtual void TakeDamage(int damage)
    {
        HP -= damage;
        if (HP <= 0)
            Die();
    }

    protected virtual void Die()
    {
        // Doesn't matter what this method does right now
    }

    public int HP { get; private set; }
    public int MP { get; private set; }
    protected Random Random { get; private set; }
}

OK, so this would be a pretty boring RPG. But this class makes sense. Everything here is directly related to the Character. Every method is either an action performed by, or performed on the Character. Hey, games are easy!

Let's focus on the Attack part and try to make this halfway interesting:

public abstract class Character
{
    public const int BaseHitChance = 30;

    public virtual void Attack(Character target)
    {
        int chanceToHit = Dexterity + BaseHitChance;
        int hitTest = Random.Next(100);
        if (hitTest < chanceToHit)
        {
            int damage = Strength * 2 + Weapon.DamageRating;
            target.TakeDamage(damage);
        }
    }

    public int Strength { get; private set; }
    public int Dexterity { get; private set; }
    public Weapon Weapon { get; set; }
}

Now we're getting somewhere. The character sometimes misses, and damage/hit go up with level (assuming that STR increases as well). Jolly good, but this is still pretty dull because it doesn't take into account anything about the target. Let's see if we can fix that:

public void Attack(Character target)
{
    int chanceToHit = CalculateHitChance(target);
    int hitTest = Random.Next(100);
    if (hitTest < chanceToHit)
    {
        int damage = CalculateDamage(target);
        target.TakeDamage(damage);
    }
}

protected int CalculateHitChance(Character target)
{
    return Dexterity + BaseHitChance - target.Evade;
}

protected int CalculateDamage(Character target)
{
    return Strength * 2 + Weapon.DamageRating - target.Armor.ArmorRating -
        (target.Toughness / 2);
}

At this point, the question should already be forming in your mind: Why is the Character responsible for calculating its own damage against a target? Why does it even have that ability? There's something intangibly weird about what this class is doing, but at this point it's still sort of ambiguous. Is it really worth refactoring just to move a few lines of code out of the Character class? Probably not.

But let's look at what happens when we start adding more features - say from a typical 1990s-era RPG:

protected int CalculateDamage(Character target)
{
    int baseDamage = Strength * 2 + Weapon.DamageRating;
    int armorReduction = target.Armor.ArmorRating;
    int physicalDamage = baseDamage - Math.Min(armorReduction, baseDamage);
    int pierceDamage = (int)(Weapon.PierceDamage / target.Armor.PierceResistance);
    int elementDamage = (int)(Weapon.ElementDamage /
        target.Armor.ElementResistance[Weapon.Element]);
    int netDamage = physicalDamage + pierceDamage + elementDamage;
    if (HP < (MaxHP * 0.1))
        netDamage *= DesperationMultiplier;
    if (Status.Berserk)
        netDamage *= BerserkMultiplier;
    if (Status.Weakened)
        netDamage *= WeakenedMultiplier;
    int randomDamage = Random.Next(netDamage / 2);
    return netDamage + randomDamage;
}

This is all fine and dandy but isn't it a little ridiculous to be doing all of this number-crunching in the Character class? And this is a fairly short method; in a real RPG this method might extend well into the hundreds of lines with saving throws and all other manner of nerditude. Imagine, you bring in a new programmer, and they say: I got a request for a dual-hit weapon that's supposed to double whatever the damage would normally be; where do I need to make the change? And you tell him, Check the Character class. Huh??

Even worse, maybe the game adds some new wrinkle like, oh I don't know, a backstab bonus, or some other type of environment bonus. Well how the hell are you supposed to figure that out in the Character class? You'll probably end up calling out to some singleton, like:

protected int CalculateDamage(Character target)
{
    // ...
    int backstabBonus = Environment.Current.Battle.IsFlanking(this, target);
    // ...
}

Yuck. This is awful. Testing and debugging this is going to be a nightmare. So what do we do? Take it out of the Character class. The Character class should only know how to do things that a Character would logically know how to do, and calculating the exact damage against a target really isn't one of them. We'll make a class for it:

public class DamageCalculator
{
    public DamageCalculator()
    {
        this.Battle = new DefaultBattle();
        // Better: use an IoC container to figure this out.
    }

    public DamageCalculator(Battle battle)
    {
        this.Battle = battle;
    }

    public int GetDamage(Character source, Character target)
    {
        // ...
    }

    protected Battle Battle { get; private set; }
}

Much better. This class does exactly one thing. It does what it says on the tin. We've gotten rid of the singleton dependency, so this class is actually possible to test now, and it feels a lot more right, doesn't it? And now our Character can concentrate on Character actions:

public abstract class Character
{
    public virtual void Attack(Character target)
    {
        HitTest ht = new HitTest();
        if (ht.CanHit(this, target))
        {
            DamageCalculator dc = new DamageCalculator();
            int damage = dc.GetDamage(this, target);
            target.TakeDamage(damage);
        }
    }
}

Even now it's a little questionable that one Character is directly invoking another Character's TakeDamage method, and in reality you'd probably just want the character to "submit" its attack to some sort of battle engine, but I think that part is best left as an exercise to the reader.

---

Now, hopefully, you understand why this:

public class CharacterActions
{
    public static void GetBaseAttackBonus(Character character);
    public static int RollDamage(Character character);
    public static TakeDamage(Character character, int amount);
}

...is basically useless. What's wrong with it?

• It doesn't have a clear purpose; generic "actions" are not a single responsibility;
• It fails to accomplish anything that a Character can't already do by itself;
• It depends entirely on the Character and nothing else;
• It will probably require you to expose parts of the Character class that you really want private/protected.

The CharacterActions class breaks the Character encapsulation and adds little to nothing of its own. The DamageCalculator class, on the other hand, provides a new encapsulation and helps to restore the cohesion of the original Character class by eliminating all of the unnecessary dependencies and unrelated functionality. If we want to change something about the way damage is calculated, it's obvious where to look.

I'm hoping that this explains the principle better now.

Answer 2

SRP doesn't mean that a class shouldn't have methods. What you've done is created a data-structure rather than a polymorphic object that. There are benefits to doing so, but it's probably not intended or needed in this case.

One way you can often tell if an object is violating SRP is to look at the instance variables that the methods within the object use. If there are groups of methods that use certain instance variables, but not others, that's usually a sign that your object can be split up according the the groups of instance variables.

Also, you probably don't want your methods to be static. You'll likely want to leverage polymorphism -- the ability to do something different within your methods based on the type of the instance on which the method was called.

For example, would your methods need to change if you had an ElfCharacter and a WizardCharacter? If your methods will absolutely never change and are completely self contained, then perhaps static is alright... but even then it makes testing much more difficult.

Answer 3

The method that I go about when designing classes and which is what OO is based on is that an object models a real world object.

Let's take a look at Character... Designing a Character class can be quite interesting. It might be a contract ICharacter This means that anything that wants to be a character should be able to perform Walk(), Talk(), Attack(), Have some Properties such as Health, Mana.

You could then have a Wizard, a Wizard having special properties and the way he attacks is different than, let's say a Fighter.

I tend to not get too forced by Design Principles, but also think about modelling a real world object.

Answer 4

I think it depends if your characters behaviour can change. For example, if you wanted the available actions to change (based on something else happening in the RPG) you could go for something like:

public interface ICharacter
{
    //...
    IEnumerable<IAction> Actions { get; }
}

public interface IAction
{
    ICharacter Character { get; }
    void Execute();
}

public class BaseAttackBonus : IAction
{
    public BaseAttackBonus(ICharacter character)
    {
        Character = character;
    }

    public ICharacter Character { get; private set; }   

    public void Execute()
    {
        // Get base attack bonus for character...
    }
}

This allows your character to have as many actions as you want (meaning you can add/remove actions without changing character class), and each action is responsible for only itself (but knows about the character) and for actions which have more complex requirements, inherit from IAction to add properties, etc. You'd probably want a different return value for Execute, and you may want a queue of actions, but you get the drift.

Note use of ICharacter instead of a Character, since characters may have different properties and behaviour (Warlock, Wizard, etc), but they probably all have actions.

By separating out actions, it also makes testing much easier, since you can now test each action without having to wire up a full character, and with ICharacter you can create your own (mock) character more easily.

Answer 5

I don't know that I'd really call that type of class SRP in the first place. "Everything dealing with foo" is usually a sign that you're not following SRP (which is okay, it's not appropriate for all types of designs).

A good way of looking at SRP boundaries is "is there anything I can change about the class that would leave most of the class intact?" If so, separate them out. Or, to put it another way, if you touch one method in a class you should probably have to touch all of them. One of the advantages of SRP is that it minimizes the scope of what you're touching when you make a change - if another file is untouched, you know you didn't add bugs to it!

Character classes in particular are at high degrees of danger of becoming God-classes in RPGs. A possible way of avoiding this would be to approach this from a different way - start with your UI, and at each step, simply assert that the interfaces you wish existed from the perspective of the class you're currently writing existed already. Also, look into the Inversion of Control principle and how design changes when using IoC (not necessarily an IoC container).