Why You Should Have One ViewModel Per View

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

Entities Shouldn't Have Getters

This post follows on from Entities Shouldn't Have Setters.

The principle of Tell, Don't Ask states that one of the consequences of an object having getters is that there is often the temptation for another object to read the values from the getters, make a decision based on those values, and then update the object using the setters (as covered in the last post). A more Object-Oriented design would be to simply 'tell' the object what you want to do, and allow it to make the decision based on its internal state.

However, as acknowledged by Martin Fowler, it is very often the case that we need to display the state of an entity to a UI. However, the use of getters in this case can be avoided by displaying a persistent view model instead of the entity. The persistent view model is what is displayed to the user, and it is updated whenever the entity changes. This is the basic principle behind CQRS.

Imagine our Post object from our blog example in the last post. If we were to have an Edit use case for this, the traditional series of events might be:

• Call Edit on the Post entity with the new content of the post.
• Save the Post entity.
• Load the new Post entity from the database.
• Display the various properties of the Post entity by accessing its getters.

With CQRS, it might look more like this:

• Call Edit on the Post entity with the new content of the post.
• The Post entity raises a PostEdited domain event containing the new values.
• The application layer handles the domain event, saves the entity and publishes an event on NServiceBus containing the new values.
• The read model subscribes to this event, and updates the persistent read model accordingly.
• When the UI is refreshed, it displays the properties of the persistent view model by accessing its getters.

This way we have avoided using the getters of the entity.

For more information on publishing domain events, read This post by Udi Dahan.

So I should immediately stop using getters and setters on my entities?

No, this is somewhat ideological. The first issue I have come across is that it is often desirable to use getters and setters when testing. Getters are useful for the asserts, and setters are useful because for many tests, we do not need to set up the whole entity. Maybe that is breaking some testing ideology, but in the real world, sometimes we need to be able to quickly set up entities without using their use cases.

And of course, CQRS is not always appropriate for every application.

So what was the point of these posts? The point is that you should understand that it can be done, and why and when you may choose to do it. This way you can apply these rules where it makes sense.

So it is unlikely I will be writing entities without getters or setters for now, but there are some rules to take from this:

• If you are calling getters or setters of an entity from another entity in your domain, you are probably doing something wrong.
• If you are calling getters or setters of an entity in your application layer you are probably doing something wrong (unless you are mapping to a DTO).
• If you are calling setters of an entity in your UI you are almost certainly doing something wrong.

Entities Shouldn't Have Setters

Setters on entities are redundant and a code smell. Take the following example of a blog post:

public class Post : Entity<Guid>
{
    private DateTime _postedOn;
    private Blog _blog;
    private string _text;

    public virtual DateTime PostedOn
    {
        get { return _postedOn; }
        set { _postedOn = value; }
    }

    public virtual Blog Blog 
    {
        get { return _blog; }
        set { _blog = value; }
    }

    public virtual string Text
    {
        get { return _text; }
        set { _text = value; }
    }
}

The problem here is that is that it leaves open the posibility that the entity will be modified from another entity, or even from another layer. Take this example in the application layer:

public class PostService : IPostService
{ 
    public void Compose(ComposePostRequest request)
    {
        var post = new Post();
        post.PostedOn = DateTime.Now;
        post.Blog = _blogRepository.GetById(request.BlogId);
        post.Text = request.Text;
        _postRepository.Save(post);
    }
}

In this example, the Post entity is being constructed in the application layer. The main problem with this is that inevitably business logic seeps into the application layer. What if we had to set an expiry date for the post based on a window set in the blog? Where does this logic go? The result may look like this:

public class PostService : IPostService
{ 
    public void Compose(ComposePostRequest request)
    {
        var blog = _blogRepository.GetById(request.BlogId);
        var post = new Post();
        post.PostedOn = DateTime.Now;
        post.Blog = blog;
        post.Text = request.Text;
        post.ExpiryDate = post.PostedOn.AddDays(blog.PostExpiryWindow);
        _postRepository.Save(post);
    }
}

As more logic comes in, more will be added to the application layer. The logic of the domain cannot be tested in isolation. The only way to test this logic is to mock out the PostRepository and capture the Post that is being saved. A far better way is to do this:

public class Post : Entity<Guid>
{
    private DateTime _postedOn;
    private Blog _blog;
    private string _text;
    private DateTime _expiryDate

    public virtual DateTime PostedOn
    {
        get { return _postedOn; }
    }

    public virtual Blog Blog 
    {
        get { return _blog; }
    }

    public virtual string Text
    {
        get { return _text; }
    }

    public virtual DateTime ExpiryDate
    {
        get { return _expiryDate; }
    }

    public static Post Compose(Blog blog, string text)
    {
        var post = new Post();
        post._postedOn = DateTime.Now;
        post._blog = blog;
        post._text = text;
        post._expiryDate = _postedOn.AddDays(blog.PostExpiryWindow);
        return post;
    }
}

public class PostService : IPostService
{ 
    public void Compose(ComposePostRequest request)
    {
        var blog = _blogRepository.GetById(request.BlogId);
        var post = Post.Compose(blog, request.Text;
        _postRepository.Save(post);
    }
}

Now new business logic can be added to the Compose method in Post, and the logic of Post can be tested in isolation. If you think about it, this all makes perfect sense: Compose is a use case of Post, and why would you want to change the state of an entity out side of a use case?

Persistence

Again, my persistence example is with NHibernate. NHibernate allows us to specify the way it accesses properties, using the 'access' attribute of property:

<?xml version="1.0" encoding="utf-8" ?>
<hibernate-mapping xmlns="urn:nhibernate-mapping-2.2"
                   namespace="Lucid.Domain.Entities"
                   assembly="Lucid.Domain">
  <class name="Post" table="`Post`">
    <id name="Id" column="Id" type="guid">
      <generator class="assigned"/>
    </id>
    <property name="_postedOn" column="`PostedOn`" access="field" />
    <many-to-one name="_blog" class="Bus" column="`BusId`" cascade="save-update" access="field" />
    <property name="_text" column="`Text`" access="field" />
    <property name="_expiryDate" column="`ExpiryDate`" access="field" />
  </class>
</hibernate-mapping>

The blog and post example may not have been the best example here, as Blog would be an ideal candidate for an aggregate route. In this case, the Blog entity would have a ComposePost method. The Blog would be loaded, the Post would be added and the Blog would be saved. However, aggregate routes are for another post, and for now this serves as a good example.