Dependency injection or static class for helper class?

Question

In most of my project I create a static class containing all the functions I use all in my project. But when it comes to unit testing, I feel like there's a big flaw in my design, since I can't use Moq to Mock this static class.

I can provide a simple example with one project I'm working on that copies a lot of files, so I made a little helper to avoid redundancy in my code.

public static class Helpers
{
    public static void CopyFile(string sourcePath, string destPath)
    {
        Log.Verbose("Start copy from {SourcePath} to {DestPath}", sourcePath, destPath);
        if (!Directory.Exists(Path.GetDirectoryName(destPath)))
        {
            Directory.CreateDirectory(Path.GetDirectoryName(destPath));
        }
        File.Copy(sourcePath, destPath, true);
        Log.Verbose("End of copy from {SourcePath} to {DestPath}", sourcePath, destPath);
    }
}

So basically any code that calls this function is untestable, since I can't mock a static function inside a static class which also relies on System.IO

I tried to use System.IO.Abstraction and dependency injection to solve this issue, and I then have the following code:

public interface IHelpers
{
    public void CopyFile(string sourcePath, string destPath);
}

public class Helpers(IFileSystem fileSystem) : IHelpers
{
    private readonly IFileSystem FileSystem = fileSystem;

    public void CopyFile(string sourcePath, string destPath)
    {
        Log.Verbose("Start copy from {SourcePath} to {DestPath}", sourcePath, destPath);
        if (!FileSystem.Directory.Exists(Path.GetDirectoryName(destPath)))
        {
            FileSystem.Directory.CreateDirectory(Path.GetDirectoryName(destPath));
        }
        FileSystem.File.Copy(sourcePath, destPath, true);
        Log.Verbose("End of copy from {SourcePath} to {DestPath}", sourcePath, destPath);
    }
}

Now I just need to inject my IHelper wherever I need to call CopyFile and everything seems fine. Except I have some rare occasions where it just feels wrong to inject this helper inside a class.

public class Component
{
    public string Name;
    public string Path;

    public void ExportOutput()
    {
        // ...
        // much calculation then...
        // multiple calls to Helpers.CopyFile(src,dest);
        // ...
    }
}

To solve this, I passed the IHelper class as a parameter to the ExportOutput, but this seems absolutely disgusting.

The only solution I found to this issue was to remove the ExportOutput function and to put it somewhere where I could rely on dependency injection to use the Helper class.

Is this right or am I completely missing the point?

How do you generally handle those small helper function in you code?

Answer 1

As noted, you can use the IFileSystem interface for this exact scenario.

Otherwise, if the method you're working with is static, you don't need to create an interface. You can use a delegate. That way you don't have to choose between a static method or dependency injection. You can inject the static method.

delegate void CopyFunction(string sourcePath, string destinationPath);

You can register it with DI just as you do with an interface or class:

services.AddSingleton<CopyFunction>(Helpers.CopyFile);

...and then inject it.

public class Component
{
    private readonly CopyFunction _copyFunction;

    public Component(CopyFunction copyFunction)
    {
        _copyFunction = copyFunction;
    }
}

You call it just like any other function.

_copyFunction(source, destination);

What's especially nice is that in order to mock it you don't need Moq. You can use an anonymous function.

It could be

CopyFunction copyFunction = (source, destination) => {}; // does nothing

Or if you wanted to assert that specific values were passed, you could do something like this:

string copiedSource = null;
string copiedDestination = null;

CopyFunction copyFunction = (source, destination) => 
{
    copiedSource = source;
    copiedDestination = destination;
};

...so that after the function gets called you can assert the values of copiedSource and copiedDestination.

That's a benefit of delegates. They're very easy to mock by creating an anonymous method.

Even if the code changes later and you want to replace the static method with an instance method, you can do that without replacing the delegate with an interface and changing all the code.

Another benefit of delegates is that by definition they have a single purpose. They represent one method. They can be defense against tendencies to create large interfaces with too many methods. You can add a method (or 20) to an interface, but not to a delegate.

---

You can also use Action<string, string> instead of a delegate. The risk, however unlikely, is that you want to inject two methods in two places that have that exact signature but do different things. The delegate allows you to specify the use of the method you're injecting.

Answer 2

If you want to Mock file system operations you will need an interface that abstract away the file system, and inject this everywhere the file system is used.

An alternative to your approach would be to create your CopyFile as a extension to the file system, i.e.

public static class Helpers
{
    public static void CopyFile(this IFileSystem fileSystem, string sourcePath, string destPath)
    {
        ...
    }
}

This approach will treat the CopyFile-implementation as part of the thing that is tested, and this may or may not be desirable depending on the circumstances. A possible advantage is that it would allow a the method being tested to provide its own CopyFile implementation without needing to change dependencies or tests.

Except I have some rare occasion where it just feels wrong to inject this helper inside a class

If your ExportOutput method depends on the file system, and you want this dependency to be testable, you will need to inject this dependency somehow. I would probably use inject the dependency in the constructor of the Component, or move ExportOutput to its own class. But there may be cases where using a method parameter may be appropriate.

Do note that file systems may have a bunch of quirks that may be difficult to reproduce accurately, especially considering all the ways IO operations may fail. So you may want to have at least some tests that use the actual file system, even if this may be significantly slower. Just remember to clean up any left over files afterwards.

Answer 3

I think a layer of abstraction may help this look neater, separating out the copying functionality

public interface IFileCopier
{
    public void CopyFile(string sourcePath, string destPath);
}

public class FileCopier:IFileCopier
{
    private readonly IFileSystem FileSystem;

    public void CopyFile(string sourcePath, string destPath)
    {
        Log.Verbose("Start copy from {SourcePath} to {DestPath}", sourcePath, destPath);
        if (!FileSystem.Directory.Exists(Path.GetDirectoryName(destPath)))
        {
            FileSystem.Directory.CreateDirectory(Path.GetDirectoryName(destPath));
        }
        FileSystem.File.Copy(sourcePath, destPath, true);
        Log.Verbose("End of copy from {SourcePath} to {DestPath}", sourcePath, destPath);
    }
}

public class Helpers
{
    private readonly IFileCopier FileCopier;

    public void CopyFile(string sourcePath, string destPath)
    {
      FileCopier.CopyFile(sourcePath,destPath);
    }
}

Then you can go on to mock or dependency inject the IFileCopier instance. This may feel like a simple rename (because Helper has no other functionality yet), but it highlights which functionally you want to inject and why.