Focusing on the controller's responsibility

The following is an excerpt from ASP.NET MVC 2 in Action, a book from Manning appearing in bookstores in May.  The early access (MEAP) edition is available now on http://manning.com/palermo2.  Authors include Jeffrey Palermo, Ben Scheirman, Jimmy Bogard, Eric Hexter and Matt Hinze.  Technically edited by Jeremy Skinner.

This selection is from chapter 19, Lightweight controllers.  All hyperlinks were added for this post.


A quick way to lighten the controller’s load is to simply remove responsibilities from it. Consider the burdened action, shown below:

A heavyweight controller
public RedirectToRouteResult Ship(int orderId)
{
   User user = _userSession.GetCurrentUser();
   Order order = _repository.GetById(orderId);

   if (order.IsAuthorized)
   {
      ShippingStatus status = _shippingService.Ship(order);

      if (!string.IsNullOrEmpty(user.EmailAddress))
      {
         Message message = _messageBuilder
            .BuildShippedMessage(order, user);

         _emailSender.Send(message);
      }

      if (status.Successful)
      {
         return RedirectToAction("Shipped", "Order", new {orderId});
      }
   }
   return RedirectToAction("NotShipped", "Order", new {orderId});
}

This action is doing a lot of work-it’s incomprehensible at first glance. You can almost count its jobs by the number of if statements. Beyond its appropriate role as director of the storyboard flow of the user interface, this action is deciding if the Order is appropriate for shipping and determining whether or not to send the User a notification email. Not only is it doing those things, but it’s deciding how to do them-it’s determining what it means for an Order to be appropriate for shipping and how the notification email should be sent.

Logic like this-domain logic, business logic-should generally not be in a user interface class like a controller. It violates the single responsibility principle, obfuscating both the true intention of the domain and the actual duties of the controller, which is redirecting to the proper action. Testing and maintaining an application written like this is difficult.

Cyclomatic complexity: source code viscosity

Cyclomatic complexity is a metric we can use to analyze the complexity of code. The more logical paths a method or function presents, the higher its cyclomatic complexity. In order to fully understand the implication of a particular procedure, each logical path must be evaluated. For example, each simple if statement presents two paths-one when the condition is true, and another when it’s false. Functions with high cyclomatic complexity are more difficult to test and to understand and have been correlated with increased defect rates.

A simple refactoring that can ease this is called Refactor Architecture by Tiers. It directs the software designer to move processing logic out of the presentation tier into the business tier.

After we move the logic for shipping an order to an OrderShippingService, our action is much simpler.

A simpler action after refactoring architecture by tiers
public RedirectToRouteResult Ship(int orderId)
{
   var status = _orderShippingService.Ship(orderId);
   if (status.Successful)
   {
      return RedirectToAction("Shipped", "Order", new {orderId});
   }
   return RedirectToAction("NotShipped", "Order", new {orderId});
}

Everything having to do with shipping the order and sending the notification has been moved out of the controller into a new class. The controller is left with the single responsibility of deciding where to redirect the client. The new class can fetch the Order, get the User, and do all the rest.

But the result of the refactoring is more than just a move. It’s a semantic break that puts the onus of managing these tasks in the right place. This change has resulted in a clean abstraction that our controller can use to represent what it was doing before. Other logical endpoints can reuse the OrderShippingService, such as other controllers or services that participate in the order shipping process. This new abstraction is clear, and it can change internally without affecting the presentation duties of the controller.

Refactoring doesn’t get much simpler than this, but a simple change can result in significantly lower cyclomatic complexity and can ease the testing effort and maintenance burden associated with a complex controller.

About these ads
This entry was posted in ASP.NET MVC. Bookmark the permalink.

6 Responses to Focusing on the controller's responsibility

  1. ryzam says:

    I have a question on how you use a controller as a query gateway. Do you delegate that to a service or you use controller to direct query into database?

    public JsonResult FindShipsByOrderId(int orderId)
    {
    return s.Linq().Where(….).Select(c=> new { ShipId= c.shipId, …});
    }

    or

    public JsonResult FindShipsByOrderId(int orderId)
    {
    return orderShippingService.FindShipsByOrderId(orderId);
    }

  2. Pingback: Focusing on the controller’s responsibility « Huy Nguyen's Blog

  3. Matt says:

    @ryzam

    This is annoying, but it depends. If the semantics of the linq query are clear and it’s concise.. why not use it? I think I would default to the linq query.. but there’s a pain/maintainability threshold that will be met soon in a larger project. And so I’d keep an eye on that. I think in the next year or so we’ll see an alternative that is something like query object (but not query object) that we might like better than the OCP-violating repository methods.

  4. ryzam says:

    But actually I don’t see much benefit to have another abstraction layer for query a record. I can agree if we want execute a command then it will go through appropriate layer, but for query why don’t we query direct using linq or what ever technology (NHibernate, EF) etc

  5. Pingback: Dew Drop – April 21, 2010 | Alvin Ashcraft's Morning Dew

  6. mfalanga says:

    @ryzam,

    For me, it comes down to two things: 1) being DRY, and 2) testability. If I find that I have to write that same query again, I will refactor so that it’s in one place. For testability, if I go through another layer of abstraction, I can mock out that layer to make sure that my controller is working properly. In Matt’s example, I can mock out the Ship method to return different statuses, and ensure that my controller is redirecting how it is supposed to.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s