Luke Warren Dev
Software Development Tips and Stories When I feel Like It

Your Code Sucks and Other Political Statements

There is no secret sauce or trick you haven't got around to learning to start unit testing. Your code just sucks

Your Code Sucks and Other Political Statements

OK I have got your attention. Good!

There are too many devs going around that think that unit testing was some dark art that they never learned, a waste of time or they just need some of that special sauce. Sorry folks but the reality is that unit testing is not the problem, it is your code.

With that said, do not fret! In this article I will show you a few simple tricks to get from sucky code to ... erm.. not sucky code?

Anyway, let's start with a snippet:

This code is based on a certain politician in South Africa. For non-saffas, he owed some money and needed to pay it back.

using PayBackTheMoney.Services;
using System;

namespace PayBackTheMoney
{
    public class Payer
    {
        public bool TryPayMoney(DateTime timeToPayBackTheMoney, bool canDelayProcess)
        {
            if (!canDelayProcess && DateTime.Now >= timeToPayBackTheMoney)
            {
                // Damn, have to pay
                var payer = new MoneyPayerService();
                payer.Pay(7000000M);

                // Send Thuli email
                var notifications = new NotificationService();
                notifications.Send("jacob@anc.gov.za", "thuli@madonsela.gov.za", "I pay bak moneyies.\r\n Love, Jacob");
                return true;
            }

            // "HA HA HA! "Nkandla"" HA HA HA
            return false;
        }
    }
}

Think the above code is good? It isn't. Think it is unit testable? You guessed it, it isn't!

Why? A unit test by its very definition is a method that tests a very small piece of logic in isolation. It should not rely on it's dependencies to be operational and in a certain state to be tested. Those are called integration tests.

The above is littered with concrete dependencies which makes it impossible to unit test.

Have a look again at the code. This time I have commented every issue:

using PayBackTheMoney.Services;
using System;

namespace PayBackTheMoney
{
    public class Payer
    {
        public bool TryPayMoney(DateTime timeToPayBackTheMoney, bool canDelayProcess)
        {
            // We are relying on the DateTime.Now property to return the current DateTime from the system clock
            // To test this we would need to set the clock on the PC we are using every time we want to test. What a pain!
            if (!canDelayProcess && DateTime.Now >= timeToPayBackTheMoney)
            {
                // We are using the new keyword to create an instance of our money payer service. This means that we have a concrete dependency on this class
                // Who knows what the money payer service might be dependent on to perform its "duties".
                var payer = new MoneyPayerService();
                payer.Pay(7000000M);

                // Again, as above. Newing up the NotificationService ties us to it and means that we can't test this method in isolation from its dependencies
                // Point is, we can't swap it out with a fake in our test code. We would have to make sure that an SMTP server was running!
                var notifications = new NotificationService();
                notifications.Send("jacob@anc.gov.za", "thuli@madonsela.gov.za", "I pay bak moneyies.\r\n Love, Jacob");
                return true;
            }

            return false;
        }
    }
}

As you can see, this code is using the new keyword to create services that it needs to use. This is a massive issue when it comes to unit testing because to test the Payer method you need to have both an operational NotificationService, MoneyPayerService and you would need to set your computer's clock to a new date every time you needed to test under that particular instant in time. Hear that? Those are alarm bells ringing in your ears.

Don't worry dear reader, you are not completely stuffed. We can actually very easily fix this code. Simply put, we need to make our Payer class rely on abstractions rather than concretions. This means creating interfaces to describe what each of our dependencies need to do. I like to refer to them as contracts and they will act sort of like base classes.

Interfaces are like an abstract class, except they have absolutely no implementation. Like an abstract class though, classes can inherit from them and we can then use polymorphism to handle calling the right method at the right time.

Lets now look at our fixed up class:

using DecoupledPayBackTheMoney.Contracts;
using DecoupledPayBackTheMoney.Services;
using System;

namespace DecoupledPayBackTheMoney
{
    public class Payer
    {
        private readonly IMoneyPayerService _payer;
        private readonly INotificationService _notificationService;
        private readonly IDateTimeProvider _dateTimeProvider;

        /// <summary>
        /// Here we are inverting our dependencies by letting the caller pass them in
        /// </summary>
        public Payer(
            IMoneyPayerService payer,
            INotificationService notificationService,
            IDateTimeProvider dateTimeProvider)
        {
            _payer = payer;
            _notificationService = notificationService;
            _dateTimeProvider = dateTimeProvider;
        }

        /// <summary>
        /// Lazy-man's Dependency Injection
        ///
        /// This is our public parameterless constructor. It takes care of passing in our concrete implementations.
        ///
        /// Ideally you would be using IoC framework for this like Ninject or AutoFac
        /// </summary>
        public Payer()
            : this(new MoneyPayerService(),
                  new NotificationService(),
                  new DateTimeProvider())
        {
        }

        public bool TryPayMoney(DateTime timeToPayBackTheMoney, bool canDelayProcess)
        {
            DateTime now = _dateTimeProvider.GetCurrentDateTime();
            if (!canDelayProcess && timeToPayBackTheMoney <= now)
            {
                // Damn, have to pay
                _payer.Pay(7000000M);

                // Send Thuli email
                _notificationService.Send("jacob@anc.gov.za", "thuli@madonsela.gov.za", "I pay bak moneyies.\r\n Love, Jacob");
                return true;
            }

            // "HA HA HA! "Nkandla"" HA HA HA
            return false;
        }
    }
}

Things to note:

  • We have defined interfaces for each of our dependencies.
  • Each of our services in turn inherit from these interfaces.
  • This means that our services HAVE to implement the methods defined in the interfaces.
  • Our class now relies on these abstractions rather than concretions
  • We inject our dependencies via the constructor using what is called the strategy pattern
  • Instead of needing proper versions of each of our services to test, we could now pass some fake versions of these services which do not rely on things like the system clock or a mail server
  • We can now unit test our payer class!

Note: For simplicity, we are using what is known as "poor man's dependency injection". Ideally you would rather use an inversion of control framework like Ninject or AutoFac to handle this for you so that you are even more loosely coupled.

The full source code of both the rubbish code and the better decoupled code can be found on Github. Poke around and if you have any questions leave a comment below.

Image used is from The South Africa http://www.thesouthafrican.com/jacob-zuma-announces-his-resignation-cabinet-left-jobless/


Click here to comment