ASP.NET MVC - Reusing Action Behaviors

Question

This question pertains primarily to good design.

Suppose I have a controller action like DeletePage that can be invoked in two separate views of the same controller. Assuming the delete logic is not contained in the action itself, but rather some conditional checks and the like that call the correct business logic, it doesn't make sense to duplicate the structure of the delete action when I can instead have a private method that returns an ActionResult which I call in both actions which can cause a delete. My question is where is the best place to place a reusable action method like this? Right now I'm just marking them private and sticking them in a region of the controller class, but perhaps an sealed inner class would make more sense for such a method- or somewhere else entirely.

Thoughts?

public ActionResult EditPage(int id, FormCollection formCollection)
{
 var page = _pagesRepository.GetPage(id);

 if (page == null)
  return View("NotFound");
 if (page.IsProtected)
  return View("IllegalOperation");

 if (formCollection["btnSave"] != null)
 {
  //...
 }
 else if (formCollection["btnDelete"] != null)
 {
  return DeletePage(page);
 }
 return RedirectToAction("Index");
}

public ActionResult DeletePage(int id)
{
 var page = _pagesRepository.GetPage(id);

 if (page == null)
  return View("NotFound");

 return DeletePage(page);
}

// Reusable Action
private RedirectToRouteResult DeletePage(Page page)
{
 if(page != null && !page.IsProtected)
 {
  _pagesRepository.Delete(page);
  _pagesRepository.Save();

  FlashMessage(string.Format(PageForms.PageDeleted, page.Name), MessageType.Success);

  return RedirectToAction("Index");
 }
 return RedirectToAction("Index");
}

Answer 1

In my opinion, your reusable code is an Action because it is returning an ActionResult. So your use is fine. The DeletePage(Page page) could potentially remain public.

I look forward to other opinions.

Answer 2

I don't see why you need to make your reusable method an action method. Why not just a private method that returns void/bool/etc indicating the result of the save, and let your public action method return the RedirectToAction()? Effectively it's the same result, but I think it's a clearer approach.

public ActionResult DeletePage(int id)
{
        var page = _pagesRepository.GetPage(id);

        if (page == null)
                return View("NotFound");

        DeletePage(page);
        return RedirectToAction("Index");
}

//reusable method
private void DeletePage(Page page)
{
    //your same validation/save logic here
}

In the future you might consider moving this private DeletePage method into a separate service class that performs the validation and/or saving. Returning an an ActionResult would definitely not make sense in that case, so I think this example would be a more appropriate approach for your scenario.

Answer 3

Personally I agree with Kurt. The concept of Deleting an unprotected page should be decoupled from what action the controller should perform. Secondly it's confusing from the code what should happen when the page is protected. In one action it redirects to the index, in the second it redirects to the "IllegalOperation" view. Personally I'd do something a little like...

public ActionResult DeletePage(int id) {
    var page = _pagesRepository.GetPage(id);
    if (!PageIsValidForDeletion(page)) {
         string invalidPageView = FindViewForInvalidPage(page);
         return View(invalidPageView);
    }
    DeletePage(page);
    return RedirectToAction("Index");
}