Home .Net [Test Refactoring Part 1] Introducing The Problem Code

[Test Refactoring Part 1] Introducing The Problem Code

by Trent
0 comments

Welcome to the first post in the Dot Net Core Unit Testing series! In this post we will take a look at the first form of our spaghetti code and highlight all of the reasons it is difficult to test.

As we progress through the series we will incrementally refactor the code and write unit tests to provide more clarity about its functionality. Each revision will be in a separate project in the Visual Studio solution so that you can see the changes over time. Follow along with the code in GitHub, which is accessible via our Code Sloth Code Samples page.

Getting back into .Net? Check out our post on getting started with .Net 6 here.

The Business Problem and Problem for the Business

The Code Sloth blog has grown exponentially over the years. So much so that we have even reached our dream of putting a sloth on the moon. Can you believe it?

In fact, the blog has become so successful that we have commercialised sloth space travel for all! Avid Code Slothians are able to request that we launch a designated number of sloths, on a certain type of Rocket ship to the moon.

Our genius system has evolved over the years to not only calculate the thrust that each rocket will require for the number of passengers on board, but also formulate a set menu that the sloths can enjoy once they reach their destination. On top of this (if you could believe the solution could get any better) our system also calculates the optimal location on the moon for our furry friends to enjoy a picnic meal, based on the number of guests and their allocated menu.

Unfortunately for the business, the incremental code changes that were added to support this awesome functionality have made the code impossible to test. When the local authorities found that we were launching sloths into space with untested code, we were asked to fix this pronto!

Tackling all of the refactoring at once isn’t an option, as the team have other tight deadlines to work towards. Therefore it is important to understand how our refactoring work can be broken down into manageable phases over time to mitigate the risk of a big change and allow us to make incremental improvements alongside our other projects which are underway.

Logical Flow of Code

At first glance the logic to place sloths on the moon is quite simple (thanks to our fleet of rocket ships having state of the art auto pilot navigation systems). This means that our code is responsible for:

  • Polling a queue to determine if a Code Slothian engineer has requested a launch
  • Looking up the specs of the requested rocket ship from the database
  • Using these specs and the requested number of sloths to calculate the appropriate thrust
  • Formulating the menu for the flight. If there are any issues in this step, we fall back to a pre-defined set menu for smaller groups, but terminate early if a large group is scheduled for departure
  • An ideal location is then calculated based on the number of sloths and their menu
  • The sloths are then launched into space with a web API call to their rocket ship, providing all of the flight data for their new adventure. If for some reason this attempt fails, we try to reprocess the message from the queue again

Our Primary Class Under Test: RocketLauncher

RocketLauncher.cs contains our class that we would like to cover with tests. Let’s take a look.

using BadCodeToBeJudged.BusinessLogic;
using BadCodeToBeJudged.Database;
using BadCodeToBeJudged.Infrastructure;
using BadCodeToBeJudged.WebApi;
using Microsoft.Extensions.Logging;
using System.Text.Json;

namespace BadCodeToBeJudged
{
    /// <summary>
    /// The Code Sloth rocket launching station is a very busy department. 
    /// User's signal that they would like to launch one or more sloths into space by posting a message to a launch queue, 
    /// with information about the designated rocket's model to launch and how many sloths should be on board.
    /// 
    /// This class has grown in complexity over time, as the launch process has become more complex. A recent push for automated
    /// testing sees an engineer tasked with writing unit tests for the RocketLauncher.
    /// </summary>
    internal class RocketLauncher
    {
        private ThrustCalculator thrustCalculator;
        private RocketDatabaseRetriever rocketDatabaseRetriever;
        private RocketQueuePoller rocketQueuePoller;
        private RocketLaunchingService rocketLaunchingService;
        private ILogger<RocketLauncher> logger;

        public RocketLauncher(
            ThrustCalculator thrustCalculator, 
            RocketDatabaseRetriever rocketDatabaseRetriever, 
            RocketQueuePoller rocketQueuePoller, 
            RocketLaunchingService rocketLaunchingService, 
            ILogger<RocketLauncher> logger
        )
        {
            this.thrustCalculator = thrustCalculator ?? throw new ArgumentNullException(nameof(thrustCalculator));
            this.rocketDatabaseRetriever = rocketDatabaseRetriever ?? throw new ArgumentNullException(nameof(rocketDatabaseRetriever));
            this.rocketQueuePoller = rocketQueuePoller ?? throw new ArgumentNullException(nameof(rocketQueuePoller));
            this.rocketLaunchingService = rocketLaunchingService ?? throw new ArgumentNullException(nameof(rocketLaunchingService));
            this.logger = logger ?? throw new ArgumentNullException(nameof(logger));
        }

        public async void LaunchRocketsToTheMoon(CancellationToken cancellationToken)
        {
            while (true)
            {
                var rocketLaunchMessage = await rocketQueuePoller.PollForRocketNeedingLaunch();
                if (rocketLaunchMessage == null)
                {
                    await Task.Delay(TimeSpan.FromSeconds(5));
                    continue;
                }

                try
                {
                    var databaseResult = await rocketDatabaseRetriever.FindRocketStatistics(rocketLaunchMessage.RocketModelId);
                    var requiredThrust = thrustCalculator.CalculateThrust(
                        databaseResult.ThrustValue1,
                        databaseResult.ThrustValue2,
                        databaseResult.ThrustValue3,
                        databaseResult.ThrustValue4,
                        databaseResult.ThrustValue5,
                        rocketLaunchMessage.numberOfSlothsToLaunch
                    );

                    var preferredFoodForJourney = await rocketDatabaseRetriever.GetFoodToFeedSlothsOnTheirJourney(rocketLaunchMessage.RocketModelId, rocketLaunchMessage.numberOfSlothsToLaunch);
                    if (preferredFoodForJourney == null)
                    {
                        if (rocketLaunchMessage.numberOfSlothsToLaunch < 10)
                        {
                            preferredFoodForJourney = new PreferredFoodForJourney("only slightly toxic leaves");
                        }
                        else
                        {
                            throw new Exception("We can't cater a trip for 10 or more sloths without carefully planned food for space travel!");
                        }
                    }

                    var coordinatesToLandOnMoon = StaticCoordinateCalculator.CalculateCoordinatesToLand(rocketLaunchMessage.numberOfSlothsToLaunch, preferredFoodForJourney.typeOfFoodToFeedSloths);

                    var rocketLaunchResult = await rocketLaunchingService.LaunchRocket(rocketLaunchMessage.RocketModelId, rocketLaunchMessage.numberOfSlothsToLaunch, requiredThrust, preferredFoodForJourney.typeOfFoodToFeedSloths);

                    if (rocketLaunchResult.launchWasSuccessful)
                    {
                        await rocketQueuePoller.RemoveMessageFromQueue(rocketLaunchMessage.messageId);
                    } else
                    {
                        throw new Exception($"Failed to launch rocket {rocketLaunchMessage.RocketModelId} with {rocketLaunchMessage.numberOfSlothsToLaunch} on board. Request id: {rocketLaunchMessage.messageId}. Retrying shortly.");
                    }
                }
                catch (Exception ex)
                {
                    logger.LogError($"Error caught while processing message {JsonSerializer.Serialize(rocketLaunchMessage)}. Error: {ex.Message} ");
                }
            }
        }
    }
}

In the grand scheme of bad code, this class is quite small. Unfortunately its current shape means that cannot unit test it. At all.

In the spirit of Sloth Code simplicity the example code does not deal with the additional (unnecessary) complexity of real infrastructure, such as using database clients, or go through the process of instantiating real httpclients for making web requests. Instead, our code just pretends to query a database or API, as can be seen in PretendDatabaseClient.cs:

namespace BadCodeToBeJudged.Database
{
    // This class is a pretend database client, likely authored by an external party
    internal class PretendDatabaseClient
    {
        private string connectionString;
        private TimeSpan connectionTimeout;
        private TimeSpan queryTimeout;
        private int connectionRetries;
        private int queryRetries;

        private readonly Random random = new Random();

        public PretendDatabaseClient(string connectionString, TimeSpan connectionTimeout, TimeSpan queryTimeout, int connectionRetries, int queryRetries)
        {
            this.connectionString = connectionString ?? throw new ArgumentNullException(nameof(connectionString));
            this.connectionTimeout = connectionTimeout;
            this.queryTimeout = queryTimeout;
            this.connectionRetries = connectionRetries;
            this.queryRetries = queryRetries;
        }

        private static string[] foods = new[] { "sushi", "pasta", "only slightly toxic leaves" };

        public async Task<PreferredFoodForJourney> FindFood(string query)
        {
            // Pretend to take time making a db query
            await Task.Delay(TimeSpan.FromMilliseconds(500));
            
            // Return a random result. This demonstrates that we don't have any control over our data source
            // and indicates that unit testing this component is not possible, as the output is not deterministic
            var randomFoodIndex = random.NextInt64(0, foods.Length - 1);
            return new PreferredFoodForJourney(foods[randomFoodIndex]);
        }

        public async Task<RocketThrustStatistics> FindRocketStatistics(string query)
        {
            // Pretend to take time making a db query
            await Task.Delay(TimeSpan.FromMilliseconds(500));
            
            // Return a random result. This demonstrates that we don't have any control over our data source
            // and indicates that unit testing this component is not possible, as the output is not deterministic
            return new RocketThrustStatistics(
                random.Next(),
                random.Next(),
                random.Next(),
                random.Next(),
                random.Next()
            );
        }
    }
}

This class pretends to take some time making a database call and then returns some randomised data. The randomness of this data makes it particularly hard to test; a topic for another blog post!

With that in mind let’s launch into our top blockers for being able to write simple unit tests!

Infinite or Sticky Loops

Some things just aren’t meant to last forever. The orchestration logic of our code is one of those things.

Programming infinite loops in your code will make make writing unit tests practically impossible. I say practically impossible, because there may be the potential for exception handling (or lack thereof) to break you out of the loop in an error flow. We’re aiming for comprehensive test coverage though, so testing a single error case doesn’t help us very much.

public async Task LaunchRocketsToTheMoon()
{
	while (true)
	{
		...
	}
}

Sticky loops might not last forever, but present a different type of challenge altogether.

Concrete Classes

When you first learn about object oriented programming, you learn about the magic of polymorphism. This amazing tool lets you unlock a world of flexibility in your programming! One abstraction could literally be any number of different concrete implementations at runtime. How awesome is that?

internal interface ISloth
{
   ... Some awesome things that sloths can do
}

internal class RegularSloth : ISloth
{
    ... The way a regular sloth sloths about
}

internal class RocketShipSloth : ISloth
{
    ... The way a rocket ship trained sloth sloths about
}

The reality of polymorphism for most business applications however, is that one abstraction will resolve to one concrete implementation at runtime. While this a little less awe inspiring than being able to produce a near-infinite number of specialty sloths this code is still very important.

Unfortunately our rocket launching code knows very little of interfaces:

internal class RocketLauncher
{
    private ThrustCalculator thrustCalculator;
    private RocketDatabaseRetriever rocketDatabaseRetriever;
    private RocketQueuePoller rocketQueuePoller;
    private RocketLaunchingService rocketLaunchingService;
    private ILogger<RocketLauncher> logger;
	
    ... etc ...
}

Being non-committal should be a programmer’s primary personality trait (when coding that is!) and this code has clingy written all over it. If we don’t commit to a specific implementation of code at runtime we will find programming unit tests much easier at test time.

Time Providers

A big mistake that can hinder the unit testability of your code is programming against concrete time providers.

Every time that you call DateTime.Now you make your code more difficult to test, if that time contributes to an assertable output from your function. This is because time waits for nobody and the time between DateTime.Now and your assertion logic lacks determinism and therefore cannot be asserted accurately. Furthermore calling DateTime.Now may result in your tests only passing at certain times of the day!

public int CalculateThrust(int input1, int input2, int input3, int input4, int input5, int numberOfSloths)
{
	var result = (input1 + input2 + input3 + input4 + input5) * numberOfSloths;
	if (DateTime.Now.Hour <= 12)
	{
		return result;
	}
	else
	{
		return result + 10;
	}
}

The substantially worse counterpart to DateTime.Now is forced delays in your code. They may be completely valid, such as creating space between queue polling attempts, or during backoff in a retry policy. Despite this, it is not reasonable that each unit test case actually wait for the delay period. If you have a 5 second delay in your code, your unit test should not have to wait 5 seconds when executing it!

var rocketLaunchMessage = await rocketQueuePoller.PollForRocketNeedingLaunch();
if (rocketLaunchMessage == null)
{
	await Task.Delay(TimeSpan.FromSeconds(5));
	continue;
}

Static Classes

We’ve all been there. Perhaps it’s a Friday afternoon and you’re drafting a pull request.

“Ugh, I really need a new class, but I don’t want to have to configure dependency injection. I know! I’ll make the class static and write some tests for it instead”. Can you smell the pasta cooking? Chances are that youve just added some more spaghetti code to the pot of boiling water that is your program’s codebase.

internal static class StaticCoordinateCalculator
{
    ... a whole world of testing pain awaits you ...
}

Too Many Dependencies (or Responsibilities)

A constructor with too many parameters should make you cringe; especially if theyre complex types. Not only are these dependencies going to be difficult and time consuming to prepare in a unit test, they likely mean that your class is doing too much. A class that is overloaded with functionality forces you to write a gigantic suite of unit tests to support it.

public RocketLauncher(
            ThrustCalculator thrustCalculator, 
            RocketDatabaseRetriever rocketDatabaseRetriever, 
            RocketQueuePoller rocketQueuePoller, 
            RocketLaunchingService rocketLaunchingService, 
            ILogger<RocketLauncher> logger
        )

Test setup should be simple and concise. Orchestrating multiple layers of complex types, which potentially have complex type dependencies of the own is not.

Solving this problem is much more subjective than some of the above issues and can take a bit of practice. However, there are some tricks to brute force simplification in the short term, if logical boundaries or patterns cannot be established.

Non-Functional Code

This does not mean code that does not work. It means that the code does not follow functional design principles and produce assertable outputs.

 public async Task<FoodForJourney> GetFoodToFeedSlothsOnTheirJourney(int id, int numberOfSloths)
{
	try
	{
		return await client.FindFood($"select * from foodTable where rocketId = {id} and numberOfSlothsToCaterFor = {numberOfSloths}");
	}
	catch(Exception ex)
	{
		logger.LogError($"Exception caught while determining food to feed sloths. {ex.Message}.");
		
		// Red flag
		return new FoodForJourney();
	}
}

This often comes in the form of not carefully thinking about the return value (or lack thereof using null) or type of a function. In the example above we return a default initialised FoodForJourney during the error flow. What does this mean for the consumer of this code?

Sloth Summary

We’ve covered a lot of ground in this article. By now you should have an understanding of the logical flow of the application, and have pulled down the GitHub repository via our Code Samples page.

Takeaway: Dot Net Core Unit Testing Strategies

Over the coming articles we will address each of the concerns above as we explore how we can easily write unit tests for sloth space travel:

  • Infinite loops
  • Time providers
  • Concrete classes
  • Static classes
  • Too many dependencies (or responsibilities)
  • Non functional code

Dot Net Core Unit Testing Series

Check out the entire series below!

You may also like