Put your controllers on a diet: a survey

Previous posts in this series:

In the last post, I removed all the abstractions and layers from my application in order to provide a better view on what my next steps should be. This is like climbing to the top of a mountain or climbing a tree, you have to move to a higher elevation to increase your purview. I’m not interested in just this one controller, but all my controllers. I want to build a consistent architecture across my system, as every new controller/action shouldn’t require a new decision on how it should be built.

So what are our goals here? Why do any refactoring at all?

Looking at just this one controller, I don’t currently see any reason to go any further. I wouldn’t even write tests for this necessarily, there’s so little going on.

But maybe I do, and testing these controllers isn’t really a big deal. It’s an integration test, one that would exercise the database. We already know how to do that, so there’s not much point in refactoring these controllers any in the goal of “testability”. It’s already testable. I can control all the pieces I need to in this picture:

public ActionResult Index(int? minSessions)
{
    minSessions = minSessions ?? 0;

    var list = (from conf in _session.Query<Conference>()
        where conf.SessionCount >= minSessions
        select new ConferenceListModel
        {
            Id = conf.Id,
            Name = conf.Name,
            SessionCount = conf.SessionCount,
            AttendeeCount = conf.AttendeeCount
        }).ToArray();

    return View(list);
}

Back to our goals, why go further? What about complexity? Is this complex?

[HttpPost]
public ActionResult Edit(ConferenceEditModel form)
{
    if (!ModelState.IsValid)
    {
        return View(form);
    }

    var conf = _session.Get<Conference>(form.Id);

    conf.ChangeName(form.Name);

    foreach (var attendeeEditModel in form.Attendees)
    {
        var attendee = conf.GetAttendee(attendeeEditModel.Id);

        attendee.ChangeName(attendeeEditModel.FirstName, attendeeEditModel.LastName);
        attendee.Email = attendeeEditModel.Email;
    }

    return this.RedirectToAction(c => c.Index(null), "Default");
}

Again, not really. I’ve got a few things going on (validation, querying, mutation and then redirection) covering various layers (UI/DB/Domain) all in one method. Is this fundamentally wrong?

Well, it depends.

If I just have a small application, who cares, additional abstraction only detracts. If I’m erecting a backyard shed, I don’t need steel piers driven into the ground for stability. A treehouse doesn’t need central heating and air or running water. The beauty of software is that unlike construction, we can pivot and refactor our design to an architecture suited to the complexity of our system.

Where will our complexity lie? I’m not entirely sure yet, but one thing I do know – controllers can get messy, sloppy, muddled and disheveled. Looking over the entire controller class  (https://gist.github.com/jbogard/7102870) there’s a clear pattern here:

  • GET actions query the database and return a view model to the view
  • POST actions validate, load an entity, mutate it, and redirect to a success page

Whatever direction we go, these two kinds of activities (GET/POST, read/write) are very, very different. Controllers make me put all that code in one place, but what if things get a little crazy? What if our validation for conference names require a unique name (subtle, but it’s implied in this design). What about caching? What if building my view screens require data from multiple locations, and can’t be done with one query? What if editing successfully results in sending an email out to attendees?

It’s these complexities that lead me down a direction of clear separation of GET/POST concerns. Whatever abstractions I build, I’m going to make sure that I don’t cross the GET/POST boundary (or for those paying attention, the Command/Query boundary).

Next up: commands and queries.

Related Articles:

Post Footer automatically generated by Add Post Footer Plugin for wordpress.

About Jimmy Bogard

I'm a technical architect with Headspring in Austin, TX. I focus on DDD, distributed systems, and any other acronym-centric design/architecture/methodology. I created AutoMapper and am a co-author of the ASP.NET MVC in Action books.
This entry was posted in Architecture, ASP.NET MVC, Design. Bookmark the permalink. Follow any comments here with the RSS feed for this post.
  • Pingback: The Morning Brew - Chris Alcock » The Morning Brew #1470

  • beton

    Nice read. Waiting for C/Q article :)

    Cheers

  • Michael

    Awesome series.

    I just wanted to add one small bit that usually pops in every scenario that involved post and that really starts to pile up. Example would be the easiest to show it.

    Lets say you need to an “edit” page for your event. There “author” field should be dropdown and you have to fill it twice – in “get” and “post” action. And now imagine that you need to display only authors with the state “active” and you have other fields like that. Thats a lot of “init” code for view model. Extracting it into private method usually creates not so nice code that should work correctly for “create”, “display edit via get” and “display edit after post” actions.

    • http://www.joshka.net/ Joshua McKinney

      PRG even on the non-successful post solves this problem neatly. Everything is display via get, there is no display via post.

      • Michael

        What is PRG? And how’s that POST action don’t display anything – you need to display the same form with modified (by user) values alongside with errors after post sometimes.

        • josh

          Post redirect get pattern. Instead of returning view(model) return redirect(“getaction”) Let model.state handle the refilling values magically. Persist modelstate in tempdata between the post and the get. You can do this with an easy action filter.

          • Michael

            Ah, I’ve heard of it. Gonna try it and see it’s drawbacks. Thanks.

          • Michael

            Well, I’ve tried it and one thing that really bothers me is that when I use filters to save/restore model state from temp data is that I loose the ability to setup initial values in get method. Imagine I want to create a user – in “[get] create” action I can create model class and assign some initial values to it. Then user post his values to “[post] create” action where we got some validation errors and we redirect him back to “[get] create” where all his inputed values are overwritten with initial values unless I create some old fashioned “if postback” condition.

  • Petr Snobelt

    It looks like you also need “single action controller”. I read about it in Simple.Web http://blog.markrendle.net/2012/06/01/simple-web/