6 types of code you shouldn't have inside your .NET controllers

“Your .NET controllers should be thin”

The ever-repeated platitude with 3 metric tons of baggage to unpack.

Why should they be thin? How does that benefit you? What steps can I take to get them thin if they aren’t already? How do I keep them that way?

All valid (and common) follow-up questions. I’ve discussed part of the why in some earlier articles, so in this article we are going to take another angle.

To begin the process of identifying the steps needed to thin out controllers, we need to understand some of the common ways they can become fat.

In my experience, I’ve found 6 common types of code that sneak into our controllers that ultimately would be better suited elsewhere. This list is not comprehensive though, as I’m sure there are even more.

1. Mapping Data Transfer Objects (DTOs)

Because our controllers are on the figurative front lines of the request pipeline, there is often a need to create request and response objects if the requirements of the request are more complicated than simply using URL arguments and HTTP status code responses.

Any time you’ve got DTOs, you’ve also got a need to map the values they hold to something more functional like a domain entity.

You know, this kind of stuff:

public IActionResult CheckOutBook([FromBody]BookRequest bookRequest)
{
    var book = new Book();

    book.Title = bookRequest.Title;
    book.Rating = bookRequest.Rating.ToString();
    book.AuthorID = bookRequest.AuthorID;

    //...
}

This mapping logic is innocent enough, but it quickly bloats the controller and adds an additional responsibility. Ideally, our controller’s single responsibility is only to delegate the next action at the level of the HTTP request.

2. Validation

Obviously we can’t have bad input making its way into the inner castle walls of our domain. Validation protects us from that, usually first on the client, but then again on the server.

I like to treat controllers sort of like head chefs. They have assistants that prepare all the ingredients for them, so they can work their magic on the final plating. There are plenty of ways you can set up validators along the request pipeline in ASP.NET MVC so that the controller can assume the request is valid and delegate the next action.

This type of code is unexcusable!

public IActionResult Register([FromBody]AutomobileRegistrationRequest request)
{
    // //validating that a VIN number was provided...
    if (string.IsNullOrEmpty(request.VIN))
    {
        return BadRequest();
    }

    //...
}

Rubbish! (spoken in a Gordon Ramsay voice)

3. Business Logic

If you’ve got anything business-related in the controller, you will likely have to write it again elsewhere.

Sometimes there is overlap with validation too. If your validation logic has rules that a business person would be deciding (rather than simple things like number ranges or things a string can be), you run the risk of having to repeat that code.

4. Authorization

Authorization is similar to validation in that it is a protective barrier. Rather than bad requests making it into the domain, authorization prevents bad users from getting somewhere they are not allowed.

Also like validation, ASP.NET offers many ways to compartmentalize authorization (middleware and filters, for example.)

If you’re checking properties on your User within the controller action to grant/block something, you miiight have some refactoring to do.

5. Error Handling

It burns, it BURNS!

public IActionResult GetBookById(int id)
{
    try
    {
      //the important stuff that a head chef would do...
    }
    catch (DoesNotExistException)
    {
      //stuff an assistant should be doing...
    }
    catch (Exception e)
    {
      //please, no...
    }
}

This one is quite broad, and sometimes handling exceptions must be done in the controller, but I’ve found there is almost always a better, more localized place. And if there isn’t, you can take advantage of global exception handling middleware to catch the most common errors and return something consistent to your clients.

Going back to the head chef metaphor, I like my head chef not to have to worry about that kind of thing. Let them do their single responsibility and assume someone else will handle anything unexpected.

6. Data Storage/Retrieval

Work like getting or saving entities using a Repository often ends up in the controller to save time. If the controller’s are just CRUD endpoints, then hey, why the hell not.

I even have an older article that shows controllers with this very trait.

Rather than just calling this a bad behavior, I think pointing out an alternative way will highlight why this can bloat your controller’s unneccesarily.

First, looking at it from a design perspective (with a focus on the Single Responsibility Principle), if you have objects that are designed for persistence being used by your controllers, your controllers are then have more than one reason to change.

public IActionResult CheckOutBook(BookRequest request)
{
    var book = _bookRepository.GetBookByTitleAndAuthor(request.Title, request.Author);

    // if you've got the above statement already, you'll be
    // tempted to add the book checkout biz logic here...
    //...

    return Ok(book);
}

Beyond basic CRUD, persistence logic in the controller is a code smell for additional sprouts of logic that would be better suited deeper down the chain.

This is where I like to use either some kind of application service (behind an interface) to handle the work, or delegate to some kind of CQRS command/query object.

That’s it!

Can you think of any more types?

If you’d like to go deeper, I’ve put together a set of recipes with code samples and an exercise specifically for making your controllers thin. It is not only the how, but provides a deep exploration of the why.

You can get one of the recipes at the bottom of the original article.