A classic rule to ahere to in MVC is to have a one-to-one relationship between Views and ViewModels. Sometimes it is tempting to share ViewModels between Views in the name of efficiency, but the long term costs will outweigh any short term gains. The following is an explanation of one such situation.
As always, the code in this example has been recreated to illustrate the issue under discussion. I never post any of my client's code. This is a simplified version of the actual situation.
The Problem
Recently I had to modify an ASP.Net MVC Razor View that listed a Customers. The View resembled this:
@using Lucidity.SampleSystem.Enumerations
@model IEnumerable<Lucidity.SampleSystem.ViewModels.CustomerSummaryViewModel>
@{
ViewBag.Title = "Customers";
}
<h2>Customers</h2>
<table>
<tr>
<th>
Customer
</th>
<th>
Registered On
</th>
<th>
Contract Value
</th>
<th></th>
</tr>
@foreach (var customer in Model) {
string customerDescription = null;
if (customer.Type == CustomerType.Individual)
{
customerDescription =
customer.Title + " " +
customer.GivenName + " " +
customer.FamilyName;
}
else if(customer.Type == CustomerType.Corporate)
{
customerDescription = customer.CompanyName;
}
<tr>
<td>
@customerDescription
</td>
<td>
@Html.DisplayFor(modelItem => customer.RegisteredOn)
</td>
<td>
@Html.DisplayFor(modelItem => customer.ContractValue)
</td>
<td>
@Html.ActionLink("Edit", "Edit",
new { id = customer.Id },
new { aria_label = "Edit " + customerDescription }) |
@Html.ActionLink("Details", "Details",
new { id = customer.Id },
new { aria_label = "Details for " + customerDescription }) |
@Html.ActionLink("Delete", "Delete",
new { id = customer.Id },
new { aria_label = "Delete " + customerDescription })
</td>
</tr>
}
</table>
SyntaxHighlighter is doing some funny things here - this is best viewed in plain text.
This offending code lies between lines 26 and 38. The issue is the presence of logic in the View. This is bad practice for numerous reasons:
- This is not how ASP.Net MVC is intended to be used, and so causes confusion for future maintainers of the code.
- The View is difficult to test. Yes, there are options, but it is far simpler for this logic to be in a ViewModelFactory, and for that to be unit tested. And in the case of testing, if it's simpler, it's more likely to be done.
- There are already various places for logic to be spread around, rightly or wrongly (Domain, ViewModelFactory, Controller). We don't need another one.
Why was This Decision Made?
So why would a developer have done this? It does not appear to be the work of an incompetent or complacent developer.
The rest of the code base would suggest that the team had a good grasp of ASP.Net MVC, and would have known this was not standard practice.
One thing that caught my eye was the @model for the View:
@model IEnumerable<Lucidity.SampleSystem.ViewModels.CustomerSummaryViewModel>
It doesn't look like this ViewModel has been designed specifically for this View. In fact, it is used on a PartialView called _CustomerSummary that is displayed at the top of many of the pages.
Standard practice would be for this sort of logic calculation to be performed in a ViewModelFactory, and set a field in the ViewModel. What is odd is that a field called 'Description' does actually exist in the ViewModel:
namespace Lucidity.SampleSystem.ViewModels
{
public class CustomerSummaryViewModel
{
public Guid Id { get; set; }
public string Title { get; set; }
public string GivenName { get; set; }
public string FamilyName { get; set; }
public string CompanyName { get; set; }
public CustomerType Type { get; set; }
public DateTime RegisteredOn { get; set; }
public decimal ContractValue { get; set; }
public string Description { get; set; }
}
}
However, this field was already being set in a ViewModelFactory for the _CustomerSummary View, but it's just a simple concatenation - no logic to display CompanyName for corporate customers.
The Background Cause
When the requirement came for a list of customers, a design decision was made to reuse the CustomerSummaryViewModel for the new View. This would have seemed like a good idea at the time: The fields to be displayed in the Customer list were just a subset of those displayed in the CustomerSummary. The Description field was always the same for both. So the new View was bound to the existing ViewModel, and the new controller used the Existing ViewModelFactory to generate the ViewModel from the list of Customer entities. A search through the source control history confirms this - the requirements were once aligned.
The problem with this approach comes when the a change is requested for either of the Views that are bound to that ViewModel. In this case, a new requirement was raised for the Company name to be displayed for corporates. Where does this new logic go? The ViewModelFactory doesn't know if it is creating a CustomerSummaryViewModel for _CustomerSummary or the Index. It would have been possible, perhaps, to pass in a flag to the ViewModelFactory, telling it what it was creating it for, and it could then make a decision accordingly. But it is likely time constraints caused the developer to take this approach.
We are all capable of hacky behaviour like this, if pushed down a dead end and when the pressure's on. The trick is not to push oursleves or our fellow develeopers into such dead ends. Keeping to the rule of one View per ViewModel is one such way of avoiding such dead ends.
Sources
http://lostechies.com/jimmybogard/2009/06/30/how-we-do-mvc-view-models/
http://lostechies.com/jimmybogard/2013/07/17/how-we-do-mvc-4-years-later
http://stackoverflow.com/questions/9413034/viewmodel-per-view-or-per-model
