WinForms MVC with Dependency Injection

Question

I am in the process of re-writing a WinForms application from scratch (and it has to be WinForms, as much as I want to use WPF and MVVM). Do do this I have chosen to use the MVC pattern and attempt to use Dependency Injection (DI) where possible to increase testability, maintainability etc.

The problem I am having is with use of MVC and DI. With the baisic MVC pattern, the controller must have access to the view and the view must have access to the controller (see here for an WinForms example); this leads to a circular reference when using Ctor-Injection and this is the crux of my question. First please consider 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;

    [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);
    }

    private IDocumentController GetDocumentController(string path)
    {
        return return CompositionRoot.ResolveAll<IDocumentController>()
            .SingleOrDefault(provider => provider.Handles(path));
    }

    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; }
}

I am new to DI and Ninject so I have two questions:

1. How can I prevent myself using this.view.SetController(this); in the SpreadsheetController, here it feels like I should be using the IoC Container, but using pure Ctor-Injection leads to circular reference and a StackOverflowException. Can this be done using pure DI?

because I don't have a binding framework like with WPF (or with ASP.NET the ability to link the view and the controller implicitly), I have to expose the view and the controller to each other explicitly. This does not "feel" right and I think this should be possible vie the Ninject IoC container but I don't have the experience to establish how this can be done (if it can).

2. Is my use of Ninject/DI correct here. The way I am using my CompositionRoot and the method GetDocumentController(string path) feels like the service locator anti-pattern, how can I make this right?

At the moment this code works fine, but I want to get it right. Thanks very much for your time.

Continence

I noticed you said earlier that you would prefer to us MVVM and WPF. Well I have been for a while using WVVM with WinForms and find it to be much more efficient than either MVC or MVP. WinForms bindings aren't as advanced as in WPF but I find that in most cases the work quite well enough. To help me with the whole thing I have been using DevExpress MVVM for WinForms: documentation.devexpress.com/#WindowsForms/CustomDocument113955 – Monatomic

Answer 1

I am working on a project with a similar architecture.

I guess your main problem is that the event handlers of your view directly call the controller. E.g:

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

Try to avoid this. Let your controller objects be the masters of your application. Let the views and models be "blind and deaf".

When your view encounters a user action, just raise another event. Let the controller be responsible to register to this event and handle it. Your view is going to be like this:

public event EventHandler<EventArgs> RibbonButtonTestClicked ;

protected virtual void ribbonButtonTest_Click(object sender, EventArgs e)
{
    var handler = RibbonButtonTestClicked;
    if (handler != null) handler(this, EventArgs.Empty);
}

If you do this, you should be able to get rid of all the controller reference in the view. Your controller contstructor will look like this:

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

Since you can not resolve your object tree from a view anymore, add a method "GetView()" to your controller and change your Program.Main() method:

CompositionRoot.Initialize(new DependencyModule());
Application.EnableVisualStyles();
Application.SetCompatibleTextRenderingDefault(false);
var appCtrl = CompositionRoot.Resolve<ApplicationShellController>()
Application.Run(appCtrl.GetView());

Effluvium

I am having an issue with my main shell closure. When I click on the main windows close button I am currently wiring the controllers of the IDocuments to event handlers in the associated views and then the controllers can perform theclose / taredown. This issue with this is that from the main shell I only have access to the views and from here I cannot access the underlying controllers to test if the documents are in a dirty state. I want to keep the views "blind" but I can't see another way to access the controllers other than view.GetController() then do something nasty. Any thoughts? – Continence

@Killercam, are you using the WPF DockPanel over a ElementHost? We are using the DevExpress WinForms DockPanel and DevExpress XtraForms. They work together pretty well. When the user closes the main window, each open panel is getting a ClosingPanel event, befor the main window is getting the FormClosing event and they use all the same CancelEventArgs argument. So our panel controllers can listen to the ClosingPanel event and set the Cancel property to false, e.g. when unsaved changes are present. I can not help you with WPF related problems offhand since i lack in WPF experience. – Effluvium

This is WinForms but I am using a free DockPanel suite. I am having to listen to the OnFormClosing event for the relevant docked form and then deal with the request depending on the context (is the application closing, is the user merely closing a form etc.). Thanks very much for your reply but it sounds like you are doing what I expected I will need to do. I have written a DocumentManager that caches the controllers and manages closures and opening of documents etc. Of course the controller now have their own TryClose and CanClose methods (from interfaces) which I use to do the checks – Continence

Answer 2

First I must say I don't work with WinForms, but I think there are some problems in your implementation. First, this block

private IDocumentController GetDocumentController(string path)
{
    return return CompositionRoot.ResolveAll<IDocumentController>()
        .SingleOrDefault(provider => provider.Handles(path));
}

Indicates that there may be multiple IDocumentControllers registered in your container. Now notice that SpreadsheetController takes ISpreadsheetView in constructor. It means that when this controller is resolved, SpreadsheetView is resolved and constructed, and that view is UI control which might be expensive to construct. Now imagine you have 20 IDocumentControllers registered. When the above code (GetDocumentController) is executed, they are ALL resolved and 20 UI controls are constructed, of which 19 are then immediatly discarded.

This is not good and indicates you don't need to take instance of a view in that controller's constructor. Instead, you need a way to create that instance when needed, and this leads us to factory pattern. Create ISpreadsheetViewFactory (or even IDocumentViewFactory) which will create instances of IDocumentViews for you. Something like this:

interface IDocumentViewFactory {
    ISpreadsheetView Create(IDocumentController controller);
}

And implementation

class DocumentViewFactory : IDocumentViewFactory {
    public ISpreadsheetView Create(IDocumentController controller) {
        return new SpreadsheetView(controller);
    }
}

Then, register this factory in your container, change constructor of SpreadsheetView, remove SetContainer method, and change constructor of SpreadsheetController to accept IDocumentViewFactory. Then, do not create view directly in controller constructor, because see above - this will create potentially many UI controls for nothing. Instead, use Lazy pattern and instantiate SpreadsheetView only when needed (using factory).

As for your second question - yes, you use your container as service locator in your GetDocumentController. If you want to avoid this, use Multi Injection and inject array of IDocumentController into your main view's constructor.

Reservation