3
\$\begingroup\$

I am looking for best practice in handling conditions inside the controller actions in ASP.NET MVC:

public ActionResult Edit(int Id = 0)
{
   var Item = _todoListItemsRepository.Find(Id);
   if (Item == null)
      return View("NotFound");
   if (!Item.IsAuthorized())
      return View("NotValidOwner");

   return View("Edit", Item);
}

The above two ifs are used in other actions inside the controller. So, in order not to repeat these conditions in all the actions. I have used the below approach:

[HttpGet]       
[Authorize]
[ModelStatusActionFilter]
public ActionResult Edit(int Id = 0)
{
    var Item = _todoListItemsRepository.Find(Id);        
    return View("Edit", Item);
}


public class ModelStatusActionFilterAttribute : ActionFilterAttribute
{
    private readonly ITodoListItemsRepository _todoListItemsRepository;
    public ModelStatusActionFilterAttribute()
        : this(new TodoListItemsRepository())
    {

    }
    public ModelStatusActionFilterAttribute(ITodoListItemsRepository     todoListItemsRepository)
    {
        _todoListItemsRepository = todoListItemsRepository;
    }
    public override void OnActionExecuting(ActionExecutingContext filterContext)
    {
        try
        {
            var Id = Convert.ToInt32(filterContext.RouteData.Values["Id"]);
            var Item = _todoListItemsRepository.Find(Id);
            if (Item == null)
            {
                filterContext.Result = new ViewResult() { ViewName = "NotFound" };
            }
            else if (!Item.IsAuthorized())
            {
                filterContext.Result = new ViewResult() { ViewName = "NotValidOwner" };
            }
        }
        catch
        {

        }
    }                
}

I am unsure if this is the best practice in handling such scenarios. Could someone please advise?

\$\endgroup\$
3
\$\begingroup\$

This is just copy of my answer to your question on SO.

Usually you don't use action filters for so-called business logic of your web application - this is what the controllers are for. Action filter are rather for the whole stuff which is external to the actual logic - common case is logging, performance measurement, checking if user is authenticated / authorized (I don't think this is your case, although you call IsAuthorized method on the "Item").

Reducing code is generally good thing but in this case, I don't think putting the logic to action is a good way, because you've actually made it a bit unreadable, and unreadable code is in my opinon much worse than repeated code. Also, specifically in your case, for all valid items you actually call the _todoListItemsRepository.Find() twice (for each valid item), which might be costly if this is some webservice call or db lookup.

If the code is just repeated throughout the actions, make a method out of it like:

private View ValidateItem(Item) {
    if (Item == null)
      return View("NotFound");
   if (!Item.IsAuthorized())
      return View("NotValidOwner");
return null; }
\$\endgroup\$
  • \$\begingroup\$ "and unreadable code is in my opinon much worse than repeated code." Double plus good. Welcome to Code Review. \$\endgroup\$ – RubberDuck Dec 30 '14 at 0:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.