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.

A picture of me

Welcome, my name is Paul Stovell. I live in Brisbane and work on Octopus Deploy Octopus Deploy, an automated deployment tool for .NET applications.

Prior to founding Octopus Deploy, I worked for an investment bank in London building WPF applications, and before that I worked for Readify, an Australian .NET consulting firm. I also worked on a number of open source projects and was an active user group presenter. I was a Microsoft MVP for WPF from 2006 to 2013.

Alan Christensen
Alan Christensen
13 Apr 2012

Nice post. I've gone through the same though process.

I tend to use AutoMapper to go from the model to the viewmodel.

For the other direction I use the command processor pattern (influenced from CQRS) which is similar to your ModelCommand. See http://codebetter.com/iancooper/2011/04/27/why-use-the-command-processor-pattern-in-the-service-layer/

Small nitpic: Execute() on EditUserCommand is a little ambiguous to me. I try to name my commands according to what they will do. E.g. for this CRUD style, it might be UpdateUserCommand. Then it is clearer to me what Execute does. It gets more useful in more intention revealing approaches e.g. CancelOrderCommand just takes an Order ID.

The other difference is I have split the command and the command handler. Not just SRP, this also works well with your auditing comment as auditing may be able to be fairly generic. Multiple command handlers per command must be supported. The auditing command handler handles (audits) all auditable commands.

14 Apr 2012

+1 on the nice post as well!

I do something very similar, though for my POST action I do something like this:

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

[HttpPost]
[ModelStateToTempData]
public ActionResult Edit(UserEditModel model)
{
    if (!ModelState.IsValid)
    {
        return RedirectToAction("Edit");       
    }

    saveCommand.Execute(model);
    return RedirectToAction("Index");
}

I'm a big fan of the Post/Redirect/Get (PRG) pattern. It allows me to only take in what I need to take in (in this case UserEditModel), redirect back to the Edit GET action if the model is invalid, and the user doesn't get that annoying "Reloading this page with resubmit any information you entered" message if they refresh the page. ModelStateToTempData is an attribute that transfers the ModelState to and from TempData so on the redirect to the Edit GET action the invalid ModelState is still available. By doing this, you wouldn't necessarily need the Rebuild() method.

14 Apr 2012

Nice one Paul - we've been meaning to write some of this up and never got around to it.

For us this is all about CQRS - more specifically about our journey to adopt CQRS and all of it's silver bullets! But it's a long road and for us (and perhaps lots of other developers) when starting a new project (or tweaking a legacy one) the intellectual and financial costs can be too high.

But I believe that by adopting this simple pattern, by separating our reads from our writes, by introducing Builders/Commands it starts us on that journey and we gain the following benefits from day one:

  • Much simpler Controllers with hardly any dependencies, rather than the 8 repositories, 2 services and 1 logger which were being injected to the constructor previously. Most of our controller actions are now 2-3 lines long, and they simply "control" the flow of data in and out of the system.
  • A clean and testable way to construct ViewModels
  • A clean and testable way to execute business logic, via Commands. Now this could result in an anaemic Domain Model, but as I say, this is a journey and we can resolve that when we get into the CommandHandlers (as Alan pointed out). It's also better to have core business knowledge in small well named Command, than buried deep in a class ending in the word "Service" or "Manager" or "Controller" - argh!!!
14 Apr 2012

A couple of other non-obvious, but HUGE benefits IMO:

  • The ability to cleanly split the workload - junior developers on the Builders - more senior guys on the Commands.
  • Logging/Auditing comes for free - it's not uncommon for developers to either not do any auditing, or to not log everything - it's easy to miss a few things. Having a CommandBase object intercept and log every call to Execute() plus perhaps the time and logged in user, gives you the ability to step through the state-changes of the system. Great for seeing what the users are doing and even better for helping to diagnose bugs.
  • Make working on legacy projects fun again - we have projects running dating back 3-4 years which are running MonoRail - but the same principles apply - if we need to make a change or add a new feature, we'll apply this pattern to those older projects too - only for the new bits, but it means I can write tests for a legacy project which previously we thought was un-testable!

In more complex scenarios we also have Commands which call out to other Commands - eg. One to save the database, one to send an email, one to call a WebService. This enables your Commands to remain simple, adhere to SRP and not become the new home for all of the spaghetti code.

Totally agree with Alan about the naming of Commands - for a very CRUD-like form it can sometimes be difficult to choose meaningful names. But the CQS/CQRS approach encouraged us to adopt more of a "Task Based UI" which helps with the naming - See http://codebetter.com/gregyoung/2010/02/16/cqrs-task-based-uis-event-sourcing-agh/

Next stop for us? - CommandHandlers, Events, Event stores and maybe even a couple of Message Buses - who knows!

16 Apr 2012

Hey Paul,
Nice post. I have been doing something similar but with a couple little twists and it has been working out pretty well for me.

The code is here as it would not fit into one comment. https://gist.github.com/2395851

My delegating controller just inherits from Controller but has the Invoker property which allows me to execute the specific handlers.

I use strongly typed models for everything, which allows me to look up the handler very easily and execute it. This is done via a container usually.

If the modelstate is invalid I just return the Edit() action, as the Modelstate will still have all the errors and so the page will get populated correctly. It also means you only ever have to write that code once.

This scenario enables easy unit testing of the business logic in the handlers and similar ease in testing the interactions of the controllers. eg. Ensuring the handler got called and the redirect/display was invoked correctly.

I'm trying to formalise this into a mini library at the moment. It anyone is interested ping me on twitter at @schotime.

Cheers Adam

Jimmy P
Jimmy P
16 Apr 2012

+1 for PRG Steven

Do like the command pattern though too

Robert Vukovic
Robert Vukovic
16 Apr 2012

I really don't see any benefit with this method. It is not simpler, and there is more code/classes to maintain. You just moved code from one place to another. Maybe it is useful when actoin is long but not in example you gave.

More code => more bugs, more maintenance effort.

17 Apr 2012

I went through several phases myself. As of late, I'm really liking the CQRS-ish approach.

Shea Strickland
Shea Strickland
18 Apr 2012

I'm interested to see how you tackle control flow, and conditional paths... I genuinely am :)

24 Apr 2012

For conditional paths, you could simply return a boolean value, from the Execute method or add two Func parameters to the Execute method, for success and failure, and let the command object call the appropriate one.

Miguel Moura
Miguel Moura
24 Apr 2012

I am doing this differently and, in my option, easier to maintain.

I use a Service Layer (with Agatha) with Response/Request/Handlers.

These commands are usually very simple: CreateUser, DeleteUser, UpdateUser, FindUserByUsername, GetRoles

The responses return a DTO Objects sometimes with extra functionality.

On the controllers I use a dispatcher to run a Request and get the Response. Then I map the Response to ViewModel using AutoMapper.

For the CountriesList and other lists I usually use small classes which I name Tasks that behave has helpers used across the application.

On the controller I place the CountriesList in the ViewBag ... I usually place in the ViewModel only the properties to be validated.

What do you think?

Cheers, Miguel

24 Apr 2012

Getting there - https://gist.github.com/2481406

Lloyd McGhee
Lloyd McGhee
26 Apr 2012

A couple of things.

  • the country thing not posting back -> just use a
    @html.hiddenfor(m=> m.Countries) and now it will post back and forth.

-models defined in data layer-

ok the save and update thing. I just send the model or relevent submodels to the data layer and let it do what it needs to.

with this approach the controller code is tiny.

L.

30 Apr 2012

Nice point of view, thanks

Jim Taylor
Jim Taylor
30 Apr 2012

I have achieved a similar result by using a controller base class and dependency injection to wire up the retrival of the model using a model builder and by creating a command dispatch method to build a command and dispatch it. The command dispatcher then raises events which are then used to control the resulting controller action.

[HttpGet]
public ActionResult Edit(Guid id)
{
    var parameters = new EditParameters(id, ContextService.CurrentUsername);
    return View<EditParameters, Edit>(parameters);
}

[HttpPost]
public ActionResult Edit(Guid id, Edit model)
{
    return Dispatch<SaveMyEntityCommand>(model)
        .WithEvent<EntityUpdatedEvent>(@event => RedirectionToSelfWithSuccess())
        .WithEvent<EntityUpdatedErrorEvent>(@error => RedirectionToSelfWithError(model, @error.Message))
        .Execute();
}
Jesse
Jesse
30 Apr 2012

I too like to use a builder pattern for putting together my view models keeping my controllers lean.

After reading through this blog post the first thing that comes to mind is "Over Engineering". There are lots of ways and patterns that one can use to do this but just because one can break the creation of a view model out over 5 or 6 .cs files using 2 or 3 design patterns does not mean that you should. At some point your basically just showing off and losing sight of the fact that A: someone has to maintain that code later on and B: you are not really making things any better once you pass a certain point of abstraction.

Using the Builder and Command patterns having references to the same properties in multiple classes and files just to build a view model is a maintenance headache and a bit overkill.

Keep it simple and testable and you'll be fine. A single builder class and some dependency injection will work just fine 9 out of 10 times in regards to putting together a view model. Really all your doing more often than not is utilizing the resources you already have in your architecture i.e service layer, repositories etc.. to hydrate a view model class.

I am not trying to belittle your blog post as I know you guys are top notch developers, my point is that there is nothing wrong with keeping things simple.

Eile
Eile
30 Apr 2012

my company is using n-tier with mvc3. We normally only have few lines of code on the controller.