Handling conditional logic inside controller actions

Question

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?

Answer 1

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; }

• RubberDuck: "and unreadable code is in my opinon much worse than repeated code." Double plus good. Welcome to Code Review.