Cleaning up ASP.NET MVC Controllers

Published on: 13 Apr 2012

I've been working with ASP.NET MVC on a few differentprojects 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.

Welcome, my name is Paul Stovell. I live in Brisbane and work on 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

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.

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.

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!!!

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!

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

16 Apr 2012

+1 for PRG Steven

Do like the command pattern though too

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.

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

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.

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.

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

30 Apr 2012

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