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.