Real-World Refactoring: DI in ThinController Project

July 24th, 2017

After looking back on some old ThinController code, today I wanted to clean up the dependency injection.

I was looking through some old posts and ran across my ThinController series (GitHub).

In this series, I wanted to created the smallest controller possible and succeeded (1 line of code for each ActionResult).

However, some weird code came out of certain sections in the project.

I also struck up a dialog with one reader and mentioned a couple of ideas on how to make this project better with some refactoring (Thanks, Ken!).

This project is my baseline for creating "Greenfield" projects and thought this would a great time to refactor the Dependency Injection section along with some POST adjustments.

(Sorry it took so long, Ken!)

In this first post, I'll refactor some of the dependency injection sticky points to make it a little more optimized and discuss the POST-ing issue on Wednesday.

Onto refactoring!

Dependency Injection Refactoring

First, I wanted to fix this interesting piece of code in the DI section of the DefaultViewModelFactory.

Infrastructure/DefaultViewModelFactory.cs

public class DefaultViewModelFactory : IViewModelFactory
{
    public TViewModel GetViewModel<TController, TViewModel>(TController controller)
    {
        var container = new Container(_ =>
            _.Scan(e =>
            {
                e.AssembliesFromApplicationBaseDirectory();
                e.WithDefaultConventions();
                e.ConnectImplementationsToTypesClosing(typeof(IViewModelBuilder<,>));
            }));

        var model = container.GetInstance<TViewModel>();
        var modelBuilder = container.GetInstance<IViewModelBuilder<TController, TViewModel>>();

        // Redirect and assist developers in adding their own modelbuilder/viewmodel
        if (modelBuilder == null)
            throw new Exception(
                string.Format(
                    "Could not find a ModelBuilder with a {0} Controller/{1} ViewModel pairing. Please create one.",
                    typeof(TController).Name, typeof(TViewModel).Name));

        return modelBuilder.Build(controller, model);
    }

    public TViewModel GetViewModel<TController, TViewModel, TInput>(TController controller, TInput data)
    {
        var container = new Container(_ =>
            _.Scan(e =>
            {
                e.AssembliesFromApplicationBaseDirectory();
                e.WithDefaultConventions();
                e.ConnectImplementationsToTypesClosing(typeof(IViewModelBuilder<,,>));
            }));

        var model = container.GetInstance<TViewModel>();
        var modelBuilder = container.GetInstance<IViewModelBuilder<TController, TViewModel, TInput>>();

        // Redirect and assist developers in adding their own modelbuilder/viewmodel
        if (modelBuilder == null)
            throw new Exception(
                string.Format(
                    "Could not find a ModelBuilder with a {0} Controller/{1} ViewModel/{2} TInput pairing. Please create one.",
                    typeof(TController).Name,
                    typeof(TViewModel).Name,
                    typeof(TInput).Name)
            );

        return modelBuilder.Build(controller, model, data);
    }
}

THIS made me feel dirty.

If you notice the container, we are scanning again in one of our methods and then again in another.

We already performed a scan at startup! Now, we scan a second and third time.

Not good. Once is enough.

It wasn't bad enough when I saw this code, but Jimmy Bogard also mentioned some Do's and Don'ts of containers and this was one of them (Avoid scanning an assembly more than once).

The dilemma with this approach is we need a container for creating our ViewModel. How can we get the container to create a new ViewModelBuilder instance for our ViewModel?

If we look at the parameter, we see a controller is always passed.

Ah-HA!

Since we will be using the ViewModelFactory on every controller, why not attach the Container to controllers?

This would make things a heck of a lot easier.

Refactor the DI

First step is to move the dependency injection code into the DependencyResolution IoC.cs at startup.

This will make our application faster instead of executing ad-hoc loading of assemblies.

We'll move the scanning into a new class called DemoRegistry.cs.

DependencyResolution/DemoRegistry.cs

public class DemoRegistry : Registry
{
    #region Constructors and Destructors
 
    public DemoRegistry()
    {
        Scan(
            scan => {
                scan.AssembliesFromApplicationBaseDirectory();
                scan.WithDefaultConventions();
                scan.AddAllTypesOf<AbstractUnitOfWork>();
                scan.ConnectImplementationsToTypesClosing(typeof(IViewModelBuilder<,>));
                scan.ConnectImplementationsToTypesClosing(typeof(IViewModelBuilder<,,>));
            });
    }

    #endregion
}

We can now adjust our IoC.cs to include our DemoRegistry.

DependencyResolution/IoC.cs

public static class IoC {
    public static IContainer Initialize() {
        return new Container(c =>
        {
            c.AddRegistry<DefaultRegistry>();
            c.AddRegistry<DemoRegistry>();
        });
    }
}

Building the Base (Controller)

Our next step is to make sure we have a container for every controller we create.

Infrastructure/BaseController.cs

public class BaseController : Controller
{
    public IContainer Container { get; set; }

    public BaseController(IContainer container)
    {
        if (container == null)
        {
            throw new Exception("Invalid reference to a container (Container is null).");
        }

        Container = container;
    }
}

This is probably the easiest of changes in the project. StructureMap will automatically give us a container, we save it, and we expose the Container for descendant classes and functionality.

If we visit our FaqController, you'll see there are minimal changes.

Controllers/FAQController.cs

public class FaqController : BaseController
{
    private DefaultViewModelFactory _factory;

    public FaqController() : this(new DefaultViewModelFactory()) { }

    public FaqController(DefaultViewModelFactory factory, 
        IContainer container = null) : base(container)
    {
        _factory = factory;
    }

    // GET: FAQ
    public ActionResult Index()
    {
        return View(_factory.GetViewModel<FaqController, FaqViewModel>(this));
    }
}

Two things may catch your eye:

  1. Our FaqController now has a parameterless constructor and a DefaultViewModelFactory and IContainer parameter with optional null.
  2. We inherit from BaseController.

Even though we set the IContainer to null, StructureMap helps us out with an instance and we pass it along to the BaseController.

Our HomeController (and any other controller) should follow this same approach by inheriting from BaseController.

Full Circle

With all of our changes, what does our DefaultViewModelFactory look like now?

Quite simply, we replace one line with another.

Infrastructure/DefaultViewModelFactory.cs

public class DefaultViewModelFactory : IViewModelFactory
{
    public TViewModel GetViewModel<TController, TViewModel>(TController controller)
    {
        var container = (controller as BaseController).Container;

        var model = container.GetInstance<TViewModel>();
        var modelBuilder = container.GetInstance<IViewModelBuilder<TController, 
            TViewModel>>();

        // Redirect and assist developers in adding their own modelbuilder/viewmodel
        if (modelBuilder == null)
            throw new Exception(
                string.Format(
                    "Could not find a ModelBuilder with a {0} Controller/{1} " +
                    "ViewModel pairing. Please create one.",
                    typeof(TController).Name, typeof(TViewModel).Name));

        return modelBuilder.Build(controller, model);
    }

    public TViewModel GetViewModel<TController, TViewModel, TInput>(TController controller, 
        TInput data)
    {
        var container = (controller as BaseController).Container;

        var model = container.GetInstance<TViewModel>();
        var modelBuilder = container.GetInstance<IViewModelBuilder<TController, 
            TViewModel, TInput>>();

        // Redirect and assist developers in adding their own modelbuilder/viewmodel
        if (modelBuilder == null)
            throw new Exception(
                string.Format(
                    "Could not find a ModelBuilder with a {0} Controller/{1} ViewModel/{2} " +
                    "TInput pairing. Please create one.",
                    typeof(TController).Name,
                    typeof(TViewModel).Name,
                    typeof(TInput).Name)
                );

        return modelBuilder.Build(controller, model, data);
    }
}

The piece of code replaced was the first line in both methods. Since we know every controller will have a Container field, we access it, and we can proceed forward with getting our instance of TViewModel and TViewModelBuilder.

Conclusion

Once I looked at some old code, I realized (YIKES!) I needed to refactor some code to make the application a little more zippy.

The funny part about this refactoring is we reshuffled some code, added a couple lines to controllers, and replaced one line with another.

While this didn't seem like a huge refactoring effort, we made the application faster by removing a time-consuming process of scanning for assemblies when a controller wanted to create a ViewModel (which was every time).

All we needed to do was pass the container around.

In the end, it's always all about performance.

Was there a better way to refactor this? Post your comments and let's discuss.