Use of Ninject as an IoC container in a WinForms MVC application

I am a relatively experienced WPF developer who has had good exposure to MVVM. I have been developing legacy WinForms applications for a while and have recently been asked to fully re-write a large one.

To do this I want to use TDD and in order to facilitate this and make the application as extensible as possible going forwards (opposite to what the old one was), I want to incorporate Dependency Injection and use the MVC pattern.

I would like some guidance on a few things:

• Is my use of Ninject for the DI Container proper and correct?

• Is my use of MVC proper and correct?

• How could I improve my code?

Program.cs (the main entry point of the WinForms application):

static class Program
{
    [STAThread]
    static void Main()
    {
        FileLogHandler fileLogHandler = new FileLogHandler(Utils.GetLogFilePath());
        Log.LogHandler = fileLogHandler;
        Log.Trace("Program.Main(): Logging initialized");

        CompositionRoot.Initialize(new DependencyModule());
        Application.EnableVisualStyles();
        Application.SetCompatibleTextRenderingDefault(false);
        Application.Run(CompositionRoot.Resolve());
    }
}

DependencyModule.cs

public class DependencyModule : NinjectModule
{
    public override void Load()
    {
        Bind().To();

        Bind().To();
        Bind().To();
    }
}

CompositionRoot.cs

public class CompositionRoot
{
    private static IKernel ninjectKernel;

    public static void Initialize(INinjectModule module)
    {
        ninjectKernel = new StandardKernel(module);
    }

    public static T Resolve()
    {
        return ninjectKernel.Get();
    }

    public static IEnumerable ResolveAll()
    {
        return ninjectKernel.GetAll();
    }
}

ApplicationShellView.cs (the main form of the application)

```
public partial class ApplicationShellView : C1RibbonForm, IApplicationShellView
{
private ApplicationShellController controller;

public ApplicationShellView()
{

This feels a bit awkward:

public class CompositionRoot
{
    private static IKernel ninjectKernel;

    public static void Initialize(INinjectModule module)
    {
        ninjectKernel = new StandardKernel(module);
    }

    public static T Resolve()
    {
        return ninjectKernel.Get();
    }

    public static IEnumerable ResolveAll()
    {
        return ninjectKernel.GetAll();
    }
}

As I understand it, an application's composition root is simply where composition occurs - ideally as close as possible to the entry point. This class doesn't encapsulate a "composition root", it merely wraps an IKernel and ultimately deprives the actual composition root from all the flexibility and power that Ninject offers, by literally forcing One True Way of accessing the kernel.

The thing is, the mere existence of that class raises a Big Red Flag: because it exists, and because it's storing a type-level (static) kernel instance, it's tremendously easy to have this line of code just about anywhere in the project:

var smurf = CompositionRoot.Resolve();

...and you have it, in the ApplicationShellController class - and since you've exposed static methods, you don't even have to pass it around as a dependency ...but it's a service locator nonetheless, because ApplicationShellController has a dependency on the CompositionRoot type - i.e. the root isn't at the root anymore!

private IDocumentController GetDocumentController(string path)
{
    return CompositionRoot.ResolveAll()
        .Cast()
        .Where(provider => provider.Handles(path))
        .Select(provider => provider)
        .FirstOrDefault();
}

It seems that LINQ bit is more complicated than it should be. ResolveAll returns an IEnumerable, so the Cast is redundant. Then the Select isn't projecting anything, and the Where could be the predicate for FirstOrDefault:

return CompositionRoot.ResolveAll()
                      .FirstOrDefault(provider => provider.Handles(path));

If the intent is to return one controller though, and the intent is that there's only ever one controller to handle the given path, then you should use .SingleOrDefault instead of .FirstOrDefault, to make the intent clearer (and the method would throw an exception if there's ever two or more controllers for a given path, instead of returning, well, some random controller of whatever type the thing feels like giving you).

If I understand your architecture correctly, you don't actually need a new instance of a controller every time this method gets called.

But that's what you're getting.

I think the ApplicationShellController constructor should look like this:

private readonly IApplicationShellView _view;
private readonly IEnumerable _controllers;

public ApplicationShellController(IApplicationShellView view, IEnumerable controllers)
{
    _view = view;
    _controllers = controllers;
}

Ninject sees the IEnumerable dependency as an opportunity for a multi-binding: it will automatically resolve an instance of everything that binds to IDocumentController, and pass it in.

You don't need the [Inject] attribute here. Don't use [Inject] attributes. Imagine you have your composition root in a completely different assembly - the only assembly in your solution that needs a reference to Ninject, is the assembly with the composition root. Having a dependency on Ninject anywhere else should be forbidden, your code should be written in a completely IoC Container-agnostic way: having an [Inject] attribute in your business code suggests that your business code is able to work with an IKernel at any given time, and you don't want to give that impression - be it only because in time, the code will be maintained by someone else (yes, future you is someone else!), and it's always a good idea to send a clear message to that future maintainer, that the IoC container lives and dies in its own little corner and doesn't travel anywhere.

Of course there are exceptions... for example you might want to use the Ninject.Logging extension and constructor-inject an ILogger to any logging-enabled type.

So, I'd remove the CompositionRoot class, and move the IKernel straight to the entry point, reference the conventions extension, and do something like this:

[STAThread]
static void Main()
{
    var kernel = new StandardKernel();
    kernel.Bind(t => t.FromThisAssembly()
                      .SelectAllClasses()
                      .BindAllInterfaces());

    FileLogHandler fileLogHandler = new FileLogHandler(Utils.GetLogFilePath());
    Log.LogHandler = fileLogHandler;
    Log.Trace("Program.Main(): Logging initialized");

    Application.EnableVisualStyles();
    Application.SetCompatibleTextRenderingDefault(false);
    Application.Run(kernel.Get());
}

The conventions extension can do so much more than automagically bind all interfaces in a single instruction though - that was just to illustrate the poin

StackExchange Code Review Q#122238, answer score: 4