This project is read-only.

IPresenterFactory is flawed

Jan 21, 2016 at 9:10 PM
hi,
I found this framework while looking for an MVP framework for my next WinForms little project. However, as I prefer using Simple Injector as my DI container, I've set to implement IPresenterFactory using it (I forked the project to a branch with that name). When I came to implement the Create method, looking at the other implementations using other DI containers, I realized it expects the factory to inject the provided viewInstance to a parameter named view.

I think this design is flawed. You in fact expect the factory to do the job of the DI container. In addition, you force the user to name the IView parameter view. I can easily see why would someone want to name his IMainView : IView as mainView. Simple Injector, which in general promotes good design practices, does not support arbitrary parameter injection to the services it retrieves, without writing some plugin to replace its default behavior.

Looking at your unit test as an example, the consumer should register View1 as an implementation of IView with the DI container, and so, when BasicPresenter is requested, the DI container should satisfy its dependency on IView using a View1 instance. That's what Dependency Injection does.

My implementation will expect the user to register any interface inheriting IView with a type which implements it, and will generally ignore the provided viewType and viewInstance. I will, however, add code who's sole purpose is to satisfy the unit tests.
Jan 21, 2016 at 10:13 PM
I just completed writing the unit test for Simple Injector. Having configured it to supply View1 for IView parameters, it simply worked without further special code. In other words, my implementation completely ignores the viewType and viewInstance arguments to Create.
Jan 24, 2016 at 6:56 AM
Edited Jan 24, 2016 at 6:57 AM
Hi tsahiasher,

Thanks for checking out my project. And thanks for your feedback.


Whilst I do not agree that the design of the usage of IPresenterFactory is flawed, I do agree that the PresenterFactories for Ninject and StructureMap should probably have been written without a requirement that the IView parameter to the Presenter’s constructor be named "view" .

If you look at the UnityPresenterFactory, there is no such requirement and you can name the IView parameter anything you like.

I disagree that the IPresenterFactory is doing the job of the DI Container. Firstly, we should be referring to an Inversion of Control container (IOC), rather than DI container. The control of resolving the dependency is delegated to the IOC. The factory is handling the injection of the dependency. If you look at this post by Mark Seemann http://blog.ploeh.dk/2014/05/19/di-friendly-framework/ , you will see that it is not poor practice to use an IOC inside a factory which has the responsibility of returning to the framework the instantiated object. If you look at the class "WindsorCompositionRoot" in that article, you will see the example I am referring to (in that case, it is pertinent to the ASP.NET MVC framework).

Going back to the Ninject example, you could probably rewrite the Create method as follows:
        public virtual IPresenter Create(Type presenterType, Type viewType, IView viewInstance)
        {
            if (presenterType == null)
                throw new ArgumentNullException("presenterType");
            if (viewType == null)
                throw new ArgumentNullException("viewType");
            if (viewInstance == null)
                throw new ArgumentNullException("viewInstance");

            Kernel.Bind<IView>().ToConstant(viewInstance).InTransientScope();
            Kernel.Bind<IPresenter>().To(presenterType).InTransientScope();

            var presenter = Kernel.Get<IPresenter>();
            
            Kernel.Unbind<IPresenter>();
            Kernel.Unbind<IView>();

            return presenter;
        }
As you can see, there is now no reliance on the IView being named "view". I haven’t thoroughly tested that, but it passes the units tests.

I’ll make a quick comment on some other DI issues.
When you create your bindings for all of your services, you should not do that in the IPresenter factory. That IPresenter factory’s responsibility is solely to instantiate the relevant Presenter for the relevant IView. Your other services, which deliver data and process business rules, should be bound in a separate place.

So, if we take a simple example, the Program class’s Main method might look something like this:
 [STAThread]
 static void Main()
 {
     Application.EnableVisualStyles();
     Application.SetCompatibleTextRenderingDefault(false);

     IKernel kernel = new KernelFactory().CreateKernel();// in here, bind all your services/dependancies etc.       
     PresenterBinder.Factory = new NinjectPresenterFactory(kernel); 

     var mainForm = new MainForm();
     Application.Run(mainForm);
 }
The last, and probably most important, thing to note is that you will need to manage how and when your IOC disposes of objects. Resolving objects with an IOC is easy. But making decisions on their lifetime can get tricky. Especially in a stateful environment like Winforms. It is easier with web to scope things to the lifetime of an HttpRequest. But with desktop development, you need to be careful not to create memory leaks.
So, a tool to profile your memory is a good idea, if you can get access to one. I know Redgate's memory profiler has a 30 day trial, so it would be a good idea to make sure your Services and other dependencies are being disposed of when you expect them to.

All the best.
Jan 27, 2016 at 10:06 PM
After doing some more experiments with implementing IPresenterFactory, I must take some of my words back. What I ended up doing to overcome Simple Injector lack of support for arbitrary parameters is to instantiate the requested presenter by invoking its constructor using reflection, providing the viewInstance to the parameter typed as viewType, and requesting Simple Injector for instances of the other parameters. Even with its extensibility, it doesn't look like I can pass a concrete instance to instantiate a parameter, or it will require a fair amount of code to achieve this.

I'll push my code to the fork as soon as I figure out how. My first time working with Git.