CODEWITHSUNDEEP
asp.net-mvcseparation-of-concerns
Neil
Neil

Reputation: 1299

Asp.Net MVC Actions - Separation of Concerns/Single Responsibility Principle

In computer science we've been taught that each method should do one thing and one thing only. I'm a little confused then that we see MVC actions like the following given as examples of good practice:

    [AcceptVerbs(HttpVerbs.Post), Authorize]
    public ActionResult Edit(int id, FormCollection collection) {

        Dinner dinner = dinnerRepository.GetDinner(id);

        if (!dinner.IsHostedBy(User.Identity.Name))
            return View("InvalidOwner");

        try {
            UpdateModel(dinner);

            dinnerRepository.Save();

            return RedirectToAction("Details", new { id=dinner.DinnerID });
        }
        catch {
            ModelState.AddModelErrors(dinner.GetRuleViolations());

            return View(new DinnerFormViewModel(dinner));
        }
    }

Basically this piece of code provides a lot of functionality:

  1. Defines how to access the Action - Post only
  2. Defines who can access the Action - Authorize
  3. Accesses persistence mechanism - dinnerRepository
  4. Accesses state information - (User.Identity.Name)
  5. Converts NameValueCollection to strongly typed object - UpdateModel()
  6. Specifies 3 possible ActionResults and content for each - InvalidOwner/Details/Edit views

To me this seems like too many responsibilities for one method. It is also a fairly simple action ie it doesn't deal with common scenarios like:

  1. Checking business rules - "Never trust user input"
  2. Navigation paths - On successful save always returns to "Details"
  3. Different return types - Someone wants to call "Edit" from a grid and needs a JsonResult?
  4. Better error handling - YSOD if the database is not accessible during GetDinner(id)
  5. Constructing additional view data - SelectLists for Drop down lists

Not too mention the amount of testing required around this single method i.e. mocking/faking for FormCollection/UserIdentity/Authorization Provider/Repository/etc.

My question is how do we avoid cramming so much stuff into our controller actions?

I tend to think "opinions" are a great concept, especially the "Thunderdome Principle". While I have great respect for the guys involved with building FubuMVC and their reasons behind doing so, I need something I can use right now.

Edit - Well it seems I was after something like this - Opinionated Controller. I'll need to examine it further as it's for MVC Preview 5 so i may need to update it myself.

Upvotes: 7

Views: 2020

Answers (4)

john
john

Reputation: 1

the issue that i have with the "single responsibility principle" is that people do not always segment events the same - some more granular than others. so it would seem, that one could easily find a more granular view of an action in all, but the most trivial cases.

Upvotes: -1

Chrisb
Chrisb

Reputation: 729

I am a little confused by your post. First you complain that this action is doing to much, and then you complain that is not doing more.

Edited to add:

Honestly this is not a very complicated controller action. Whether it is a best practice example or not is debatable, but, you probably are not going to get a whole lot simpler than that. I suppose you could break some of it up into separate routines, but at some point you have to decide where to draw that line. Ultimately we, as programmers have to write software. Design principals are awesome, but if we are too rigid with them nothing will ever get built.

Upvotes: 1

littlegeek
littlegeek

Reputation:

I think it is still doing on the minimum actions required..for this "Action" may not fit the Absolute single responsbility - but it is single action

  1. The the attributes are saying to ASP.NET that this method will only work with HTTP.Post, and identity that is trying to use it must be authorised. - Good Security. So at this point nothing is actually being done. These are just telling the server what to check.

    ie if not HTTP.post method will not work, and if your not the list, wont work.

  2. There is validation to check if the user Identity matchs those of the dinner. - Sanity Check.

  3. This is based on Strong Type Checking - do UpdateModel(dinner) - is just making sure that the current objects in the model have been updated with the new data, then the repository is called to Save(). - This is still in the one unit of action - updating the model so that we can call save and persist.

  4. Validation checks are handled in the Catch which is adding RuleViolations to the Model, and returning the user back to the offending view - ie edit/create partial view which is passing responisbility to that to deal with.

  5. If the save works - its is just moving the "workflow" of the user to the Details - i.e clearing the form from memory and moving the work back to the details. IMHO great as - we are in a POST scenario and stuff not lying around in memory - Good.

  6. It would be easier not to move the flow to back the details and just leave on the partial edit view.

Upvotes: 0

Dave Van den Eynde
Dave Van den Eynde

Reputation: 17405

To me, this method does only one thing: update the model with the edited values received from the web form.

It's clear that doing this requires a couple of things to happen, but they are atomic and well defined. If you ever need to modify how this part of the model needs to be updated, this is the controller action to look for and update.

You could argue that one of the Thunderdome principles, "controllers should be light", is not met since a business rule is checked here. But NerdDinner is a very trivial app that it makes no sense to place this in an extra layer.

If you find that this method does too much, perhaps you should find a language in which it's forbidden to place more than one statement in a method.

Upvotes: 3

Related Questions