7
\$\begingroup\$

I am trying to move some logic outside my controller for creating my view models for the view.

I have a lot of queries to fire to make sure the complete view model is ready, so to keep my controllers thin I came up with this design - any feedback, comments or suggestions are welcome!

The interfaces looks as following:

public interface IViewModel
{
    // Marker interface
}

public interface IHandleViewModel<TViewModel> where TViewModel : IViewModel
{
    Task<TViewModel> Handle();
}

public interface IProcessViewModels
{
    Task<TViewModel> Create<TViewModel>() where TViewModel : IViewModel, new();
}

The implementation of the IProcessViewModels is using dependency injection (Simple Injector) to find the correct handler for the view model:

internal sealed class ViewModelProcessor : IProcessViewModels
{
    private readonly Container _container;

    public ViewModelProcessor(Container container)
    {
        _container = container;
    }

    public Task<TViewModel> Create<TViewModel>() where TViewModel : IViewModel, new()
    {
        var handlerType = typeof(IHandleViewModel<>).MakeGenericType(typeof(TViewModel));
        dynamic handler = _container.GetInstance(handlerType);
        return handler.Handle();
    }
}

The setup is done in the Composition Root using this extension:

 public static void RegisterViewModels(this Container container, Assembly[] viewModelAssemblies)
    {
        if (container == null)
            throw new ArgumentNullException("container");

        if (viewModelAssemblies == null) 
            throw new ArgumentNullException("viewModelAssemblies");

        container.RegisterSingle<IProcessViewModels, ViewModelProcessor>();
        container.RegisterManyForOpenGeneric(typeof(IHandleViewModel<>), viewModelAssemblies);

        container.RegisterSingleDecorator(
            typeof(IHandleViewModel<>),
            typeof(ViewModelLifetimeScopeDecorator<>)
        );
    }

The usage

public HomeViewModel : IViewModel {

    public string PageTitle { get; set; }

    public IList<TodoItem> TodoItems { get; set; }

}

public HandleHomeViewModel : IHandleViewModel<HomeViewModel> {

    private readonly IProcessQueries _queries; // CQRS

    public HandleHomeViewModel(IProcessQueries queries) {
            _queries = queries;
    }

    public async Task<HomeViewModel> Handle() {
        var model = new HomeViewModel {
             PageTitle = "This is the page title";
             TodoItems = await _queries.Execute(new GetTodoItemsFromDatabase()); // for demo purpose
        }

        return model;
    }

}

public class HomeController : Controller
{
    private readonly IProcessViewModels _models;

    public HomeController(IProcessViewModels models)
    {
        _models = models;
    }

    public async Task<ActionResult> Index()
    {
            var model = await _models.Create<HomeViewModel>();
            return model;
    }
}
\$\endgroup\$

2 Answers 2

13
\$\begingroup\$

I would say your design is fine. In general I advise injecting IHandleViewModel<T> implementations directly into consumers, because this makes it easier to verify the object graph directly and makes it clearer what the consumer actually depends on. You should do this, unless you regularly inject multiple IHandleViewModel<T>s into the same consumer, while you are pretty sure that these consumers are NOT violating the Single Responsibility Principle.

But if you decide to inject the ViewModelProcessor mediator, I advice to add a single unit/integration test to the system that verifies whether there is an IHandleViewModel<T> implementation for each view model in the system. This prevents you from getting an exception at runtime because an implementation is missing.

The IViewModel marker interface gets useful when writing such unit test, because this interface allows you to find all view models easily as follows:

// Arrange
var viewModelHandlerTypes =
    from assembly in AppDomain.CurrentDomain.GetAssemblies()
    from type in assembly.GetTypes()
    where typeof(IViewModel).IsAssignableFrom(type)
    where !type.IsAbstract && !type.IsGenericTypeDefinition
    select typeof(IHandleViewModel<>).MakeGenericType(type);

// Act
viewModelHandlerTypes.ToList().ForEach(container.GetInstance);

About Async Programming

A last suggestion I would like to make is to get rid of the complete async programming model. Asynchronous methods tend to spread through your application like a virus and make both programming and debugging your application much harder. Yes, this asynchronous programming has become WAY easier than it used to be, but it is STILL harder than synchronous programming and it will probably stay harder untill the .NET runtime has been rewritten from the ground up (if that's even possible).

I know this is against popular opinion, but there is hardly ever a reason to polute your entire code base with this asynchronous programming model. Main reason for Microsoft to push this programming model really hard is because it is more efficient when running in the cloud. This makes sense, because in Azure, you pay per CPU cycle and per the number of machines you need. But on the other hand, asynchronous programming costs way more developer cycles, and because developers are quite expensive, it is quite unlikely that your savings on the Azure bills will actually compensate the extra developer costs. But obviously, you will have to do the math yourself.

Don't get me wrong, of course we want -and need- responsive UIs so we might need a few async/await calls inside your Window, Page or View Model classes in our presentation layer built with WPF, Silverlight, Win Forms or some other client technology. You can still do this, even though your whole code base below is synchronous, with synchonous calls to the database, web services and the file system. When doing that, you will still be able to make your UI responsive, but the only difference is that you'll have a background thread sleeping most of the time, instead of using I/O completion ports. But I've never ever worked on an application where having this single extra background thread was a problem. Even for Windows Phone applications this is a non-issue.

But since you're building an MVC application, don't bother in making your controller code asynchronous, the user's browser will wait anyway, even if you make your controller asynchronous.

Asynchronous programming may be the new shiny thing in the .NET world, and with some training and experience, we can become quite effective as developers in applying it, but even than it is more painful than synchronous progamming (which is hard enough by itself), and instead of spending money on training developers learning how to do async, I rather spend this money in training them to learn the SOLID design principles, Test Driven Development, Functional Programming or writing clean code. There are so many other skills that are probably more important and more effective in reducing the total cost of ownership, that I rather have my money on that first.

\$\endgroup\$
13
  • 1
    \$\begingroup\$ Awesome [long-awaited I see] first post, welcome to CR! \$\endgroup\$ Mar 18, 2015 at 20:20
  • 2
    \$\begingroup\$ Hahaha, thank you @Mat'sMug. My first post on CodeReview, but I'm 'famous' on Stackoverflow for my long posts :-) \$\endgroup\$
    – Steven
    Mar 18, 2015 at 20:27
  • 1
    \$\begingroup\$ @janhartmann: about your question: "Is it fair enough to keep the IProcess* for these cases, and in simple cases only inject the dependent handler?" I would choose one or the other. Either use that IProcessViewModels mediator always, or never use it. I think this is much more consistent. But technically there's nothing wrong in mixing them. \$\endgroup\$
    – Steven
    Mar 19, 2015 at 10:22
  • 1
    \$\begingroup\$ The caveat needs to be made that sync will be faster during light load but as load increases it will slow down when there aren't enough threads available. Then eventually the queue will be full and requests will be refused. Using async allows the server to continue to be responsive under higher load. This has it's limits as well, but it means I can get more utilization from my individual servers. That's the reason for using async, not because individual requests will be faster than using sync methods - because they won't due to the overhead of async. \$\endgroup\$ Mar 20, 2015 at 14:52
  • 1
    \$\begingroup\$ @MarkJMiller: I addressed this in my post, where I say that the sync model might increase "the number of machines you need". Don't forget that we can increase the default thread pool size in ASP.NET, which allows us to have more parallel requests. This costs more memory and won't take us as far as the async model. But again you should ask yourself: do you really need this? \$\endgroup\$
    – Steven
    Mar 20, 2015 at 23:01
7
\$\begingroup\$

Why the marker interface?

public interface IViewModel
{
    // Marker interface
}

public interface IHandleViewModel<TViewModel> where TViewModel : IViewModel
{
    Task<TViewModel> Handle();
}

What you really want to do here, is ensure that TViewModel is a reference type - right? There's a generic type constraint specifically for that:

public interface IHandleViewModel<TViewModel> where TViewModel : class
{
    Task<TViewModel> Handle();
}

This is a Dependency Injection anti-pattern - lookup Service Locator:

private readonly Container _container;

public ViewModelProcessor(Container container)
{
    _container = container;
}

Your IoC container should be in one single place in your entire code base: the composition root. Passing it around as a dependency is always a Bad Idea.

That "processor" is actually a.. wait... an abstract factory.. factory.

public Task<TViewModel> Create<TViewModel>() where TViewModel : IViewModel, new()
{
    var handlerType = typeof(IHandleViewModel<>).MakeGenericType(typeof(TViewModel));
    dynamic handler = _container.GetInstance(handlerType);
    return handler.Handle();
}

And your controller is masking its true dependencies:

public HomeController(IProcessViewModels models)
{
    _models = models;
}

It's not injected with models - it's injected with a Service Locator that has the capability of giving it just about anything.

The beauty of Dependency Injection, is that when it's well implemented, constructors become a static documentation for the dependencies of every single type out there. Injecting a Service Locator destroys that, and empowers your controller with keeping the control over its true dependencies. That's what Mark Seemann describes as the Control Freak anti-pattern.

I strongly encourage you to read his book, it's a very enjoyable read.

\$\endgroup\$
4
  • \$\begingroup\$ ViewModelProcessor is practically an implementation of the mediator pattern. I usually advice injecting an IHandleViewModel<T> directly into the consumer instead of this processor, to prevent breaking the object graph, but using a mediator is fine in general. Whether or not the OP is applying the Service Locator pattern in his ViewModelProcessor depends on WHERE this class is defined. If that this class is defined INSIDE his Composition Root it is not an application of that anti-pattern, as Mark Seemann describes here. \$\endgroup\$
    – Steven
    Mar 18, 2015 at 19:30
  • \$\begingroup\$ I am not injecting IHandleViewModel<T> in the constructor, since the controller have multiple actions and I would probably end up having many. I will take note, that if I only have one dependency, I would do just that. The registering of the ViewModelProcessor is done in the composition root by the extensions method I just edited the question with. \$\endgroup\$ Mar 18, 2015 at 19:50
  • \$\begingroup\$ @Steven feel free to post your own review/answer. I've never felt the need to inject my IoC container anywhere, whatever the reason was for doing so. But I've never used Simple Injector either. \$\endgroup\$ Mar 18, 2015 at 19:57
  • 2
    \$\begingroup\$ Hi Mat, don't worry. I posted my own review :-). With your response you seem to imply that with Simple Injector you often need to inject the container, which isn't the case. I agree with you that you should never have to inject the container, into application code that is; but since the container is already highly available in your composition root, and your composition root already completely depends on the container, there is no harm into injecting the container into a class that is part of this composition root. \$\endgroup\$
    – Steven
    Mar 18, 2015 at 20:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.