Would dependency injecting into entites be considered against SRP?

Question

I asking a question on Stackoverflow earlier and someone pointed me to a previous post of his, He states that injecting a dependency into an entity violates the Single Responsibility Principle.

To save you clicking the link and give some context here is the entity being discussed

public class MyEntity
{
     private readonly IValidatorFactory _validatorFactory;
     public MyEntity(IValidatorFactory validatorFactory)
     {
         _validatorFactory = validatorFactory;
     }

     //Entity Properties Removed For Clarity

     public void Validate()
     {
         if(_validatorFactory == null)
         {
                throw new ArgumentNullException("Validator Factory was null, cannot validate this item");
         }

         var validator = _validatorFactory.GetValidator(this.ItemTypeId);
         valid = validator.Validate();

     }
}

I would have thought that having the IValidatorFactory abstracting the validation type from the entity was exactly what the SRP was about. It passes off the validation concerns to another class. It also adheres to the Open/Closed principal and allows for polymorphism/extending rather than editing existing client code.

To have any real value here, the entity must require different types of validation based on some value, which is what happens when this.ItemTypeId is used to obtain the correct IValidator reference.

Is this a complete misunderstanding of SOLID principals?

//edit

The question flagged as a possible duplicate does not really answer this. Entities are classes and should encapsulate their own validation, otherwise you are heading into anemic domain model anti pattern territory. I am specifically asking if by having a dependency controlling the validation, am I going against SOLID principals.

Answer 1

So, there's nothing here that seems like a SRP violation to me. The entity has properties and uses a helper to know if it's valid. These could be better separated (you need to get a validator yourself to test the entity), but letting an entity know if it's valid or not is fine.

Having a factory provide validators is a fine single responsibility. Having a validator that works against a single entity is a fine single responsibility.

Now, all that said, this is kind of smelly. Having an ID that implicitly maps to a type is some tight coupling for no great benefit. Having a factory for validators when you're almost always going to only have one is YAGNI and over-engineering. Checking the factory in validate rather than the constructor is a little dodgey, since you're not catching the bug where it happens.

Answer 2

SRP is a notoriously difficult principle to pin down. As I understand it, what you're saying is that the entity doesn't actually know how to validate itself, so validation is not one of its responsibilities. What Mark is saying is that an entity's responsibility is to store data and nothing else, so having a Validate method, especially one that needs injection of a factory, smells wrong.

This is the conflict that Martin Fowler talks about in the Code Smells chapter of Refactoring. He suggests that it's better to consider a responsibility "a reason to change". ie. Every class should have a single reason to change.

In that context, Mark has a point. Now you have multiple reasons to change this entity ... if the data structure changes; if the factory needs different information to select a validator; if the validator needs more information to validate.

That's not much of a violation, in truth. Some of those changes are fairly unlikely. But if an entity can validate itself, why not store itself too? You can easily inject a database object along with your validator factory. Ask yourself: When does it become too much?

Also, consider the calling code. Is it, ultimately, cleaner to say entity.Validate() than validator.Validate(entity)? And how are you generating your entity? Are you passing property values into the constructor, along with the Validator Factory, or is it mutable? This calling class seems like it might be very prone to change, for many reasons.

Which is fine if there are any benefits. But I cannot, personally, imagine what those might be.

EDIT: After a bit of extra thought, it occurs to me that this is also the discussion around Anaemic Domain Models, on which Martin Fowler also has much to say.

So there are very much two sides to that argument. However, if you're taking the fat-model approach, you have a different problem. You should then have a different entity type for each ItemTypeId, as Euphoric suggested.

Answer 3

I would have thought that having the IValidatorFactory abstracting the validation type from the entity was exactly what the SRP was about.

No. Not at all.

SRP is about a class sticking to its purpose, and there are two aspects. Frist, doing what it's supposed to do and second, not doing what it's not supposed to be doing. I've always thought of validation as the former.

Validating its own state is certainly the given class's responsibility. Why? A well done class hides state and exposes functionality. So a class uses its properties to perform functions. It does not expose properties and let, or force, client code to do said functions. And so I expect to see property validation in that class. Further, given a composition of classes every class would be doing its own specific validation and I'd expect to see the containing class doing object interaction-validation, and calling upon the participating classes to do their part.

The factory pattern is generally used for isolating/consolidating the building of complex class/class structure — "object graphs" some would say. And typically I see this when building sub-class variations on a base. But we typically do not "factoryize" those fundamental bits of a class that define what that class is. And a class is in part its own properties and their validation.