Published on 8/7/2019

Previously we refactored the syndication out into its own service (and placed it behind an abstraction). Before we carry on completing that service implementation, I want to move back to the actual BlogService implementation and just wrap that up.

I missed a part of the plan

I mentioned in part 8 that I noticed we never completed the Publish() method to do an actual save of the updated blog settings. I put into the code a bit of comment that read

//TODO save

It should be easy to complete this part. We already have a call to save a blog entry on the adaptor. Let’s just verify that we call that when we publish.

[TestMethod]
public async Task Publish_IsPublished_MarkedTrue()
{
    //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
    Assert.IsTrue(blog.IsPublished);
    _dbAdaptorMock.Verify(x => x.Create(blog));
}

That’s a simple addition to the unit test since the mock for the adaptor is already there, and to pass it we simply add a call to the abstraction of the adaptor in the service. I chose to put it in the IsPublished test, but in fact, I could have put it in any of the positive scenario tests. As long as it’s in at least one of them. Job done.

Naming things

But this method on the adaptor is called Create. That’s not a name that aligns well with publishing a method. The blog had been created previously. This time what we’re really doing is updating the blog. So we can create a new method on the adaptor called Update(), or we can change Create() to instead read Save(). This type of change is typically team and project-specific and will mostly be dictated by existing patterns and conventions already in the code. You can argue that you want the code to reflect the CRUD acronym to prevent ambiguity for instance, and that’s a very valid argument. You can also argue that Create(Blog) and Update(Blog) has the exact same signature, and so there’s no point in duplicating API on the adaptor’s abstraction. But it is important that you are consistent within the scope of the project.

I’m going to change the name to read Save() and move on.

Warnings

So far Visual Studio is reporting one warning. I like to deal with these early so that the trivial ones don’t create a backlog that makes it difficult to see and find the complicated ones. This is also part of keeping your code clean. For example, if a package that you depend on decides to deprecate a method that you call, and you don’t deal with it timeously, you will encounter a breaking change at some point in the future. And typically that point will be when you are forced to upgrade under pressure from environmental problems for example. At that point, you really don’t want to deal with trivialities like deprecations and unnecessary code changes that have to be tested and reviewed. And warnings can have a significant impact on your predicted timelines when presented in bulk.

For this one we just need to sort out the asynchronous flow of the new Syndicate() method on the new service we added last time. The only call that is also asynchronous in that method is the Enqueue() call, but it is inside the anonymous Action supplied to the ForEach() extension. If we async await on the lambda and the call, we’re not actually doing anything asynchronously in the Syndicate() method. There are also other problems with this code, like error propagation. This SO answer explains in more detail and provides us with a better approach to handling this using the Task API. Let’s rewrite that method a bit.

public async Task Syndicate(Blog blog)
{
    if (_syndicationCollection == null || _syndicationCollection.Count == 0)
    {
        _logger?.LogWarning("No syndications configured for processing");
    }
    else
    {
        IEnumerable<Task>  syndicationTasks = _syndicationCollection.Select(t => SyndicateItem(t, blog));
        await Task.WhenAll(syndicationTasks);
    }
}

private async Task SyndicateItem(BlogSyndicationOption option, Blog blog)
{
    IBlogSyndication syndication = _blogSyndicationFactory.GetInstance(option.Provider);
    syndication.Blog = blog;
    await _blogSyndicationQueue.Enqueue(syndication);
}

We provide a private asynchronous method which we use to project each syndication item into a Task, and then we await all the tasks. An alternative (and more old school implementation) would have been to change the code to do a traditional foreach loop.

foreach (var option in _syndicationCollection)
{
    …
    await _blogSyndicationQueue.Enqueue(syndication);
}

Again, decide within your team which approach works best for you and your mates.

Conclusion

This is a short part, but I wanted to illustrate how dealing with TODOs and Warnings early helps to save you time later. Whether you conscientiously plan to deal with it as part of your delivery commitment, or whether you slide it in as part of other work doesn’t really matter, but make it part of the culture of your team and your personal workflow to always pay attention to these code smells and improvements.