Cleaning up ASP.NET MVC Controllers

I've been working with ASP.NET MVC on a few different projects now, and yet I've never been happy with my controllers. My view models were normally pretty simple, but my controllers, especially when saving, were starting to feel like spaghetti.

Let me give you a typical example. Below is a controller for editing a user with first name, last name and country. You can see the full Gist for the details, but the controller looked like this:

public ActionResult Edit(string id)
{
    var user = string.IsNullOrEmpty(id) ? session.Find<User>(id) : new User();
    var countries = session.FindAll<Country>();

    var model = EditUserModel.CreateFrom(user, countries.Select(c => CountryModel.CreateFrom(c)).ToList());
    return View(model);
}

[HttpPost]
public ActionResult Edit(EditUserModel model)
{
    if (!ModelState.IsValid)
    {
        // Since the posted data won't contain the country list, we have to re-fill the model. This code feels hacky
        var countries = session.FindAll<Country>();
        model.Countries = countries.Select(c => CountryModel.CreateFrom(c)).ToList();
        return View(model);        
    }

    var user = string.IsNullOrEmpty(model.Id) ? session.Find<User>(model.Id) : new User();
    session.Store(user);

    user.FirstName = model.FirstName;
    user.LastName = model.LastName;
    // Messy handling for referenced entity
    user.Country = session.Find<Country>(model.CountryId);

    session.SaveChanges();
    return RedirectToAction("Index");
}

Yuck! So tonight, I started looking for ways to clean it up. In researching, I came across a suggestion by Chuck Norris (yes, THE Chuck Norris!) that involved using an intermediary object. I posted a Gist with the pattern applied to my first scenario. Now, my controller was much simpler:

class UserController : Controller
{
    IModelBuilder<EditUserModel, User> builder = new EditUserModelBuilder();

    public ActionResult Edit(string id)
    {
        var user = session.Find<User>(id) ?? new User();
        return View(model, builder.CreateFrom(user));
    }

    [HttpPost]
    public ActionResult Edit(EditUserModel model)
    {
        if (!ModelState.IsValid)
        {
            return View(builder.Rebuild(model);       
        }

        builder.ApplyChanges(model);
        session.SaveChanges();
        return RedirectToAction("Index");
    }
}

This was achieved by outsourcing the shaping and saving to a 'builder' class. Here is how the builder looked:

class EditUserModelBuilder : IModelBuilder<EditUserModel, User>
{
    readonly ISession session;

    public EditUserModelBuilder(ISession session)
    {
        this.session = session;
    }

    public EditUserModel CreateFrom(User user)
    {
        var model = new EditUserModel();
        model.Id = user.FirstName;
        model.FirstName = user.FirstName;
        model.LastName = user.LastName;
        model.Country = user.Country.Id;
        model.Countries = GetCountries();
        return model;
    }

    public EditUserModel Rebuild(EditUserModel model)
    {
        model.Countries = GetCountries();
        return model;
    }

    public User ApplyChanges(EditUserModel model)
    {
        var user = string.IsNullOrEmpty(model.Id) ? new User() : session.Find<User>(model.Id); 
        session.Store(user);

        user.FirstName = model.FirstName;
        user.LastName = model.LastName;
        user.Country = session.Find<Country>(model.CountryId);
        return user;
    }

    ICollection<SelectListItem> GetCountries()
    {
        var countries = session.FindAll<Country>();
        return countries.Select(c => new SelectListItem { Value = c.Id, Text = c.Name }).ToList();
    }
}

After I posted that Gist, Jay McGuinness pointed out that at his company, they use a slightly different technique: they seperate the "read" part of the builder from the "write" part by introducing the command pattern. I posted an example Gist, again applied to my first scenario. The command looks like this:

class UserEditCommand : IModelCommand<UserEditModel>
{
    readonly ISession session;

    public UserEditCommand(ISession session)
    {
        this.session = session;
    }

    public void Execute(UserEditModel model)
    {
        var user = string.IsNullOrEmpty(model.Id) ? new User() : session.Find<User>(model.Id); 
        session.Store(user);

        user.FirstName = model.FirstName;
        user.LastName = model.LastName;
        user.Country = session.Find<Country>(model.CountryId);

        session.SaveChanges();

        // Auditing and other interesting things can happen here
    }
}

Note that the command and builder take two different parameters. This is achieved by splitting the view model into two and using inheritance:

class UserEditModel
{
    public string Id { get; set; }
    public string FirstName { get; set; }
    public string LastName { get; set; }
    public string CountryId { get; set; }
}

class UserViewModel : UserEditModel
{
    public ICollection<SelectListItem> Countries { get; set; }
}

What's great about this is that the view model can add additional view-specific properties, while the command only depends on the basic information needed to save details.

I like this approach for a few reasons:

1. The controller is more readable
2. The builder and command can both take dependencies - e.g., the UrlHelper or repositories - without filling up the controller
3. The commands become a great place to put auditing and permission checking code

Thanks to Jay and Chuck for these suggestions, I certainly feel a lot better about my MVC controllers going forward.

The full Gist of the final solution is below:

This page has been translated into Spanish language by Maria Ramos from Webhostinghub.com/support/edu.