Published on 7/30/2019

In this part we’ll do one of my favourite things in software development. Taking code that has already been built and proven to work, and restructuring it to better fit our principles. This falls under the catch-all of refactoring

Open class surgery

In part 8 we identified that the constructor for BlogService takes too many parameters. It has become the culmination of a lot of infrastructure and abstraction. And while we did a quick refactor on the syndication code and stuffed that into a single method, it still feels like all those dependencies should be extracted out into a separate service or component. We’re not just going to dig around inside a method here, we’re going to crack open the BlogService class and transplant stuff from it to a whole new class. This includes taking some unit testing arrangement with it, and probably some asserts too.

Concept: Components

What are components and how are they different from services? The main difference is the context. A component is something that remains useful without the context of your domain. Apart from that a component works in the same way and is used in the same way as a service. You’ll typically have a main abstraction layer and develop and configure a default implementation for that abstraction. It’s a self-contained module of code that you can give to someone else and they can make use of it within their own domain.

You might have followed that the syndication code we’ve written so far doesn’t function without knowledge of the blog model (we set it as a property on the IBlogSyndication abstraction). It has that coupling to the domain model. In order to make it into a full component, we would have to spend additional time to decouple it from that by building an adaptor for it. I’m not going to do that now. I’m going to incur that technical debt now for the sake of simplicity.

Concept: Technical Debt

There are many definitions and forms of technical debt. The two main sources of it are bad code and bad design (see also: accidental complexity). But it also happens a lot that technical debt is incurred conscientiously by the team (or the client) based on business pressures and project objectives. For example, if you’re building a minimum viable product as a startup perhaps there is some stuff you don’t need to be doing right now and instead focus on getting product feedback and maybe get to launch. However make no mistake, technical debt is also real debt. It is something you will have to pay for later with real money and time. And during those times you won’t have the ability to respond to other opportunities. This is also called the opportunity cost.

In this case we’re just building a stupid blog for me. I will try to minimize the debt by isolating the syndication code as much as possible, and only have it dangling by a single umbilical cord - the dependency on the Blog domain model. Everything else I will try to decouple and build as if there is a component design in place.

Cut it away by testing first

Just like a surgeon makes marks on the patient’s skin where to cut, we start our process with a unit test. We already know what our code is supposed to do, so we can easily crib from the existing Publish() unit test into a new one. We do this to ensure that we maintain the expected behavior of the code, or put another way, given a specific input our code should remain deterministic to deliver the same specific output.

public class BlogSyndicationServiceTests
{
    readonly Mock<IBlogSyndicationQueue> _blogSyndicationQueueMock = new Mock<IBlogSyndicationQueue>();
    readonly Mock<IBlogSyndicationFactory> _blogSyndicationFactoryMock = new Mock<IBlogSyndicationFactory>();
    readonly Mock<IOptionsMonitor<BlogSyndicationOptionCollection>> _blogSyndicationOptionsMock = new Mock<IOptionsMonitor<BlogSyndicationOptionCollection>>();
    readonly Mock<ILoggerFactory> _loggerFactoryMock = new Mock<ILoggerFactory>();
    readonly Mock<ILogger> _loggerMock = new Mock<ILogger>();

    public BlogSyndicationService Service => new BlogSyndicationService(_blogSyndicationQueueMock.Object, _blogSyndicationFactoryMock.Object, _blogSyndicationOptionsMock.Object, _loggerFactoryMock.Object);

    public BlogSyndicationServiceTests()
    {
        _loggerFactoryMock.Setup(x => x.CreateLogger(It.IsAny<string>())).Returns(_loggerMock.Object);
    }

    [TestMethod]
    public async Task Syndicate_Verify()
    {
        //arrange
        string title = "Hello Test!";
        Blog blog = new Blog() { Title = title };
        Mock<IBlogSyndication> syndicationMock = new Mock<IBlogSyndication>();
        _blogSyndicationOptionsMock.SetupGet(x => x.CurrentValue)
            .Returns(new BlogSyndicationOptionCollection()
            {
                new BlogSyndicationOption() { Provider = "RSS" },
                new BlogSyndicationOption() { Provider = "Twitter" }
            });
        _blogSyndicationFactoryMock.Setup(x => x.GetInstance(It.IsAny<string>()))
            .Returns(syndicationMock.Object);

        //act
        await Service.Syndicate(blog);

        //assert
        _blogSyndicationFactoryMock.Verify(x => x.GetInstance("RSS"));
        _blogSyndicationFactoryMock.Verify(x => x.GetInstance("Twitter"));
        _blogSyndicationQueueMock.Verify(x => x.Enqueue(syndicationMock.Object), Times.Exactly(2));
        syndicationMock.VerifySet(x => x.Blog = blog, Times.Exactly(2));
    }
}

All this was copied from the Publish() test, except for the test class which I created. We also need a test for the warning log.

[TestMethod]
public async Task Syndicate_NoConfig_Log()
{
    //arrange
    string title = "Hello Test!";
    Blog blog = new Blog() { Title = title };
    _blogSyndicationOptionsMock.SetupGet(x => x.CurrentValue)
        .Returns((BlogSyndicationOptionCollection)null);

    //act
    await Service.Syndicate(blog);

    //re-arrange
    _blogSyndicationOptionsMock.SetupGet(x => x.CurrentValue)
        .Returns(new BlogSyndicationOptionCollection() { });

    //act
    await Service.Syndicate(blog);

    //assert
    _loggerMock.Verify(x => x.Log(LogLevel.Warning, It.IsAny<EventId>(), It.IsAny<object>(), It.IsAny<Exception>(), It.IsAny<Func<object, Exception, string>>()), Times.Exactly(2));
}

The only real difference between the tests here and the those for the Publish() method is that I repurposed the title variable, the call to the syndication service and of course the names of the tests themselves to reflect the changed method call.

Red

Naturally none of this compiles, so we have a bit of work to do. I create a new BlogSyndicationService in the domain, and complete that to satisfy the unit test call. There is also the constructor to complete so that it can receive all the required dependencies. I copied a lot of this from the BlogService implementation.

public class BlogSyndicationService
{
    readonly IBlogSyndicationQueue _blogSyndicationQueue;
    readonly IBlogSyndicationFactory _blogSyndicationFactory;
    readonly BlogSyndicationOptionCollection _syndicationCollection;
    readonly ILogger _logger;

    public BlogSyndicationService(IBlogSyndicationQueue blogSyndicationQueue, IBlogSyndicationFactory blogSyndicationFactory, IOptionsMonitor<BlogSyndicationOptionCollection> syndicationCollectionOptions, ILoggerFactory loggerFactory)
    {
        _blogSyndicationQueue = blogSyndicationQueue;
        _blogSyndicationFactory = blogSyndicationFactory;
        _syndicationCollection = syndicationCollectionOptions.CurrentValue;
        _logger = loggerFactory?.CreateLogger<BlogSyndicationService>();
    }

    public async Task Syndicate(Blog blog)
    {
        throw new NotImplementedException();
    }
}

When we run our test we get an exception of course. Not implemented!

Green

Let’s copy the actual method implementation from the service too. That Syndicate() method that we extracted before is exactly what we need now. In theory, this code should fulfill our unit tests immediately, since the tests themselves were created from the service tests.

public async Task Syndicate(Blog blog)
{
    if (_syndicationCollection == null || _syndicationCollection.Count == 0)
    {
        _logger?.LogWarning("No syndications configured for processing");
    }
    else
    {
        _syndicationCollection.ForEach(x =>
        {
            IBlogSyndication syndication = _blogSyndicationFactory.GetInstance(x.Provider);
            syndication.Blog = blog;
            _blogSyndicationQueue.Enqueue(syndication);
        });
    }
}

In fact, I copied it verbatim and pasted it over the stub one and just changed it to be public. And this does indeed pass the two new tests.

Blue

However there is a small problem here. This new method is marked as async, but there is no await in it. This means that it will always execute synchronously (and you should see a warning about it too). We’ll deal with this later.

Now that we’ve duplicated the syndication functionality, we need to remove it from the BlogService tests and implementation. Let’s start with the tests.

Red

The first test to remove is the warning log one since it is no longer the role of the BlogService to log anything about empty syndication config. There is nothing to change here, it should be removed completely. This test-case fall away completely in this context.

The second test needs to be updated. We’re no longer interested in the queue or the factory or even the config. That is all part of the syndication service now. But we are interested in correctly calling the syndication service. Let’s replace all the verifications with a single one. To do that however we need a mock object. We can’t mock a concrete class, so we’ll have to define an interface for the syndication service.

public interface IBlogSyndicationService
{
    Task Syndicate(Blog blog);
}

In the unit test class we setup a mock for this, and then replace the verify asserts. I also remove the bits that are arranged for the factory and options.

readonly Mock<IBlogSyndicationService> _syndicationServiceMock = new Mock<IBlogSyndicationService>();


[TestMethod]
public async Task Publish_EnqueueSyndications()
{
    //arrange
    string title = "hello_test";
    Blog blog = new Blog() { Title = "Hello Test!" };
    _dbAdaptorMock.Setup(x => x.Read(title))
        .ReturnsAsync(blog);

    //act
    await Service.Publish(title);

    //assert
    _syndicationServiceMock.Verify(x => x.Syndicate(blog));
}

If you run the test now it won’t pass.

Green

In order to complete the loop, we need to pass this new mock into the BlogService class so that we can use it. That means we add yet another constructor parameter.

readonly IBlogDatabaseAdaptor _dbAdaptor;
readonly IBlogSyndicationQueue _blogSyndicationQueue;
readonly IBlogSyndicationFactory _blogSyndicationFactory;
readonly BlogSyndicationOptionCollection _syndicationCollection;
readonly IBlogSyndicationService _blogSyndicationService;
readonly ILogger _logger;

public BlogService(
    IBlogDatabaseAdaptor dbAdaptor, 
    IBlogSyndicationQueue blogSyndicationQueue, 
    IBlogSyndicationFactory blogSyndicationFactory,
    IOptionsMonitor<BlogSyndicationOptionCollection> syndicationCollectionOptions,
    IBlogSyndicationService blogSyndicationService, 
    ILoggerFactory loggerFactory)
{
    _dbAdaptor = dbAdaptor;
    _blogSyndicationQueue = blogSyndicationQueue;
    _blogSyndicationFactory = blogSyndicationFactory;
    _syndicationCollection = syndicationCollectionOptions.CurrentValue;
    _blogSyndicationService = blogSyndicationService;
    _logger = loggerFactory?.CreateLogger<BlogService>();
}

And of course we change the method implementation to actually use this new service.

public async Task Publish(string title)
{
    var blog = await _dbAdaptor.Read(title);

    blog.IsPublished = true;

    Validate(blog);

    //TODO save

    await _blogSyndicationService.Syndicate(blog);
}

Blue

Our tests now pass, but we’re left with a lot of scar tissue to cleanup and remove. The private method Syndicate() that we extracted previously inside the BlogService class is no longer being used. After removing it a lot of the privately declared services are no longer being referenced either. We remove those and start seeing errors in the constructor where these services were being assigned. We remove those. And then we’re left with a bunch of constructor parameters that aren’t being used. We remove them too. We’re left with two parameters now (the adaptor and the new syndication service abstraction). We’re starting to see errors in the unit test class now. The constructor doesn’t take all these parameters being passed to it. We remove these mock objects too. Our unit tests confirm that everything is still working as it should with all this surgery no completed.

Conclusion

In this part we embarked on a major refactoring and restructuring of our code. We saw how we made a decision to exchange an ideal and absolutely abstracted and reusable implementation for simplicity and less development time. We saw how it helps to first establish your target unit tests first and then start the refactoring process to help you make sure things are working. And we saw how removing code after changing the tests helps us to know that the code still works.