How to factor error-handling code out of an MVC controller?

Question

Most of the time, I can figure out how to factor out business logic from an MVC controller into a service or a model. However the exception to this is error handling, which it seems to me is the controller's responsibility. However, it can lead to rather "un-skinny" controllers. For example:

if ((foundEvent = repoEvent.GetEventById(id)) == null) {
    return HttpNotFound("Could not find event with id {0}.".FormatWith(id));
}

var assessment = isSample ? foundEvent.SampleAssessment : foundEvent.ActualAssessment;
if (assessment == null) {
    return HttpNotFound("Could not find {0}assessment for event with id {1}".FormatWith(isSample ? "sample " : "", id));
}

if (assessment.UnmappedCaseStudiesCount == 0) {
    TempData[TempDataKeys.ErrorMessage.GetEnumName()] = "This assessment (\"{0}\") has no case studies!".FormatWith(assessment.Name);
    return RedirectToAction("Index", "Events");
}

In all the cases above, it seems to me that the logic does belong in the controller because the controller is setting up error messages rather than returning a view (this is re-enforced by the fact that things like HttpNotFound and RedirectToAction are members of the Controller class, and so are unavailable to classes that don't extend Controller). However you can see how this would get long and messy after a short while. Is there a way to factor out this kind of error handling in the controller to make the controller more "skinny", or does this stuff simply belong in the controller?

Here's a larger snippet of code indicating how I refactored to allow 2 action methods to use the same view model setup code. However, it still results in 90+ lines of code in the controller just for 2 action methods; again, not a very "skinny" controller:

#region Private methods
private StartAssessmentViewModel getStartViewModel(int eventId, bool isSample, out ActionResult actionRes) {
    actionRes = null;

    EventRepository repoEvent = new EventRepository();
    Event foundEvent;
    if ((foundEvent = repoEvent.GetEventById(eventId)) == null) {
        actionRes = HttpNotFound("Could not find event with id {0}.".FormatWith(eventId));
        return null;
    }

    var assessment = isSample ? foundEvent.SampleAssessment : foundEvent.ActualAssessment;
    if (assessment == null) {
        actionRes = HttpNotFound("Could not find {0}assessment for event with id {1}".FormatWith(isSample ? "sample " : "", eventId));
        return null;
    }

    if (assessment.UnmappedCaseStudiesCount == 0) {
        TempData[TempDataKeys.ErrorMessage.GetEnumName()] = "This assessment (\"{0}\") has no case studies!".FormatWith(assessment.Name);
        actionRes = RedirectToAction("Index", "Events");
        return null;
    }

    try {
        // Has the assessment finished? (samples don't count)

        UserAssessmentRepository repoUa = new UserAssessmentRepository();
        var foundUa = repoUa.GetUserAssessment(foundEvent.EventId, assessment.AssessmentId, User.Identity.Name);
        // TODO: check that foundUa.Assessment.IsSample is OK; may need to make .Assessment a concrete instance in the repo method
        if (foundUa != null && !foundUa.Assessment.IsSample) {
            if (_svcAsmt.IsAssessmentFinished(foundUa)) {
                // TODO: test that this way of displaying the error works.
                TempData[TempDataKeys.ErrorMessage.GetEnumName()] = "You have already completed the assessment for this event ('{0}'); you cannot start it again.".FormatWith(foundEvent.Name);
                actionRes = RedirectToAction("Index", "Events");
                return null;
            }

            // Has it been started already?
            if (_svcAsmt.IsAssessmentStarted(foundEvent.EventId, foundUa.AssessmentId, User.Identity.Name)) {
                actionRes = RedirectToAction("Question", new { id = foundUa.UserAssessmentId });
                return null;
            }
        }

        return Mapper.Map<StartAssessmentViewModel>(assessment);
    }
    catch (Exception ex) {
        TempData[TempDataKeys.ErrorMessage.GetEnumName()] = "Could not display start screen for assessment {0} on event {1}; error: {2}".FormatWith(assessment.AssessmentId, foundEvent.EventId, ex.Message);
        actionRes = RedirectToAction("Index", "Events");
        return null;
    }
}
#endregion

public ActionResult Start(int id, bool isSample = false) {
    // Set up view model
    ActionResult actionRes;
    StartAssessmentViewModel viewModel = getStartViewModel(id, isSample, out actionRes);
    if (viewModel == null) {
        return actionRes;
    }

    return View(viewModel);
}

[HttpPost, ActionName("Start")]
public ActionResult StartConfirmed(int id, StartAssessmentViewModel viewModel) {
    // Set up view model
    ActionResult actionRes;
    StartAssessmentViewModel newViewModel = getStartViewModel(id, viewModel.AssessmentIsSample, out actionRes);
    if (newViewModel == null) {
        return actionRes;
    }

    if (!ModelState.IsValid) {
        return View(newViewModel);
    }

    // Model is valid; if it's not a sample, we need to check the access code
    if (!viewModel.AssessmentIsSample) {
        if (viewModel.AccessCode != "12345") {
            // Invalid access code
            TempData[TempDataKeys.ErrorMessage.GetEnumName()] = "The access code '{0}' is incorrect.".FormatWith(viewModel.AccessCode);
            return View(newViewModel);
        }
    }

    // Access code is valid or assessment is sample; redirect to first question
    return RedirectToAction("Question", new { id = 1 });
}

Answer 1

I agree that this sort of error handling is the responsibility of the Controller, so I think you've got it in the right place. With regard to making the methods 'skinnier', you could do worse than checking out Bob Martin's Clean Code guidelines, which would recommend refactoring your larger methods into more smaller methods, something like this:

if (assessment.UnmappedCaseStudiesCount == 0) {
    TempData[TempDataKeys.ErrorMessage.GetEnumName()] = 
        "This assessment (\"{0}\") has no case studies!".FormatWith(assessment.Name);
    actionRes = RedirectToAction("Index", "Events");
    return null;
}

...to use helper methods, something like this:

if (AssessmentHasNoCaseStudies(assessment, out actionRes)) {
    return null;
}

...

private bool AssessmentHasNoCaseStudies(Assessment assessment, out ActionResult actionRes)
{
    actionRes = (assessment.UnmappedCaseStudiesCount == 0)
        ? RedirectToAction("Index", "Events")
        : null;

    if (actionRes == null)
    {
        return false;
    }

    TempData[TempDataKeys.ErrorMessage.GetEnumName()] = 
        "This assessment (\"{0}\") has no case studies!".FormatWith(assessment.Name);

    return true;
}