Don’t pass dependencies into helper methods

This post is part of a series of blog posts about bad codes or bad practices I’ve come across. Note that this is my personal opinion and some of the things I mention could be debatable.

Post 1: Code review, why it matters

Post 2: Not including JS, CSS and static content files in project

Post 3: Let’s just not unit test our code

Post 4: Don’t pass dependencies into helper methods

Post 5: Inline SQL query statement in static helper class

Problem

A few weeks ago, I come across a static helper method while reviewing a Pull Request. This method accepts two service interfaces and a couple other parameters. It then goes off to these services to grab more data before doing more calculation with the returned data and other parameters.

i.e. Something similar to this

public interface ITestServiceA
{
    Dictionary<string, string> GetTestDataA();
}

public interface ITestServiceB
{
    Dictionary<string, string> GetTestDataB();
}

public static class Helper
{
    public static bool CheckSomething(ITestServiceA serviceA, ITestServiceB serviceB)
    {
        var dataA = serviceA.GetTestDataA();
        var dataB = serviceB.GetTestDataB();

        // More logic here
    }
}

public interface ISampleService
{
    string GetSomeString();
}

public class SampleService : ISampleService
{
    private readonly ITestServiceA _testServiceA;
    private readonly ITestServiceB _testServiceB;

    public SampleService(ITestServiceA testServiceA, ITestServiceB testServiceB)
    {
        _testServiceA = testServiceA;
        _testServiceB = testServiceB;
    }

    public string GetSomeString()
    {
        var result = Helper.CheckSomething(_testServiceA, _testServiceB);

        // More logic
    }
}

Effect

This makes every service or controller that calls this static method to depend on the same dependencies as the static method. This means when you unit test these services or controllers, you have to mock these dependencies as well when only the static method depends on these, not the services/controllers that calling it. Another issue is since it’s a static method, you cannot mock this method. You would need to understand the implementation of this static method and setup test inputs accordingly to the method if you want to get a particular result back from the method.

Solution

I have two solutions to solve this:

1. Don’t pass dependencies into static method but instead have the caller pass in the exact data the static method needs. To me, static method should be functional. This means it should not need to go off to other services to do its calculation. It should be self-contained and only performs its calculation based on the inputs passed into it. The above code now looks like this.

   public interface ITestServiceA
   {
       Dictionary<string, string> GetTestDataA();
   }
   
   public interface ITestServiceB
   {
       Dictionary<string, string> GetTestDataB();
   }
   
   public static class Helper
   {
       public static bool CheckSomething(Dictionary<string, string> dataA, Dictionary<string, string> dataB)
       {
           // More logic here
       }
   }
   
   public interface ISampleService
   {
       string GetSomeString();
   }
   
   public class SampleService : ISampleService
   {
       private readonly ITestServiceA _testServiceA;
       private readonly ITestServiceB _testServiceB;
   
       public SampleService(ITestServiceA testServiceA, ITestServiceB testServiceB)
       {
           _testServiceA = testServiceA;
           _testServiceB = testServiceB;
       }
   
       public string GetSomeString()
       {
           var dataA = serviceA.GetTestDataA();
           var dataB = serviceB.GetTestDataB();
           var result = Helper.CheckSomething(dataA, dataB);
   
           // More logic
       }
   }

2. Don’t use static class/method, create an interface and make a utilities service class implementing that interface. This utilities service class can have its dependencies injected via constructor.

   public interface ITestServiceA
   {
       Dictionary<string, string> GetTestDataA();
   }
   
   public interface ITestServiceB
   {
       Dictionary<string, string> GetTestDataB();
   }
   
   public interface IUtilitiesService
   {
       bool CheckSomething();
   }
   
   public class UtilitiesService : IUtilitiesService
   {
       private readonly ITestServiceA _testServiceA;
       private readonly ITestServiceB _testServiceB;
   
       public UtilitiesService(ITestServiceA testServiceA, ITestServiceB testServiceB)
       {
           _testServiceA = testServiceA;
           _testServiceB = testServiceB;
       }
   
       public bool CheckSomething()
       {
           var dataA = serviceA.GetTestDataA();
           var dataB = serviceB.GetTestDataB();
   
           // More logic here
       }
   }
   
   public interface ISampleService
   {
       string GetSomeString();
   }
   
   public class SampleService : ISampleService
   {
       private readonly IUtilitiesService _utilitiesService;
   
       public SampleService(IUtilitiesService utilitiesService)
       {
           _utilitiesService = utilitiesService;
       }
   
       public string GetSomeString()
       {
           var result = _utilitiesService.CheckSomething();
   
           // More logic
       }
   }

I prefer the second solution because the services/controllers calling this method don’t need the same dependencies, only the utilities service has these dependencies and it allows easily mocking returned result when unit testing them.

When I request changes on this Pull Request and suggest the second solution, the code author’s argument is that technically whatever calling this method depends on its dependencies too. He also argues that if injecting all dependencies into the constructor then it might be a waste constructing these dependencies as the method you’re calling might not need all these dependencies.

For the first argument, sure that it technically does depend on the same dependencies but in the second solution we would only depend on one dependency and you don’t care what other dependencies this dependency might depend on. It is one of the main reason why we use dependency injection.

With the second argument, you might as well don’t use dependency injection at all if you’re concerned about wasted resources constructing the dependencies. Also, if you have a controller that calls a service that calls this method, then the top level code would have to know all dependencies that its dependency depends on. That would be very hard to maintain as you would have to deal with multiple dependencies instead of one. Imagine how many dependencies you would have to mock in unit tests. This only gets worse if you have service calling another service, calling another service, calling another service…