5
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ Looks like a pretty standard IoC set up to me. What exactly bothers you about it? \$\endgroup\$ – RubberDuck Aug 21 '15 at 11:36
  • \$\begingroup\$ Well @RubberDuck, I was thinking that the need to know the number of parameters the constructor takes is kind of weird, as I will need to change that in the container set up to be up to date to the class itself. \$\endgroup\$ – mitomed Aug 21 '15 at 11:40
5
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • 3
    \$\begingroup\$ This is one of the best examples of codeless answers I've seen in a long while. ++ for beating me to the interface:class ratio in regards to the IoC container. \$\endgroup\$ – RubberDuck Aug 21 '15 at 22:30
3
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ if (totalNumberOfFiles == 0) is wrong (should be 1), plus one could argue that his two ifs are sanity checks. I prefer the original code. \$\endgroup\$ – BCdotWEB Aug 21 '15 at 15:19
  • \$\begingroup\$ I changed it, but I also think that if (totalNumberOfFiles == 1) is also a sanity check. my version shows what you want to happen first. which is how most people read code. if it were a try catch, you wouldn't write the catch before the try, would you, @BCdotWEB? \$\endgroup\$ – Malachi Aug 21 '15 at 15:24
  • \$\begingroup\$ I respectfully disagree that you should code what you want to happen first. Many developers subscribe to the Fail-Fast school of thought. \$\endgroup\$ – Rick Davin Aug 21 '15 at 15:30
  • \$\begingroup\$ I'm making a few assumptions about the context here that may or may not be right, but let's say this gets called a few thousand times per minute (unlikely) and more often than not the data is fine and exactly equal to one, in that case this is slightly more efficient. Slightly. Of course, if the data is not equal to one more often than not, then this isn't any more efficient. (In fact, it would be worse.) It's a good solution if the situation warrants it, but I find it unlikely that it's warranted and is most likely premature optimization. \$\endgroup\$ – RubberDuck Aug 21 '15 at 16:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.