Is it correct application of the SRP(single responsibility principle)?

Question

I have a java class:

class User {
 private String name;
 private String address;
 private int age;
 private BigDecimal salary;
 // other fields
 //getters setters
}

I can receive a map of new values in these fields and update it. It looks like this: ChangeItem changeItem where changeItem.key is field's name and changeItem.value is the field's value

I create strategies for updating each field. For example common interface:

public interface UpdateStrategy<T> {
    T updateField(T t, ChangeItem changeItem) throws ValidationExceptions;
}

And some implementation:

public class UpdateNameStrategy implements UpdateStrategy<User> {

    private static final Pattern USER_NAME = Pattern.compile(...);

    @Override
    public User updateField(User user, ChangeItem changeItem) throws ValidationExceptions {

        String fieldValue = changeItem.value;

        if (!validate(fieldValue))
            throw new ValidationExceptions(changeItem);

        user.setName(fieldValue);

        return user;
    }

    private boolean validate(String value){
        return USER_NAME.matcher(value).matches();
     }
}

In the real project I have 40 fields and 40 strategies for each field(with different validation and logic).

I think this class violates the SRP(single responsibility principle). And I move validation logic to separately class. I change the validation method to:

public class UpdateNameStrategy implements UpdateStrategy<User> {

    @Override
    public User updateField(User user, ChangeItem changeItem) throws ValidationExceptions {

        String fieldValue = changeItem.value;

        ValidateFieldStrategy fieldValidator = new UserNameValidate(fieldValue);

        if (!fieldValidator.validate())
            throw new ValidationExceptions(changeItem);

        return user;
    }
}

and

public class UserNameValidate implements ValidateFieldStrategy {

    private static final Pattern USER_NAME = Pattern.compile(...);

    private String value;

    public UserNameValidate(String value) {
        this.value = value;
    }

    @Override
    public boolean validate() {
        return USER_NAME.matcher(value).matches();
    }
}

And now I have 40 strategies for update fields and 40 validators. Is it the correct way? Or maybe I can change this code more clear?

Answer 1

I'm sorry for being blunt, my eyes are bleeding while looking at this. You took one unnecessarily complicated validation model and you split it in two to make it even more complicated. And none of it has much to do with the Single Responsibility Principle.

Without knowing anything specific to your domain problem, this looks like a superfluous usage of the Strategy pattern.

I've never seen a legitimate domain problem requiring a validation strategy split like this, for every single field.

An object in a domain is not just a collection of fields. It is also behavior governing the fields (which is the object state) and the rules governing the mutability of that state.

In general we want rich objects with behavior. And that behavior typically includes validation.

I sincerely doubt that every single field in a model requires validation to this level of granularity. Put the validation in the object's setter methods and be done with it.

You are killing yourself doing all this elaborate setup. We all want structure, but at some point all of this is just ceremony for building very tall sand castles.

Validation in general is part of an object. And an object is responsible, it is its responsibility to govern its state, the collection of fields and values it possesses and controls.

Single Responsibility Principle does not mean extracting the responsibility of validating fields out of an object. That responsibility is intrinsic to the object.

Single Responsibility Principle concerns itself with "external" responsibility, the responsibility of an object to provide a single coherent function (or set of coherent functions) to someone that uses that object.

Consider a Printer object. This object is responsible to print. It is not responsible to manage the network connections between a printer and a user, for instance.

SRP is not limited to classes, but also packages and modules. A Mathematics module should provide you with, obviously, mathematical routines. It should not provide routines for filesystem manipulation, right?

That's what the SRP is about. What you are doing, extracting validation behavior out of an object, that has little, if anything, to do with SRP.

Sometimes one might want to extract out common validation routines (check if a string is black or null, or whether a number is a natural number.)

So you might have a class like this:

public class User {
    // some fields, blah blah

    public void setName(final String aName){
        if( aName == null || a.aName.trim().length() < 1){
            throw new SomeException("empty string blah blah");
        }
        this.name=aName.trim(); // model requires this to be trimmed.
    }

    public void setRawField(final String aValue){
        if( aName == null || a.aName.trim().length() < 1){
            throw new SomeException("empty string blah blah");
        }
        this.rawField=aValue;  // model requires this to not be trimmed.
    }

public void setRawField2(final String aValue){
    // model requires this field to be non-null,
    // can be blank, and if not blank, must be all lower case.
    if(aValue == null) {
        throw new NullPointerException("null string blah blah");
    }
    this.rawField2=aValue.toLowerCase();

}

changed into a class that delegates minutia to an external validation utility class or module.

public class User {
     // some fields, blah blah

     public void setName(final String aName){
         // model requires this to be trimmed
         this.name=Validator.notEmptyOrDie(aName).trim();
     }

     public void setRawField(final String  aValue){
         // model requires this to *not* be trimmed
         this.rawField=Validator.notEmptyOrDie(aValue);
     }

public void setRawField2(final String aValue){
    // model requires this field to be non-null,
    // can be blank, and if not blank, must be all lower case.
    // too contrive to refactor, leave it here.
    if(aValue == null) {
        throw new NullPointerException("null string blah blah");
    }
    this.rawField2=aValue.toLowerCase();

}

 public class Validator {
     static public String notEmptyOrDie(final String aString){
         if( aString == null || aString.trim().length() < 1){
             throw new SomeException("empty string blah blah");
         }
         return aString;
 }

This is an approach I actually follow, to refactor parts of common validation. I factor out minutia.

But the core validation logic, if any, it remains in the object. Notice that validation is still part of the User class. All that got extracted is the minutia.

The logic that declares the intent of validation (check if black or die) still remains part of the User class. It is intrinsic to the class' behavior.

In some models, the User class might not require validation at all. It might be just a data shuttle, a POJO.

OTH, in a model that requires it to validate its state, that state should usually go inside the class, and a developer must have a very good argument for extricating that logic the way you did in your sample code.

SRP says nothing about how you compose responsibility internal to the object, only external to consumers of said object.

As a rule of thumb, validation of object fields belong to the object as logic internal to the object. It is intrinsic to the object's behavior, invariants, pre conditions and post conditions.

Very rarely you extract out the entire validation out of an object (unless we are talking about POJOs serialized and deserialized by an external package, and with validations added declaratively via annotations or some sort of controlling configuration descriptor.)

Hit me up if you still have any questions. Not sure how fast I can answer back, but I don't mind to answer questions if I can.

**** EDIT ***

User @qujck mentions the valid concern in this proposed approach, that it is not possible to differentiate all validation exceptions (becuase they use common exceptions for all.)

One possibility (which I've used) is to have overloaded and/or polymorphic validators:

public class Validator {
    static public String notEmptyOrDie(final String aString){
        return Validator.notEmptyOrDie(aString, null);
    }

    static public String notEmptyOrDie(final String aString, 
                                       final String aFieldName){
        if( aString == null || aString.trim().length() < 1){
            throw new SomeException( 
                (aFieldName==null? "" : aFieldName + " ") 
                + "empty string blah blah");
        }
        return aString;
    }
}

If one uses a hierarchy of validation exceptions with common constructors, then one could take this further by passing the desired exception class, and use reflection to create instances to be thrown.

I've done that also. Actually, I'm doing that now for a common error-throwing mechanism in an EJB layer that itself reaches to another system via network protocols.

But that's something I do to cope with an existing system, not something I would do if I had a design choice. And it still limits itself to refactoring validation or error handling to its core elements.

Actual, object-specific validation still remains at/within the object itself.