ASP.NET MVC ViewModel Pattern

This looks very similar to the recommended practice in the Wrox Professional ASP.NET MVC book, the first chapter of which is available for free from the above URL.

Starting on page 100 they have a section on ViewData and ViewModels.

When a Controller class decides to render an HTML response back to a client, it is responsible for explicitly passing to the view template all of the data needed to render the response. View templates should never perform any data retrieval or application logic – and should instead limit themselves to only have rendering code that is driven off of the model/data passed to it by the controller.

[...]

When using [the "ViewModel"] pattern we create strongly-typed classes that are optimized for our specific view scenarios, and which expose properties for the dynamic values/content needed by our view templates. Our controller classes can then populate and pass these view-optimized classes to our view template to use. This enables type-safety, compile-time checking, and editor intellisense within view templates.

Taken from "Chapter 1 "Nerd Dinner" from Professional ASP.NET MVC 1.0 written by Rob Connery et al published by Wrox". The original is available at http://tinyurl.com/aspnetmvc

There are couple of things that bother me.

1. The terminology. ViewModel is this case is a simple view data that is populated and later consumed by controller. View knows nothing about controller as ASP.NET MVC infrastructure is responsible for selecting controllers and appropriate actions. Controller handles user interaction. I think it looks more like Passive View than ViewModel (I assume that by ViewModel you mean Model-View-ViewModel pattern).

2. The details. The controller that populates view data is not supposed to know the details of how the view is implemented. However OrganisationViewModel.Country discloses unnecessary details (SelectListItem is pure view implementation detail). Thus making controller dependent on view implementation details. I think it should be changed to avoid it. Consider using some object that will hold the data for a country.

Hope this helps.

In general, I think it looks good, and it's usually a good idea to create viewmodels for your domain objects.

I haven't looked at every single line of code, but one thing that caught my attention was the constructors of OrganisationViewModel. I'd rewrite it using:

public OrganisationViewModel() : this(new Organisation()) { }public OrganisationViewModel(Organisation o){  Organisation = o;  InitCollections();}

This removes some duplicate code, as you don't have to call InitCollections() in both constructors. Of course, this is just a minor detail, and has nothing to do with the general idea.