Overriding methods in classes-dependencies

April 9, 2018

I’m continuing my unit testing anti-patterns article series. Today, we will talk about overriding methods in classes-dependencies.

Overriding methods in classes-dependencies

This one is not as straightforward as the previous anti-patterns, and you might even not consider it as such. Which is fine, I too employed it a lot (and was quite happy about it) in the past. But let’s not put the cart before the horse.

So, what is it about?

Let’s take an example. Let’s say that you’ve got a class, StatisticsCalculator:

public class StatisticsCalculator
{
    public async Task<(double weightDelivered, double totalCost)> Calculate(int customerId)
    {
        List<DeliveryRecord> records = await GetDeliveries(customerId);

        double weightDelivered = records.Sum(x => x.Weight);
        double totalCost = records.Sum(x => x.Cost);

        return (weightDelivered, totalCost);
    }

    public async Task<List<DeliveryRecord>> GetDeliveries(int id)
    {
        var client = new HttpClient();
        string json = await client.GetStringAsync("http://external.provider.com/api/deliveries?customerId=" + id);
        var result = JsonConvert.DeserializeObject<List<DeliveryRecord>>(json);
        return result;
    }
}

Its main function is to calculate some statistics: the weight of all deliveries sent to a particular customer and the total cost of all those deliveries.

As you can see above, to perform such a calculation, the class retrieves delivery records from an external provider first. For that, it uses the provider’s REST API (GetDeliveries method).

Let’s also say that you’ve got another class, CustomerController, that uses StatisticsCalculator as one of its dependencies:

public class CustomerController
{
    private readonly StatisticsCalculator _calculator;

    public CustomerController(StatisticsCalculator calculator)
    {
        _calculator = calculator;
    }

    public async Task<string> GetStatistics(int customerId)
    {
        (double weightDelivered, double totalCost) = await _calculator.Calculate(customerId);

        return $"Total weight delivered: {weightDelivered}. Total cost: {totalCost}";
    }
}

How would you go about testing the controller’s GetStatistics method?

You don’t want to do that as is, i.e. with the StatisticsCalculator class as a dependency, because that class refers to the external API. This external API requires some complex machinery to work properly, so it’s gonna be more work for you to set up such a test. But more importantly, it’s a volatile dependency, and you don’t want your tests to work with it directly.

So it’s a good idea to substitute this dependency with a test double, such as stub.

But at the same time, you don’t want to replace StatisticsCalculator completely either. It contains important business functionality - the calculation logic itself - and you want it to be covered with unit tests.

So, what do you do?

One way to overcome this dilemma is to make the GetDeliveries method virtual and override it with a mock, like this:

[Fact]
public async Task No_delivery_records_form_correct_result()
{
    // Arrange
    var mock = new Mock<StatisticsCalculator> { CallBase = true };
    mock.Setup(x => x.GetDeliveries(It.IsAny<int>()))
        .Returns(Task.FromResult(new List<DeliveryRecord>()));
    var controller = new CustomerController(mock.Object);

    // Act
    string result = await controller.GetStatistics(1);

    // Assert
    Assert.Equal("Total weight delivered: 0. Total cost: 0", result);
}

With this approach, you are able to substitute the volatile part of the class with a test double while keeping the rest intact.

This line does the job for us:

new Mock<StatisticsCalculator> { CallBase = true }; // Preserves the base class behavior

It allows the mock to preserve the base class' behavior for the methods that you don’t override in the test.

Alright, as you have guessed already, this is another anti-pattern. Can you guess why? Give it a minute before reading next, it’s an interesting and fun exercise.

You probably think that it’s because this test uses a mock. This is a valid suggestion but it’s not that. The use of mocking here is actually justified because the mock acts as a stub in this test. Which is pretty much the only valid use case for mocks.

The anti-pattern here is in the way the test isolates the external dependency. The fact that you need to substitute not an entire class but only a part of it is a strong sign that this class violates the single responsibility principle.

And indeed, if you look closely at the StatisticsCalculator class, it combines two distinct responsibilities: communicating with the external API and processing the results it gets from it. Here it is once again:

public class StatisticsCalculator
{
    public async Task<(double weightDelivered, double totalCost)> Calculate(int customerId)
    {
        List<DeliveryRecord> records = await GetDeliveries(customerId);

        double weightDelivered = records.Sum(x => x.Weight);
        double totalCost = records.Sum(x => x.Cost);

        return (weightDelivered, totalCost);
    }

    public async Task<List<DeliveryRecord>> GetDeliveries(int id)
    {
        var client = new HttpClient();
        string json = await client.GetStringAsync("http://external.provider.com/api/deliveries?customerId=" + id);
        var result = JsonConvert.DeserializeObject<List<DeliveryRecord>>(json);
        return result;
    }
}

The Calculate method here is where the domain logic resides. GetDeliveries just gathers the required inputs for that logic.

The proper approach would be to introduce a separate gateway class that would fetch data from the 3rd party API and then feed that data into the calculator to perform the calculations. So, in other words, split StatisticsCalculator into two classes each of which would be responsible for its own piece of functionality.

Here’s how it would look after the refactoring:

public class DeliveryGateway : IDeliveryGateway
{
    public async Task<List<DeliveryRecord>> GetDeliveries(int customerId)
    {
        var client = new HttpClient();
        string json = await client.GetStringAsync("http://external.provider.com/api/deliveries?customerId=" + customerId);
        var result = JsonConvert.DeserializeObject<List<DeliveryRecord>>(json);
        return result;
    }
}

public class StatisticsCalculator
{
    public (double weightDelivered, double totalCost) Calculate(List<DeliveryRecord> records)
    {
        double weightDelivered = records.Sum(x => x.Weight);
        double totalCost = records.Sum(x => x.Cost);

        return (weightDelivered, totalCost);
    }
}

Now, DeliveryGateway is what we can substitute in tests while StatisticsCalculator can be left as is because it doesn’t touch any external dependencies. StatisticsCalculator is purely functional.

And here’s the controller class after the refactoring:

public class CustomerController
{
    private readonly StatisticsCalculator _calculator;
    private readonly IDeliveryGateway _gateway;

    public CustomerController(StatisticsCalculator calculator, IDeliveryGateway gateway)
    {
        _calculator = calculator;
        _gateway = gateway;
    }

    public async Task<string> GetStatistics(int customerId)
    {
        List<DeliveryRecord> records = await _gateway.GetDeliveries(customerId);
        (double weightDelivered, double totalCost) = _calculator.Calculate(records);

        return $"Total weight delivered: {weightDelivered}. Total cost: {totalCost}";
    }
}

In the GetStatistics method, it is clear now that there are two separate actions involved: getting deliveries and calculating the statistics. Only one of these actions requires an external dependency, that’s why we introduced an interface for only one of them.

So, don’t substitute parts of classes with test doubles, that’s a strong sign there’s a bigger problem involved: a violation of the single responsibility principle. Instead, isolate the volatile bits. Extract them into a separate class, and substitute that class entirely.

Note that the solution we came up with was to untangle communicating pieces of code - the gateway and the calculator - and introduce a mediator between them - the controller. Although, the controller was already in place, and so we used this existing class as the mediator.

This is a common theme across several unit testing (and not only unit testing) best practices. And it was the solution for one of the previous anti-patterns too.

And that is: try to represent the program flow as a sequence of steps with clear inputs and outputs. This simple principle is the cornerstone of functional programming. And the best part is that you don’t need to use a functional language to do that, you can adhere to this principle with any programming language.

Summary

Classes that combine business logic and reaching out to external services violate the single responsibility principle.
Instead of substituting the volatile bits of such classes in tests, extract those bits into a separate class and substitute it entirely.

If you enjoy this article, check out my Pragmatic Unit Testing training course, you will love it too.