0

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?

  • 1
    Why don't you just let the constructor make the validation of the passed ID, and make assertation NUnit style like: Assert.DoesNotThrow<InvalidOperationException>(() => yourClass.FindByRegNumber('123')) ? Also you use 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 4 '17 at 8: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 4 '17 at 8:39
  • @kayess Interesting discussion concerning throwing and catching – Pavel Voronin Apr 4 '17 at 9:07
  • 1
    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 4 '17 at 12:50
4

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 4 '17 at 13:33
2

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.

  • 1
    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 4 '17 at 8:46
  • 1
    @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 4 '17 at 9:02

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.