Base service for two different implementations

Question

I have code that fetches different templates for HTML and CSS in the file system. The templates are stored in different folders, so I have two implementations for the code that fetches them, passing the correct path as a parameter.

   private string _templatePath;

    private readonly IConfigurationManager _configurationManager;
    private readonly IFileSystem _fileSystem;

    public BaseTemplateFileService(string templatePath, 
        IConfigurationManager configurationManager,
        IFileSystem fileSystem)
    {
        _templatePath = templatePath;
        _configurationManager = configurationManager;
        _fileSystem = fileSystem;
    }

    protected abstract string GetExtensionPattern(string templateName);

    public string GetTemplate(string templateName)
    {
        var templatesPath = _configurationManager.GetAppSetting(_templatePath);
        var fullName = GetExtensionPattern(templateName);

        var templatesFolder = _fileSystem.DirectoryInfo.FromDirectoryName(templatesPath);
        var files = templatesFolder.GetFiles(fullName, SearchOption.AllDirectories);
        var totalNumberOfFiles = files.Count();
        if (totalNumberOfFiles < 1)
        {
            throw new PanelException(string.Format("No template file to choose with name: {0}", fullName));
        }
        if (totalNumberOfFiles > 1)
        {
            throw new PanelException(string.Format("Multiple template files to choose with name: {0}", fullName));
        }
        using (var streamReader = files[0].OpenText())
        {
            var content = streamReader.ReadToEnd();

            return content;
        }
    }

public class HtmlTemplateFileService : BaseTemplateFileService
{
    public HtmlTemplateFileService(IConfigurationManager configurationManager,
        IFileSystem fileSystem)
        : base("panelHtmlPath", configurationManager, fileSystem)
    {   
    }

    protected override string GetExtensionPattern(string folderName)
    {
        return string.Format("{0}.htm*", folderName);
    }
}

public class CssTemplateFileService : BaseTemplateFileService
{
    public CssTemplateFileService(IConfigurationManager configurationManager,
        IFileSystem fileSystem)
        : base("panelCssPath", configurationManager, fileSystem)
    {   
    }

    protected override string GetExtensionPattern(string folderName)
    {
        return string.Format("{0}.scss*", folderName);
    }
}

What I don't like about this approach is that I need to set up my IoC contaier (i.e. Unity) like this

container.RegisterType<CssPanelGenerator>(new InjectionConstructor(
    new ResolvedParameter<ITemplateService>("cssTemplateService"), typeof(CssVariableService), typeof(ProcessSass)));

and same for the HTML one.

Is there a cleaner way to avoid code repetition but without the need to use this slightly more convoluted way of doing dependency injection that I can't think of?

Answer 1

App settings

When dealing with something like ConfigurationManager, Session, some state bag, etc., it's common to want to just inject the whole thing. The problem with that is that not only do you jump through some hoops to wrap the thing so it can be injected, but also the provider-specific means of interacting with it comes along. So you tie yourself to always having a keyed dictionary of sorts to pass in. You might be better served injecting the one setting instead of putting AppSettings access in your method. I'm not sure how this is accomplished in Unity, but you can inject .FromAppSettings(...) in Castle Windsor and even more.

File System

Kind of the same thing. I understand why you are doing this, but there's a difference between really isolating code and turning procedural code inside-out by calling the same instructions through multiple methods. It will probably help to simplify things by fetching your files a layer up and passing them to your class method(s). If you're like "Yeah, but how do I know my file access works?" I would respond with, "Are you trying to test your code in isolation, or trying to make your code runnable in unit tests?" With the former approach, you may get less test coverage, but you can get extremely granular tests around what it is you're doing. The latter approach results in integration tests that require considerable setup and probably end up testing the .NET Framework (i.e. this file would always be able to be accessed unless there was a bug in the framework).

Not sure how flexible your options are with file types, but the level of granularity you probably want to pursue is being able to pass in mock files and overriding the OpenText() method to ensure your mocked content is returned from files that pass validation. But keep it simple--don't go overboard with trying to inject file objects or wrapping them in interfaces either just so you can write tests.

Inheritance

You probably don't need to have a base class and an interface for that base class. Just register the base class in your container (why not make it abstract?) and use that in your signatures. Is that interface used for anything but making a mock? Plus, a 1:1 interface to class ratio or close to it usually means "too many interfaces" or "I'm worrying too hard about breaking the rules of making code testable."

Don't worry about specifying type associations in IoC configuration

I can't think of a more appropriate place to define any rules around typing than the container. As long as the abstract class is necessary, this is fine. This is fairly common when you have multiple classes that implement the same interface (a good thing, it means you are using an interface for its intended use and not just as a slick means to mock something).

But...

The only thing that is different between your classes is a suffix string. You don't need an inheritance structure to accomplish this--just make the suffix a parameter in your constructor! Then you can associate each different type of template generator with a specific named instance, or you can use a factory method that is run when resolving generators, where each generator registration provides a different string.

Some Philosophy

Instead of trying to get all code under test, try to completely isolate logic starting from the inside and working outwards, redrawing the boundaries between objects as you go. Chances are this C# code is not in an ASP.NET MVC app that was designed for DI. If you have to move code up a layer or completely out of the DI paradigm to get rid of awkward dependencies, it's not the end of the world, especially if you can still test it in isolation. After all, you could still make a static method in your page or form-level layer that can be tested by a test class directly accessing it. Otherwise you may end up causing yourself a lot of pain trying to split a (rather simple) process up in steps across multiple objects.

Could the same means of simplification by parameterizing the suffix also be applied to the generators for additional reduction in complexity and number of objects? I haven't seen the code, but I'm thinking probably.

In closing

Prioritize simplicity and elegance over a focus on separation of concerns.

Answer 2

Your code here could be cleaned up a little bit:

    if (totalNumberOfFiles < 1)
    {
        throw new PanelException(string.Format("No template file to choose with name: {0}", fullName));
    }
    if (totalNumberOfFiles > 1)
    {
        throw new PanelException(string.Format("Multiple template files to choose with name: {0}", fullName));
    }
    using (var streamReader = files[0].OpenText())
    {
        var content = streamReader.ReadToEnd();

        return content;
    }

You should always code what you want to happen first, and only throw exceptions at the end (if possible), like this:

if (totalNumberOfFiles == 1)
{
    using (var streamReader = files[0].OpenText())
    {
        var content = streamReader.ReadToEnd();
        return content;
    }
}
else if (totalNumberOfFiles < 1)
{
    throw new PanelException(string.Format("No template file to choose with name: {0}", fullName));
}
else if (totalNumberOfFiles > 1)
{
    throw new PanelException(string.Format("Multiple template files to choose with name: {0}", fullName));
}

This way, if everything is going as expected, you only check the totalNumberOfFiles once instead of checking them twice for no reason.