Static methods and testability

Question

I have an application service which returns entity by its registration number

public Entity FindByRegNumber(string number)
{
    if (!RegNumber.IsValid(number))
    {
        return null;
    }

    var regNumber = new RegNumber(regNumber);
    return repository.FindBy(regNumber);
}

Here RegNumber is a value object and it has static method IsValid which verifies that string correctly represents registration number. This method is used inside constructor as well, but, if string is not valid, constructor throws InvalidOperationException.

If IsValid method belonged to some injected dependency, I could just check in my test that IsValid was called (a kind of white box testing). Now I have to test FindByRegNumber repeating all the input used for testing IsValid method (black box testing).

It looks like the only reason to get rid of static method is to make testing easier. It does not even decreases coupling while call to FullRegistrationNumber constructor stays.

Another approach is to introduce a factory FullRegistrationNumberCreator' which could abstract away both validation and construction ofFullRegistrationNumber`, but it seems like an overkill and overengineering.

Is here a correct solution?

Why don't you just let the constructor make the validation of the passed ID, and make assertation NUnit style like: `Assert.DoesNotThrow(() => yourClass.FindByRegNumber('123'))` ? Also you use [tag:domain-driven-design], so I recommend revisiting your ubiqitous language with your business experts to see if `FindByRegNumber` is a business choice for the problem domain to make it more ubiqitous like: `FindEntityByRegistrationNumber` or so. — kayess, Apr 04 '17 at 08:32
@kayess Aggree about namings. Constructor throws indeed,then I have to explicitly catch and return `null`. And In my test I need then only provide some invalid input to check this branch of execution. — Pavel Voronin, Apr 04 '17 at 08:39
@kayess Interesting [discussion](http://wiki.c2.com/?DontUseExceptionsForFlowControl) concerning throwing and catching — Pavel Voronin, Apr 04 '17 at 09:07
Possible duplicate of [What is the best practice for refactoring a static method in order to make it more testable?](http://softwareengineering.stackexchange.com/questions/12292/what-is-the-best-practice-for-refactoring-a-static-method-in-order-to-make-it-mo) — gnat, Apr 04 '17 at 09:17
see also [Is static universally “evil” for unit testing and if so why does resharper recommend it?](http://softwareengineering.stackexchange.com/q/5757/31260) — gnat, Apr 04 '17 at 09:17
@gnat The question is not `how?`, but rather `should I`? It looks like constraints of the platform make me to do something excessive in order to make it testable. I see no other reason for refactoring, but testability. For example Typemock allows me to mock or check, whether static method was called, but it is also a kind of a hack. — Pavel Voronin, Apr 04 '17 at 09:27
Seems like your function should take a `RegNumber`, not a `string`. Make use of your type system, it's there exactly for this reason, to help you prevent passing bad data. If you can't construct an invalid `RegNumber`, you don't have to ensure it is valid every time it is passed to a function. — Vincent Savard, Apr 04 '17 at 12:50
@VincentSavard Well, as it is an application service and user is allowed to enter any string I cannot require `RegNumber` to be passed initially — Pavel Voronin, Apr 04 '17 at 13:39
Of course you can. You should validate that what the user entered is valid upon submission, not deep into your business logic. You shouldn't cross layers when you know you have invalid data. Separate your concerns. — Vincent Savard, Apr 04 '17 at 14:00
@VincentSavard And if it is a WCF method?;) At the very entry I convert string to valid representation. Exactly what you are talking about. — Pavel Voronin, Apr 04 '17 at 14:12

score 4 · Accepted Answer · answered Apr 04 '17 at 12:52

First thing to point out is that I disagree with the way you test validity and create instance of RegNumber.

I would design it in a way that there is only one way to create RegNumber instance and that also also includes validity check.

For example something like

public static bool RegNumber.TryParse(string number, out RegNumber regNumber)
{
  //...
}

So to create new instance of RegNumber, you have to call this method. This then introduces structural constraint, that if something uses RegNumber type, it is using valid one.

Then, I would take the pragmatic approach and say, that the code you present doesn't have any interesting logic to test. And it is enough to write two high-level integration tests. First to check with valid registration number, second with invalid registration number. No need to test whole registration invalidation again.

Nice solution. As I wrote, validation is made inside constructor, but it is not so explicit comparing to your approach. Though I wish we used VS2017 with C#7 tuples support. — Pavel Voronin, Apr 04 '17 at 13:33

score 2 · Answer 2 · answered Apr 04 '17 at 08:39

I'd extract the concept of a RegNumberValidator into a class of its own.

Its instance could have a virtual bool IsValid(string) method, such that you can overwrite it for testing - and also for derived classes requiring additional validation steps.

If you prefer a static class/method, you can still use a trick described by M. Feathers in his "Working with Legacy Code", e.g.

public static class RegNumberValidator
{
    private IRegNumberValidator _instance = new NonstaticRegNumberValidator();

    public void SetInstanceForTesting(IRegNumberValidator testInstance)
    {
        _instance = testInstance;
    }

    public bool IsValid(string regNumber)
    {
        return(_instance.IsValid(regNumber));
    }
}

Note: some people think that this is a hack which should be applied in the cases of legacy code only - because there you can't get ahead without hacks. In my opinion, it is a valid pattern.

What makes you think it worths separating the validation from the business logic it protects? I tend to see that this approach is just usually frittering the cohesion and the encapsulation away. Otherwise it's not a bad idea if you really work with locked down types or so :) — kayess, Apr 04 '17 at 08:46
@kayess Cohesion could be an issue, but here the validation method just checks a string somehow, not the internal state of an instance (in other words: it does not access the internal state of the instance). Hence I think in this case the separation into different classes is not such a big violation of cohesion. — Bernhard Hiller, Apr 04 '17 at 09:02

Static methods and testability

2 Answers2