ViewModel Implementation in ASP.NET MVC - Is this code best practice?

Question

I've just started to get into using ViewModels. Can you guys check out this code to see if I'm following best practice? Is there anything out of the ordinary? Would you do the validation differently?

Sorry if code is lengthy (there's so many parts to it). I've tried to make it as easy to understand as possible.

Thanks!

Model

  public class CustomerModel
  {
    [Required(ErrorMessage="Primer nombre!")]
    public string FirstName { get; set; }

    [Required(ErrorMessage="Segundo nombre!")]
    public string LastName { get; set; }

    [Required(ErrorMessage="Edad")]
    public int? Age { get; set; }

    public string State { get; set; }
    public string CountryID { get; set; }

    [Required(ErrorMessage="Phone Number")]
    public string PhoneNumber { get; set; }
  }

ViewModel

  public class CustomerViewModel
  {
    public CustomerModel Customer { get; set; }

    public string Phone1a { get; set; }
    public string Phone1b { get; set; }
    public string Phone1c { get; set; }
  }

Controller

    public ActionResult Index()
    {
      CustomerViewModel Customer = new CustomerViewModel()
      {
        Customer = new CustomerModel(),
      };


      return View(Customer);
    }


    [HttpPost]
    public ActionResult Index(CustomerViewModel c)
    {

      //ModelState.Add("Customer.PhoneNumber", ModelState["Phone1a"]);

      // Let's manually bind the phone number fields to the PhoneNumber properties in
      // Customer object. 
      c.Customer.PhoneNumber = c.Phone1a + c.Phone1b + c.Phone1c;

      // Let's check that it's not empty and that it's a valid phone number (logic not listed here)
      if (!String.IsNullOrEmpty(c.Customer.PhoneNumber))
      {
        // Let's remove the fact that there was an error! 
        ModelState["Customer.PhoneNumber"].Errors.Clear();
      } // Else keep the error there. 

      if (ModelState.IsValid)
      {
        Response.Write("<H1 style'background-color:white;color:black'>VALIDATED</H1>");
      }
      return View("Index", c);
    }

  }

View

@model MVVM1.Models.CustomerViewModel

@using (Html.BeginForm("Index", "Detail"))
{  
  <table border="1" cellpadding="1" cellspacing="1">
    <tr>
      <td>@Html.LabelFor(m => m.Customer.FirstName)</td>
      <td>
        @Html.TextBoxFor(m => m.Customer.FirstName)
        @Html.ValidationMessageFor(m => m.Customer.FirstName)
      </td>
    </tr>
    <tr>
      <td>@Html.LabelFor(m => m.Customer.LastName)</td>
      <td>
        @Html.TextBoxFor(m => m.Customer.LastName)
        @Html.ValidationMessageFor(m => m.Customer.LastName)
      </td>
    </tr>
    <tr>
      <td>@Html.LabelFor(m => m.Customer.Age)</td>
      <td>
        @Html.TextBoxFor(m => m.Customer.Age)
        @Html.ValidationMessageFor(m => m.Customer.Age)
      </td>
    </tr>

    <tr>
      <td>@Html.LabelFor(m => m.Customer.PhoneNumber)</td>
      <td width="350">
        @Html.TextBoxFor(m => m.Phone1a, new { size="4", maxlength="3" })
        @Html.TextBoxFor(m => m.Phone1b)
        @Html.TextBoxFor(m => m.Phone1c)
        <div>
        @Html.ValidationMessageFor(m => m.Customer.PhoneNumber)
        </div>
      </td>
    </tr>
    <tr>
      <td></td>
      <td>
        <input type="submit" value="Submit" /></td>
    </tr>
  </table>
}

Corroborant

Answer 1

One thing that jumps out at me is this:

  if (ModelState.IsValid) 
  { 
    Response.Write("<H1 style'background-color:white;color:black'>VALIDATED</H1>"); 
  } 
  return View("Index", c); 

Remember that view models are good for passing data to your controller AND back to your model. I recommend you add an IsValid property to your view model and then setting that to true instead of calling Response.Write. Then simply add this to the top of your partial view:

@if (Model.IsValid)
{
    <H1 style'background-color:white;color:black'>VALIDATED</H1>
}

You can also get to ModelState in your view but some would argue that isn't a best practice. However, if you don't want to add a property to your model for something you can just see in your view you can just do this:

@if (ViewData.ModelState.IsValid)

Another nitpicky thing is that MVC validation attributes are typically used for validation on the UI. This validation can be reused in other areas but in some cases is sub-optimal. Also, you may not always be able to modify your domain models. Therefore, to keep all of my UI validation in one place I usually wrap my domain models in my view models so you get something like this:

public class CustomerViewModel                      
{                      
    public CustomerModel Customer { get; set; }

    [Required(ErrorMessage="Primer nombre!")]                        
    public string FirstName
    {
        get { return Customer.FirstName; } 
        set { Customer.FirstName = value; }
    }
...

This may seem redundant and isn't always worth the effort but it is a good practice to consider when using Entity Framework domain models or other classes which are difficult or impossible to modify.

Hegarty

Really good points. Good idea about creating an IsValid property. – Corroborant

Answer 2

I'm just getting the hang of MVC myself, but I researched this same topic yesterday and came to the conclusion that one should not directly include a model object in the ViewModel. So my understanding is that it would be a bad practice to include your CustomerModel directly in the CustomerViewModel.

Instead, you want to list out each of the properties from CustomerModel that you want to include in your ViewModel. Then you either want to manually map the data from CustomerModel to the CustomerViewModel or use a tool like AutoMapper which does it automatically with a line of code like this inside of your action method:

public ViewResult Example()
{
    // Populate/retrieve yourCustomer here
    Customer yourCustomer = new CustomerModel();

    var model = Mapper.Map<CustomerModel, CustomerViewModel>(yourCustomer);

    return View(model);
}

In this case, Mapper.Map will return a CustomerViewModel that you can pass to your View.

You will also need to include the following in your Application_Start method:

Mapper.CreateMap<CustomerModel, CustomerViewModel>();

In general I found AutoMapper pretty easy to get to work. It's automatic when the field names match, if they don't or you have a nested Object, you can specify those mappings in the CreateMap line. So if your CustomerModel uses an Address object instead of individual properties, you would do this:

Mapper.CreateMap<CustomerModel, CustomerViewModel>()
    .ForMember(dest => dest.StreetAddress, opt => opt.MapFrom(src => src.Address.Street));

Please anyone correct me if I'm wrong as I'm just getting my head around MVC as well.

Designation

Answer 3

I would say your ViewModel implementation is pretty standard. You are using the ViewModel to act as the intermediate object between your View, and your Domain Model. Which is good practice.

The only thing I'd be weary about is how you handle Model errors, and also your ViewModel should have some attributes. For instance, you might want to use the RegularExpressionAttribute Class:

  public class CustomerViewModel
  {
    public CustomerModel Customer { get; set; }

    [RegularExpression(@"^\d{3}$")]
    public string Phone1a { get; set; }
    [RegularExpression(@"^\d{3}$")]
    public string Phone1b { get; set; }
    [RegularExpression(@"^\d{4}$")]
    public string Phone1c { get; set; }
  }

Depressive

In the end this usually never works out. Models almost always end up having data you don't want to display to the user. You now can't just do editor for models, and you run the risk of over posting and under posting on submits. I know it's a lot of mapping, but I've found it's usually better to have 'simple' view models – Fafnir

Good idea on using additional RegExp attributes. – Corroborant

Does the CustomerModel actually need annotations to check validity at all? I think only the ViewModel needs the Requirement attributes. – Warren