0

Given the following class in a Legacy code base without any UT's. So any refactoring done should be done on the smallest possible scale, just in order to be able to write UT's.

public class Person
{
    private readonly PersonValidator personValidator;

    public Person()
    {
        this.personValidator = new PersonValidator(this);
    }
}

public class PersonValidator
{
    public PersonValidator(Person person)
    {
        // Logic here.
    }
}

Please note that this is very simplistic. Should I create an interface of the PersonValidator class and use factory design pattern to create this one? Note that it cannot be injected since it require this in it's constructor.

Basically, the code would become something like that:

public interface IPersonValidator 
{ }

public interface IPersonValidatorFactory
{
    IPersonValidator CreatePersonValidator(Person person);
}

public sealed class PersonValidatorFactory : IPersonValidatorFactory
{
    public IPersonValidator CreatePersonValidator(Person person)
    {
        return new PersonValidator(person);
    }
}

public class PersonValidator : IPersonValidator
{
    public PersonValidator(Person person)
    {
        // Logic here.
    }
}

public class Person
{
    private readonly IPersonValidator personValidator;

    public Person(IPersonValidatorFactory personValidatorFactory)
    {
        this.personValidator = personValidatorFactory.CreatePersonValidator(this);
    }
}

By using this approach, I get rid of the hard wired dependency on the 'PersonValidator'. And since I'm using an interface that represents a factory, I would be able to mock it and return any type that I like. So my unit test would not depend on any other concrete types.

Is this the preferred approach or am I missing something?

0
1

Is this the preferred approach

Well, it is an approach. But before you go that route, always consider if this is really worth the extra effort.

Without the factory, one can still

  • test the PersonValidator class in isolation

  • test the Person class (not isolated from the PersonValidator, of course)

So when do you want to be able to test Person in isolation? This makes mostly sense when

  • testing it with a real PersonValidator makes it hard to spot the root cause of a failing test, because Person has some complex logic apart from PersonValidator which should be tested without the latter

  • having a "real" PersonValidator results in performance problems in the unit tests for Person

If none of these reasons apply, using a factory looks pretty overengineered to me. Decoupling by factories is not an end in itself, it is a means to an end.

3
  • Thanks. It was the answer I was looking for :-) Indeed, it requires some extra effort, but if I don't use a Factory, I'm not able to test the Person class in isolation (as you mentioned), since it contains a dependency on the PersonValidator class. A UT should always strive to test a single unit and this is clearly a violation since we're testing both classes.
    – Complexity
    Mar 30 '20 at 20:33
  • 2
    @Complexity: "should always strive to test a single unit" is exactly the opposite of what my answer is actually saying - which is - do not follow this cargo cult until you have a real reason for,
    – Doc Brown
    Mar 30 '20 at 21:23
  • 1
    @Complexity "A UT should always strive to test a single unit and this is clearly a violation since we're testing both classes." <-- But a single class != a single unit. Unit != class. That is a common OOP mistake to make. Id argue that you would actually be more correct testing these two together as a unit because they seem very coupled (in a good way, not all coupling is bad, why would you want someone to even be able to create invalid persons in your domain model?).
    – wasatz
    Apr 1 '20 at 6:17
1

Should I create an interface of the PersonValidator class and use factory design pattern to create this one?

Not before you have tests in place to detect the mistakes you may make during refactoring.

In your original example, this.personValidator is an implementation detail - it's an accident of history that your class is implemented this way, not a real constraint of the behavior you are trying to capture. So you should want to leave it alone until the mistake detectors are available.

What will sometimes happen is that this "implementation detail" will have some undesirable coupling of its own - access to shared mutable state, or a network call, or something else that would make the mistake detector unstable. That's the kind of problem that you might try to address before introducing a test.

Sometimes it takes several iterations to get from "this code has NO tests" to "this code has GOOD tests".

if I don't use a Factory, I'm not able to test the Person class in isolation

That's not the first priority to address. Mistake detectors come first, then you can start modifying the design safely. When you have a new design that supports "isolated" testing, you can then implement the new tests and deprecate/retire the old ones.

2
  • Thanks for this comment.
    – Complexity
    Mar 30 '20 at 20:44
  • However, I don't see how replacing a direct instantiating with a Factory could possibly break the existing behaviour. Could you please explain this a bit more.
    – Complexity
    Mar 30 '20 at 20:45

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.