asp.net mvc controller post best practices

Question

I am bit confused about "best practice" controller using question.

My typically code look

    public ActionResult Edit(int reportId,FormCollection formCollection)
    {
        try
        {
            var report = _dbContext.EmployeeReports.Find(reportId);

            if (TryUpdateModel(report))
            {
                _employeeReportService.Update(report);
                return RedirectToAction("List");
            }

            return View("Edit", report);
        }
        catch (Exception)
        {
            // some logging etc
            return RedirectToAction("List");                
        }

Well, is better using "TryUpdateModel" or only "UpdateModel" or simple call Model.IsValid and is good idea to catch exception in controller?

Thanks

TryUpdateModel will swallow any exceptions and return false if there is a problem. UpdateModel will allow exceptions to be thrown. — jrummell, Sep 19 '11 at 16:43

score 5 · Answer 1 · answered Sep 20 '11 at 06:11

Here's an alternative way that I prefer:

[HttpPost]
public ActionResult Edit(ReportViewModel reportViewModel)
{
    if (!ModelState.IsValid)
    {
        // there were validation errors => redisplay the form
        // so that the user can fix them
        return View(reportViewModel);
    }

    // At this stage the view model is valid => we can
    // map it back to a domain model and pass to the repository 
    // for processing

    // Fetch the domain model that we want to update
    var report = _repository.Get(reportViewModel.Id);

    // map the domain model properties from the view model properties
    // in this example I use AutoMapper
    Mapper.Map<ReportViewModel, Report>(reportViewModel, report);

    // perform update
    _repository.Update(report);

    // the update wen fine => we can redirect back to the list action
    return RedirectToAction("List");
}

So, as you can see no FormCollection, no TryUpdateModel, no UpdateModel, no try/catch.

Hmm, interesting answer.On use automapper in my applications I think long time. Btw in large models I use viewmodels too. — Mennion, Sep 20 '11 at 07:38
@UserControl, actually it does address it because the view model will contain only the properties that are *allowed* to be modified by the user (those for which you have input fields in the view). The AutoMapper's `Map` method will then only update those properties on the domain model, leaving the others intact, thus addressing the mass assignment vulnerability to which your application would have been subject if you used directly the domain model. — Darin Dimitrov, Dec 26 '13 at 09:08

michael · Answer 2 · 2013-10-02T11:30:10.473

After my opinion you should always use view models instead of formcollection to avoid under-posting and over-posting issues. Therefore best practice, after my opinion, is to use a view model for rendering the view and a kind of post/get model that binds to exactly what you want the users to post to/get from an action.

This might be some extra work and some of the view models will look quite similar to the models that you use for binding in controller action, but I would say "Security over convenience."

John S · Accepted Answer · 2011-09-21T22:36:15.750

This depends if you expect and plan to deal with exceptions.

My usual approach is:

public ActionResult foobar(FormCollection formCollection)
{
    //Keep this out of the try catch scope in case you need to pass it
    // to the next method.
    Model model = new Model();

    try
    {
        if(!TryUpdateModel(model)
        {
            //Update Failed so fix it and redirect
            return redirectToAction("fixit");
        }
        if(!ModelState.IsValid())
        {
            //Update worked but model state was invalid, return to page to correct 
            //model validation errors
            return View("foobar", model);
        }
        //Update Succeeded so do other stuff
    }
    catch(Exception ex)
    {
        //Deal with Exception
        return redirectToAction("ErrorView", "ErrorController");
    }

    return redirectToAction("NextStep");
}

I try to use all of them in my code to try and catch every issue before it breaks something.

I should also mention that for the model validation, if you have aggregate classes (classes composed of other classes) using tags like [Required] dont work all to well. I usually make my own model state checker and replace if(!ModelState.IsValid()) with a call to a boolean method to check specifics. — John S, Sep 22 '11 at 15:25

asp.net mvc controller post best practices

3 Answers3

Linked