DEV Community

Joe Petrakovich
Joe Petrakovich

Posted on • Originally published at makingloops.com on

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.

Oldest comments (3)

Collapse
 
makingloops profile image
Joe Petrakovich

Thanks William, and yes actually I use both!

Collapse
 
oscherler profile image
Olivier “Ölbaum” Scherler

Interesting, but as not a .NET developer, I’d have liked to see examples of where you should put that code instead.

Collapse
 
nssimeonov profile image
Templar++ • Edited

While this is sort of OK, it's better to say what is good to do in a controller and how to do it, instead of what isn't - like when you talk to a toddler - saying "NO" is like a white noise to them - they stop for a few seconds or cry if you say that loud, but a few minutes later they try again.

Let me argue about the NOs - adding too much abstractions and following these rules (and other design patterns) usually leads to layer after layer of abstractions and getting to the bottom of such code is basically a pain in the neck. Even bigger fun is adding a new column in the database, then in the data model, then in another intermediate business object, then in the response model, then in another request model, ah even one more intermediate object and at that point I usually want to get drunk... in a "quick-and-dirty php-style code" (and I hate php in my guts, I assure you), it will be literally 15-20 lines of code all that and it will be in a single file, where you can see all of it in one place, instead of scattered in 3 different projects and 12 different files where half of the code is pointlessly mapping fields from one object to another.

A program is a set of instructions for the computer to do something and we usually have to maintain this piece of s**t for years, so the most important part of it is to be obvious and easy to understand and change. This is why in theory the single responsibility principle sounds good and you may think - hey the code is great - I see a small function and I can see what it does. Then you find another one, then another one and 8 levels deep in the function hierarchy on the 5th file you opened you realise, that at this point you need a sheet of paper to write down what the hell you are doing, how the objects "talk" to each-other, where is the factory, that created an object with the required interface and so on and so on.