A composite validator pattern

Have you ever encountered something like this?
(Note: #r denotes a collapsed region of code)

public class MyBulkyClassThatDoesALot
{
    #r Private fields
    #r Constructor
    #r Another constructor
    #r Public properties
    #r Public methods
    #r Private methods

    #region Validation

    // meanwhile, somewhere on line 20485
    public ValidationResult Validate()
    {
        ValidationResult result;
        CheckThatSomethingHolds(result);
        CheckThatSomethingElseHolds(result);

        if (this.somesituation)
        {
            CheckSituationDependentCondition(result);
            CheckSituationDependentOtherCondition(result);
        }

        ...
        // Somewhere on line 20585
        CheckLastThing(result);
        return result;
    }

    private void CheckThatSomethingHolds(ValidationResult result)
    {
        if (this.somecondition)
        {
            result.AddError(new ValidationError("Condition X failed"));
        }
    }

    private void CheckThatSomethingElseHolds(ValidationResult result)
    {
        if (this.somecondition)
        {
            result.AddError(new ValidationError("Condition Y failed"));
        }
    }

    private void CheckSituationDependentCondition(result)
    {
        if (this.somecondition)
        {
            result.AddError(new ValidationError("Condition SituationDependent1 failed"));
        }
    }

    // Are you tired of scrolling yet? Good, because this is how whomever reads this code shall feel in real life ;)

    private void CheckSituationDependentOtherCondition(result) { ... snip }

    // A sigh of relief when you reach the last one
    private void CheckLastThing(ValidationResult result)
    {
        if (this.somecondition)
        {
            result.AddError(new ValidationError("Condition Z failed"));
        }
    }

    #endregion Validation

    // And then, of course, there is a lot of code after this. If you're unlucky, there wasn't even a #region around all the Validation methods and you have to hunt them down one by one because they are scattered through the code.
}

Don’t worry, you’re not the first one to end up with this. Often, it started as a single validation rule that was just implemented directly in the class to be validated. However, there are some things to look out for.

Firstly, this approach nearly always validates public properties: You will seldom write validators that inspect private properties of the instance and then report this to the user. After all, what could the user have done to fix it? Most likely, you’ve written these valiators to inform the user that his/her input is incorrect and needs to be fixed in some specific way. In other words: Public properties that were set from the outside need to be set to other values or combinations.

Secondly, in this manner, it can be tough to unit test this. Sure, you can make the Check_ methods public and instantiate MyBulkyClassThatDoesALot, but given the name it may have a lot of dependencies that may cascade into infinity when you try to instantiate it in the first place.

By the time you create the third validation rule, it’s wise to split off some of this code into a dedicated container. You should go about this in a careful manner. Below, a suggested path you may take to split off a validator.

Introduce interface

Introduce an interface for MyBulkyClassThatDoesALot. This may seem like only a minor step (and it is in terms of changes and has hardly any risk of regression in this specific case), but it’s actually a huge step towards unit testing your validator without having to instantiate your bulky class with loads of dependencies.


public interface IMyBulkyClassThatDoesALot 
{
    #r Public properties
    #r Public methods
}

public class MyBulkyClassThatDoesALot : IMyBulkyClassThatDoesALot
{
    #r Private fields
    #r Constructor
    #r Another constructor
    #r Public properties
    #r Public methods
    #r Private methods
    #r Validation
}

Save, run all tests in the entire solution to check that nothing was affected (I estimate the chance of this to be very, very low, but be on the safe side!)

Annnnd… commit your changes! Don’t skip this, it’s a checkpoint to return to should you encounter difficulties further down the line. Many a developer made changes without saving, encountered some diffulty and then had no other choice but to revert all changes, because (s)he had no stable state halfway to return to.

Public static

Make the Check_ methods public static and pass in this as first argument of each Check_ method. Use IMyBulkyClassThatDoesALot as parameter type



    ...

    public ValidationResult Validate()
    {
        ValidationResult result;
        CheckThatSomethingHolds(this, result);
        CheckThatSomethingElseHolds(this, result);

        if (this.somesituation)
        {
            CheckSituationDependentCondition(this, result);
            CheckSituationDependentOtherCondition(this, result);
        }

        ...
        CheckLastThing(this, result);
        return result;
    }

    private void CheckThatSomethingHolds(IMyBulkyClassThatDoesALot bulky, ValidationResult result)
    {
        if (bulky.somecondition)
        {
            result.AddValidationResult(new ValidationResult("Condition X failed"));
        }
    }

    private void CheckThatSomethingElseHolds(IMyBulkyClassThatDoesALot bulky, ValidationResult result)
    {
        if (bulky.somecondition)
        {
            result.AddValidationResult(new ValidationResult("Condition Y failed"));
        }
    }

    ... <etc.>

    #endregion Validation

Now, you may encounter a problem. What if you were using members in your Check_ methods that are not public? You’ll run into compile errors now. You have some options:

  • The member is derived from some other property that is public: Do not use this private member, but use the public member instead. Downside: code duplication, unless your validator was the only one accessing the original private member, in which case you can toss out the private member altogether.
  • Making the member part of your interface makes sense: Make it public.
  • None of the above: There are other options, but they may change the behavior of your class, such as checking this case in the constructor and throwing an exception (The rationale being the user cannot do anything to prevent or circumvent this.). Talk to your architect.

Run all tests and commit.

Add unit tests

This is the part where you test the heck out of each and every method, to cover each and every scenario/use case, so that you are fully covered against regression when you complete the final step of this refactoring case study.


[TestFixture]
public class ValidationTests
{
    [Test]
    public void CheckThatSomethingHolds_<SomeCondition>_ShouldBeValid()
    {
        // Arrange
        var bulkyClassMock = new Mock();
        ValidationResult result;

        // Act
        MyBulkyClassThatDoesALot.CheckThatSomethingHolds(bulkyClassMock.Object, result);

        // Assert
        Assert.IsTrue(result.IsValid);
    }

    [Test]
    public void CheckThatSomethingHolds_<OtherCondition>_ShouldReturnValidationError()
    {
        // Arrange
        var bulkyClassMock = new Mock();
        ValidationResult result;

        // Act
        MyBulkyClassThatDoesALot.CheckThatSomethingHolds(bulkyClassMock.Object, result);

        // Assert
        Assert.IsFalse(result.IsValid);
        Assert.AreEqual("Condition X failed", result.Errors.Single().Message);
    }

    [Test]
    public void CheckThatSomethingElseHolds_<SomeCondition>_ShouldBeValid()
    {
        ..
    }

    ...
}

Run all tests and commit 🙂

There is a real chance that you cannot spend too much time on this refactoring now. There may be a deadline or other factors that make it undesirable to spend more time on this. If so, this is a good place to stop. You’ve brought all your validators under test and have left a breadcrumbs trail for the next developer to follow in your footsteps. If and when you continue, you can pick up where you left off.

Validator class and interfaces

First, introduce an interface that each of your validators will adhere to:


public interface IValidator
{
    public void Validate(IMyBulkyClassThatDoesALot bulky, ValidationResult result);
}

Introduce separate validators

One by one, introduce a validator class for each Check_ method, instantiate the class in MyBulkyClassThatDoesALot.Validate() and update the unit tests accordingly:


public class SomethingHoldsValidator : IValidator
{
    public void Validate(IMyBulkyClassThatDoesALot bulky, ValidationResult result)
    {
        if (bulky.somecondition)
        {
            result.AddValidationResult(new ValidationResult("Condition X failed"));
        }
    }
}
public class MyBulkyClassThatDoesALot
{
    ...

    public ValidationResult Validate()
    {
        ValidationResult result;
        var validator1 = new SomethingHoldsValidator();
        validator1.Validate(this, result);

        var validator2 = new SomethingElseHoldsValidator();
        validator2.Validate(this, result);

        if (this.somesituation)
        {
            var validator3 = new SituationDependentConditionValidator();
            validator3.Validate(this, result);
            var validator4 = new SituationDependentOtherConditionValidator();
            validator4.Validate(this, result);
        }

        ...
        var validatorN = new LastThingValidator();
        validatorN.Validate(this, result);
        return result;
    }

    private void CheckThatSomethingHolds(IMyBulkyClassThatDoesALot bulky, ValidationResult result)
    {
        if (bulky.somecondition)
        {
            result.AddValidationResult(new ValidationResult("Condition X failed"));
        }
    }

    ...
}

[TestFixture]
public class SomethingHoldsValidatorTests
{
    [Test]
    public void Validate_<SomeCondition>_ShouldBeValid()
    {
        // Arrange
        var bulkyClassMock = new Mock();
        SomethingHoldsValidator validator = new SomethingHoldsValidator();
        ValidationResult result;

        // Act
        validator.Validate(bulkyClassMock.Object, result);

        // Assert
        Assert.IsTrue(result.IsValid);
    }

    [Test]
    public void Validate_<OtherCondition>_ShouldReturnValidationError()
    {
        // Arrange
        var bulkyClassMock = new Mock();
        SomethingHoldsValidator validator = new SomethingHoldsValidator();
        ValidationResult result;

        // Act
        validator.Validate(bulkyClassMock.Object, result);

        // Assert
        Assert.IsFalse(result.IsValid);
        Assert.AreEqual("Condition X failed", result.Errors.Single().Message);
    }
}

Run all tests and commit.

Introduce composite validator

Introduce a composite validator, that implements the IValidator interface and takes, in its constructor, a list of validators it needs to run when its Validate() method is called. You may use a builder pattern or IoC container to instantiate this composite validator with all validation rules it needs to run, relieving MyBulkyClassThatDoesALot from this responsibility.


public class CompositeValidator : IValidator
{
    private IEnumerable _validators;

    public CompositeValidator(IEnumerable validators)
    {
        _validators = validators;
    }

    public void Validate(IMyBulkyClassThatDoesALot bulky, ValidationResult result)
    {
        foreach (var validator in _validators)
        {
            validator.Validate(bulky, result);
        }
    }

    public CompositeValidator Build()
    {
        List validators = new List
        {
            new SomethingHoldsValidator(), // Did your validator require additional values? Pass them into the Build() method and on to the constructor of your validator.
            new SomethingElseHoldsValidator(),
            new SituationDependentConditionValidator(),
            new SituationDependentOtherConditionValidator(),
            ...
            new LastThingValidator()
        };
    }
}

Note: In order to complete this pattern, push the check on if (bulky.somesituation) into the classes SituationDependentConditionValidator and SituationDependentOtherConditionValidator. In other words, each of these classes will do the if-check inside their Validate() method!

Be sure to unittest this composite validator as well. Finally, MyBulkyClassThatDoesALot will no longer contain the validation methods and will simply make a call to the CompositeValidator.Build() method.

public class MyBulkyClassThatDoesALot
{
    #r Private fields
    #r Constructor
    #r Another constructor
    #r Public properties
    #r Public methods
    #r Private methods

    #region Validation

    public ValidationResult Validate()
    {
        CompositeValidator validator = CompositeValidator.Build(); 
        ValidationResult result;
        validator.Validate(this, result);
        return result;
    }

Other ideas

I would prefer to go one step further and have each IValidator take only one parameter (IMyBulkyClassThatDoesALot) and have it return a ValidationResult, or rather, a list of ValidationMessages. I might not even have (I)MyBulkyClassThatDoesALot expose a Validate() method and leave that to the calling class instead. It’s a matter of taste.

Leave a Reply

Your email address will not be published. Required fields are marked *