3

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.

  • 1
    Possible duplicate of Use Dependency Injection For Data Objects? – Niklas H Feb 23 '16 at 14:28
  • 1
    I think biggest problem is creating a validator from "type" property. Are you sure you don't need an entity for each "type" of validation? It doesn't make much sense for single entity to have different types of validations. – Euphoric Feb 23 '16 at 14:41
  • Otherwise I don't see problem in using abstractions in entities. And how those abstractions get there is irrelevant. – Euphoric Feb 23 '16 at 14:42
  • @Euphoric In this instance, the one entity is needed. The properties are identical just some different business/industry rules. The data needs to be concat and sortable so having the same type assists that. I do takwe your point that if not of the mentioned requirement it would be much better to have the 4 entities. However, altho no factory would be needed it would still make sense to have the validator injected so it can be extended/changed in the future without changing the client. – James Feb 23 '16 at 15:00
  • Entities are about differences in business rules, not about differences in properties. It is much more logical to have different entities where each has different business rule, but all have same properties. Also, sorting and similar should be applicable to any entity. It is ironic you are trying to separate validator, but keep business rules merged into one. – Euphoric Feb 23 '16 at 15:08
5

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.

  • 2
    You pointed out every single smell I was going to mention in my own answer. Thanks, I guess... but how am I supposed to get rep now?? (+1 nice answer) – MetaFight Feb 23 '16 at 15:35
  • Great input, it would be a better solution to have a derived type with overridden validation method than having a type id property and using an injected factory. The reason for the factory check in the validate method and not constructor was I don't want an exception for selects. If the data is just being read I don't care if the function is unavailable. I agree it is a smell though. – James Feb 23 '16 at 16:37
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.

  • Upvote for your edit, I was avoiding the anemic domain model anti-pattern. I also agree a derived type is much better than the id – James Feb 23 '16 at 16:46
2

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.

  • "A well done class hides state and exposes functionality." <-- well put. – MetaFight Feb 23 '16 at 15:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.