2

A few developers and I are attempting to refactor a class that has grown too large. Currently this class is around 3K lines long. The goal of the refactor is to make the logic more maintainable.

The class holds our validation logic, which performs various checks to make sure things are good before moving to the next step of the process.

Currently each validation is a function of the validation class. They all have the same method signature.

One idea we had is to create a class for each validation, with a factory to give us a list of all validations that are needing to run.

A simple implementation of this is below:

Here's the validation interface:

public interface IValidationType
{
    bool Validate();
}

With an implementation looking like this:

public class ValidationType1 : IValidationType
{
    private readonly IDependency1 _dependency1;

    public ValidationType1(IDependency1 dependency1)
    {
        _dependency1 = dependency1;
    }

    public bool Validate()
    {
        return _dependency1.SomeFunction();
    }
}

With a factory class that accepts IValidationType's as dependencies:

public class ValidationFactory : IValidationFactory
{
    private readonly IList<IValidationType> _validations;

    public ValidationFactory(
        IValidationType validation1,
        IValidationType validation2)
    {
        _validations = new List<IValidationType>
        {
            validation1,
            validation2
        };
    }

    public IEnumerable<IValidationType> RetrieveValidations()
    {
        return _validations;
    }
}

Currently our plan is to abuse our IOC container to handle the dirty work with named registers:

public class ValidationAutofacModule : Module
{
    protected override void Load(ContainerBuilder builder)
    {
        builder.RegisterType<ValidationType1>().Named<IValidationType>("Validation1");
        builder.RegisterType<ValidationType2>().Named<IValidationType>("Validation2");

        builder.Register<IValidationFactory>(
            x => new ValidationFactory(
                x.ResolveNamed<IValidationType>("Validation1"),
                x.ResolveNamed<IValidationType>("Validation2")));
    }
}

One of our concerns with this implementation is that for each new validation created the developer would need to register it in the factory, and also in the IOC container. It also smells having to abuse the IOC container to get this to work.

Is this an anti-pattern? Is there a better way?

  • 1
    Suggestion: KIS - keep it simple. What’s the gain with this refactor? Keep the validation within the same class and build/test out the system to ensure correct working. After that discuss what the change points are. If validation is constantly changed across multiple classes only then do some more refactoring. That too I’d suggest till the time such a change needs to be done. Prefactoring and premature optimization can be a major time sink. Perhaps simple interface extraction/polymorphism maybe good enough. Use factories when you want to delegate object creation to avoid coupling. – PhD Mar 1 at 16:30
  • 4
    Not sure what DI framework you are using, but most of them support collection injection. You will be able introduce dependency as a collection of interface type and register all implementations to DI. Then when new validation created you will need only register it to DI – Fabio Mar 1 at 18:48
  • 2
    Currently each validation is a function of the validation class, @Laiv, I would say injecting a collection of validations instead having them in one class will make maintenance simpler because you will be able to change/add/remove different validations without touching rest of the code. – Fabio Mar 1 at 22:34
  • 1
    I get the impression (and I might be wrong) that the direction you are considering now is going to amount to replacing direct function calls with calls to the DI container used as a service locator, followed by a method call on the returned object - and that's not necessarily that much better. – Filip Milovanović Mar 2 at 11:36
  • 2
    My suggestion is to take a step back and stop thinking about DI containers, service locators and factories, and think about where and how do you use validation currently, and what are the distinct "pieces of logic" (concerns) involved. How do you decide which validation functions to call (so decision/selection logic)? Maybe you'll need to separate that out too. Which peaces of code invoke these functions (whose dependency is this)? (BTW, update the Q with this info.) Keep the big picture in mind, but think in smaller steps - don't jump to a complete overhaul in design immediately. – Filip Milovanović Mar 2 at 11:37
1

Asking for a collection of all implementations of one interface can be an effective pattern.

When your interface has a natural composite implementation, this is a good way to allow extension points. I think of this as a "plugin" approach: anyone can write a new implementation of your interface and its behavior gets naturally mixed in with all the other implementations.

A "natural composite" is a valid implementation of your interface that delegates to its component implementations and combines their results in a way that is natural to its purpose. Your case is straightforward: to implement Validate in the composite, just call Validate on all components and combine the results with and.

Named dependencies and factories are not the right way to handle this.

Yes, you are abusing your IOC container. The principle of DI is to ask for what you need. You need an IEnumerable<IValidationType> (an IList is inappropriate here because you should not be modifying it and don't need random access). But instead of just asking for an IEnumerable<IValidationType>, you're asking for two different implementations. As you identified, this violates open/closed because your factory will change every time you come up with a new implementation.

I also don't understand the point of your factory. It seems to have 0 responsibilities. Instead of the factory, write the composite.

public class CompositeValidationType : IValidationType
{
    public CompositeValidationType(IEnumerable<IValidationType> components) { ... }
    public bool Validate() { return components.All(o => o.Validate()); }
}

Then get your IOC container to register this composite with the right component implementations as the canonical IValidationType. Reflection can be your friend here.

I am vehemently against ever using named dependencies; it breaks the sensible rule that renaming a method parameter should always be a safe refactor.

0

Use a single factory

I would use a single factory for all validators. The single factory can in turn expose methods for retrieving validators that are appropriate, which can be called by the class that receives the injection, instead of taking the dependency directly. E.g. instead of this

public MyClass(IInjected injected)  //Constructor
{
    _dependency = injected;
}

...follow this pattern:

public MyClass(IInjected injected) //Constructor
{
    _dependency = injected.RetrieveDependency(arguments);
}

How to retrieve the right type

The One Factory can retrieve all of the validators from the IoC container by resolving an IEnumerable<IValidationType>. Once it has the list, it can fulfill requests to return validators by selecting from the master list, using logic that you have complete control over.

The factory can expose different methods for finding the right validators. Here are three common patterns:

  • The way you do it now, e.g. RetrieveValidations("Validation1")

  • Passing a type argument, e.g. RetrieveValidations<ValidationType1>()

  • Passing some item that the type system knows is associated with the item. E.g. a validator for text might use RetrieveValidationsFor<string>().

I recommend the last method since it will catch many errors at compile time that would not be caught with the other two techniques.

Extending the type system to mark which validators are correct

To avoid passing around magic strings to retrieve named registrations, you can use the type system to indicate which validators go with which types. In this example I have three validators that are good for strings, ints, and DateTimes; you can follow this pattern to support objects that your business is interested in.

public interface IValidationType
{
    bool Validate();
}

public interface IValidatorFor<T> : IValidationType
{
}

public class ValidationType1 : IValidatorFor<string>, IValidationType
{
    public bool Validate() { return true; }
}

public class ValidationType2 :IValidatorFor<int>,  IValidationType
{
    public bool Validate() { return true; }
}

public class ValidationType3 :IValidatorFor<DateTime>,  IValidationType
{
    public bool Validate() { return true; }
}

Examples

Here is an example factory, where I show the three different ways of passing the type information to get the right validator:

public class ValidationFactory : IValidationFactory
{
    private readonly IList<IValidationType> _validations;

    public ValidationFactory(ILifetimeScope container)
    {
        _validations = container.Resolve<IEnumerable<IValidationType>>().ToList();
    }

    public IEnumerable<T> RetrieveValidations<T>() where T : IValidationType
    {
        return _validations.OfType<T>();
    }

    public IEnumerable<IValidationType> RetrieveValidations(string validationName) 
    {
        return _validations.Where( v => v.GetType().FullName.Contains(validationName) );  //or some other logic depending on your problem domain
    }

    public IEnumerable<IValidatorFor<T>> RetrieveValidationsFor<T>()
    {
        return _validations.OfType<IValidatorFor<T>>();
    }
}

And some test classes to show you how the injection works:

public class Example1
{
    protected readonly IEnumerable<IValidationType> _validations;

    public Example1(IValidationFactory factory)
    {
        _validations = factory.RetrieveValidations<ValidationType1>().ToList();
    }

    public void Run()
    {
        Console.WriteLine("Example validators to be used:");
        foreach (var f in _validations)
        {
            Console.WriteLine("{0}", f.GetType().FullName);
        }
    }
}

public class Example2
{
    protected readonly IEnumerable<IValidationType> _validations;

    public Example2(IValidationFactory factory)
    {
        _validations = factory.RetrieveValidations("2").ToList();
    }

    public void Run()
    {
        Console.WriteLine("Example2 validators to be used:");
        foreach (var f in _validations)
        {
            Console.WriteLine("{0}", f.GetType().FullName);
        }
    }
}

public class Example3
{
    protected readonly IEnumerable<IValidationType> _validations;

    public Example3(IValidationFactory factory)
    {
        _validations = factory.RetrieveValidationsFor<DateTime>().ToList();
    }

    public void Run()
    {
        Console.WriteLine("Example3 validators to be used:");
        foreach (var f in _validations)
        {
            Console.WriteLine("{0}", f.GetType().FullName);
        }
    }
}

Here is a small test program:

public class Application
{
    protected readonly Example1 _example1;
    protected readonly Example2 _example2;
    protected readonly Example3 _example3;

    public Application(Example1 example1, Example2 example2, Example3 example3)
    {
        _example1 = example1;
        _example2 = example2;
        _example3 = example3;
    }

    public void Run()
    {
        _example1.Run();
        _example2.Run();
        _example3.Run();
    }
}

public class Program
{

    public static IContainer CompositionRoot()
    {
        var builder = new ContainerBuilder();
        builder.RegisterType<ValidationType1>().As<IValidationType>();
        builder.RegisterType<ValidationType2>().As<IValidationType>();
        builder.RegisterType<ValidationType3>().As<IValidationType>();
        builder.RegisterType<ValidationFactory>().As<IValidationFactory>();
        builder.RegisterType<Example1>();
        builder.RegisterType<Example2>();
        builder.RegisterType<Example3>();
        builder.RegisterType<Application>();
        return builder.Build();
    }

    public static void Main()
    {
        CompositionRoot().Resolve<Application>().Run();
    }
}

And the output:

Example validators to be used:
Example.ValidationType1
Example2 validators to be used:
Example.ValidationType2
Example3 validators to be used:
Example.ValidationType3

See the full working example on DotNetFiddle

Note: Some people might say it is "bad" for a factory to access the container. This is normally true (it leads to something which looks an awful lot like the service locator anti-pattern) but for a factory it is okay. The very purpose of the factory is to assemble the dependencies to construct objects, and as such they serve as an extension of the composition root.

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.