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

Question

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:

1. Is my use of Ninject for the DI Container proper and correct?
2. Is my use of MVC proper and correct?
3. 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<ApplicationShellView>());
    }
}

DependencyModule.cs

public class DependencyModule : NinjectModule
{
    public override void Load()
    {
        Bind<IApplicationShellView>().To<ApplicationShellView>();

        Bind<IDocumentController>().To<SpreadsheetController>();
        Bind<ISpreadsheetView>().To<SpreadsheetView>();
    }
}

CompositionRoot.cs

public class CompositionRoot
{
    private static IKernel ninjectKernel;

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

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

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

ApplicationShellView.cs (the main form of the application)

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

    public ApplicationShellView()
    {
        this.controller = new ApplicationShellController(this);
        InitializeComponent();
        InitializeView();
    }

    public void InitializeView()
    {
        dockPanel.Extender.FloatWindowFactory = new CustomFloatWindowFactory();
        dockPanel.Theme = vS2012LightTheme;
    }

    private void ribbonButtonTest_Click(object sender, EventArgs e)
    {
        controller.OpenNewSpreadsheet();
    }

    public DockPanel DockPanel
    {
        get { return dockPanel; }
    }
}

Where:

public interface IApplicationShellView
{
    void InitializeView();

    DockPanel DockPanel { get; }
}

ApplicationShellController.cs

public class ApplicationShellController
{
    private IApplicationShellView shellView;
    private List<IDocumentController> documentControllerCache = new List<IDocumentController>();

    [Inject]
    public ApplicationShellController(IApplicationShellView view)
    {
        this.shellView = view;
    }

    public void OpenNewSpreadsheet(DockState dockState = DockState.Document)
    {
        SpreadsheetController controller = (SpreadsheetController)GetDocumentController("new.xlsx");
        SpreadsheetView view = (SpreadsheetView)controller.New("new.xlsx");
        view.Show(shellView.DockPanel, dockState);
        documentControllerCache.Add(controller);
    }

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

    public IApplicationShellView ShellView { get { return shellView; } }
}

SpreadsheetController.cs

public class SpreadsheetController : IDocumentController 
{
    private ISpreadsheetView view;

    public SpreadsheetController(ISpreadsheetView view)
    {
        this.view = view;
        this.view.SetController(this);
    }

    public bool Handles(string path)
    {
        string extension = Path.GetExtension(path);
        if (!String.IsNullOrEmpty(extension))
        {
            if (FileTypes.Any(ft => ft.FileExtension.CompareNoCase(extension)))
                return true;
        }
        return false;
    }

    public void SetViewActive(bool isActive)
    {
        ((SpreadsheetView)view).ShowIcon = isActive;
    }

    public IDocumentView New(string fileName)
    {
        // Opens a new file correctly.
    }

    public IDocumentView Open(string path)
    {
        // Opens an Excel file correctly.
    }

    public IEnumerable<DocumentFileType> FileTypes
    {
        get
        {
            return new List<DocumentFileType>()
            {
                new DocumentFileType("CSV",  ".csv" ),
                new DocumentFileType("Excel", ".xls"),
                new DocumentFileType("Excel10", ".xlsx")
            };
        }
    }
}

Where the implemented interface is:

public interface IDocumentController
{
    bool Handles(string path);

    void SetViewActive(bool isActive);

    IDocumentView New(string fileName);

    IDocumentView Open(string path);

    IEnumerable<DocumentFileType> FileTypes { get; }
}

Now the view ascociated with this controller is:

public partial class SpreadsheetView : DockContent, ISpreadsheetView
{
    private IDocumentController controller;

    public SpreadsheetView()
    {
        InitializeComponent();
    }

    private void SpreadsheetView_Activated(object sender, EventArgs e)
    {
        controller.SetViewActive(true);
    }

    private void SpreadsheetView_Deactivate(object sender, EventArgs e)
    {
        controller.SetViewActive(false);
    }

    public void SetController(IDocumentController controller)
    {
        this.controller = controller;
        Log.Trace("SpreadsheetView.SetController(): Controller set successfully");
    }

    public string DisplayName
    {
        get { return Text; }
        set { Text = value; }
    }

    public WorkbookView WorkbookView
    {
        get { return workbookView; }
        set { workbookView = value; }
    }

    public bool StatusBarVisible
    {
        get { return statusStrip.Visible; }
        set { statusStrip.Visible = value; }
    }

    public string StatusMessage
    {
        get { return statusLabelMessage.Text; }
        set { statusLabelMessage.Text = value; }
    }
}

The view interfaces are:

public interface ISpreadsheetView : IDocumentView
{
    WorkbookView WorkbookView { get; set; } 
}

And:

public interface IDocumentView
{
    void SetController(IDocumentController controller);

    string DisplayName { get; set; }

    bool StatusBarVisible { get; set; }
}

All this code works and "feels okay". But I am not experienced with either MVC (at least not using WinForms) or Ninject/DI (I have used MEF extensively, but that is slightly difference that what Ninject does).

Note: I am implementing IDocumentView because I want to also open and dock other types of documents (code and text etc.).

Could you provide and guidance as to what I have done so far and answer the questions above?

Answer 1

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<T>()
    {
        return ninjectKernel.Get<T>();
    }

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

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<ISmurf>();

...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<IDocumentController>()
        .Cast<IDocumentController>()
        .Where(provider => provider.Handles(path))
        .Select(provider => provider)
        .FirstOrDefault();
}

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

return CompositionRoot.ResolveAll<IDocumentController>()
                      .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<IDocumentController> _controllers;

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

Ninject sees the IEnumerable<IDocumentController> 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<IApplicationShellView>());
}

The conventions extension can do so much more than automagically bind all interfaces in a single instruction though - that was just to illustrate the point that you don't need to manually call .Bind<ISmurf>() to explicitly bind every smurf in the village.

For example you could enforce a naming convention in a given namespace, so that anything that doesn't end with "Controller" doesn't bind to IDocumentController, for example.

---

Resolving for IApplicationShellView, Ninject will get to the ApplicationShellView type. And there's a problem:

public ApplicationShellView()
{
    this.controller = new ApplicationShellController(this);
    InitializeComponent();
    InitializeView();
}

There's nothing to inject! The view is completely coupled with the concrete controller! But wait, why does the view need to know about its parent controller?

private void ribbonButtonTest_Click(object sender, EventArgs e)
{
    controller.OpenNewSpreadsheet();
}

You need to invert the dependency here (Inversion of Control right?), and instead of having the view asking the controller to do something, have the view tell the controller it should to be doing something because the user clicked a button.

WinForms is all about events; I'd use an event.

public event EventHandler TestButtonClicked;
private void ribbonButtonTest_Click(object sender, EventArgs e)
{
    var handler = TestButtonClicked;
    handler?.Invoke(this, EventArgs.Empty);
}

Inverting the relationship between the controller and the view has just broken the circular dependency: now all the controller needs to do is handle the view's events!

private void shellView_TestButtonClicked(object sender, EventArgs e)
{
    OpenNewSpreadsheet();
}