ViewModel creator design

Question

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

Answer 1

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.

Answer 2

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.