Should SRP always be respected?

Question

I'm having a hard time understanding if it is sometimes acceptable to break the Single Responsibility Principle, or if it should be avoided at all cost.

Please note that the following code has been simplified to only keep the relevant part.

I have Hero class, that represent a Character, it can posses multiples different Items.
I have multiples class that inherit from Items, for example QuestItems and BackpackItems.
When I want to add an Item on my Hero, depending on the kind of Item I should act differently.
I've tried 2 different way to do that the first one respect the SRP :

 public abstract class Item
{
    private string _Name{get;set;}

    public string Name
    {
        get
        {
            return _Name;
        }
        protected set
        {
            if (_Name != value)
            {
                _Name = value;
            }
        }
    }
}

public class QuestItem: Item
{
    public QuestItem(String name)
    {
        Name = name;
    }
}

public class BackpackItem: Item
{
    public BackpackItem(String name)
    {
        Name = name;
    }
}
public class Hero
{
   public void AddItem(Item item){
      if(item is QuestItem){
         //Do stuff to add QuestItem
         return;
      }
      if(item is BackpackItem){
         //Do stuff to add BackpackItem
         return;
      }
   }
}

Advantages : SRP is respected
Disadvantage : if I create a new item kind inheriting from Item and forget to change my AddItem function in Hero I risk a lot of trouble.

My second solution that doesn't respect SRP :
my AddItem function become

public void AddItem(Item item){
          item.AddToHero(this);
       }

my Items class become :

 public abstract class Item
{
    private string _Name{get;set;}

    public string Name
    {
        get
        {
            return _Name;
        }
        protected set
        {
            if (_Name != value)
            {
                _Name = value;
            }
        }
    }
  public abstract void AddToHero(Hero hero);
}

public class QuestItem: Item
{
    public QuestItem(String name)
    {
        Name = name;
    }
    public override void AddToHero(Hero hero)
    {
       //Do my stuff to add my QuestItem to Hero
    }
}

public class BackpackItem: Item
{
    public BackpackItem(String name)
    {
        Name = name;
    }
    public override void AddToHero(Hero hero)
    {
       //Do my stuff to add my BackpackItem to Hero
    }
}

Advantage : I can't forget the add method from a new Item kind, as I will have an error at compile time. Disadvantage : I don't respect SRP, but I for now don't really understand why it would be so bad.

What implementation should I prefer to use ? (another one ?)

Answer 1

First of all, SRP is easily mentioned and quoted, but very often hard to follow in reality. Just figuring "what is one requirement" (that should go into one class) can be hard.

In your case, one alternative could be to move the responsibility of adding an Item to some Hero (or Monster or Whatever!) into its own class. Meaning: sometimes it makes sense to create a Service class that implements a specific piece of behavior that affects other classes A, B, C, ... but that really doesn't fit into any of A, B, C, ...

You know, not all classes need to reflect "objects". They can also represent "behavior"! So consider if some ItemAdderService would help you!

Answer 2

Well, can only Heros "have" items in the repository? Aren't there locations, non-hero characters (maybe monsters) etc. who can also own items?

Since you mention that this is simplified, it's difficult to give a definitive answer to what the right approach is. Maybe neither is.

To me this kind of questions is not so much about SRP but rather about how generic something should be, and balancing that against KISS. For instance, it may be a better solution to uncouple Items and Heros altogether and instead have rules of interaction which determine what object can be used in what way be which hero or character. Because the next problem you may face is that you have different types of Heros, maybe a Wizard and a Warrior, who cannot do the same things with the same items.

Also, keep in mind that OOP (single) inheritance often does not map well to real-world situations; consider using a more compositional model instead.