asp.net-mvc-4 - Looking for best practice to handle conditional logic inside controller actions in asp.net mvc

Question

Currently I am looking for best practice in handling conditions inside the controller actions in asp.net mvc. For example -

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 conditions marked in bold is 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. So, could someone please advise ?

Regards, Ram

Answer 1

通常，您不会将操作过滤器用于 Web 应用程序的所谓业务逻辑——这就是控制器的用途。动作过滤器适用于实际逻辑之外的所有内容 - 常见情况是日志记录、性能测量、检查用户是否经过身份验证/授权（我不认为这是你的情况，尽管你在 "物品”）。

减少代码通常是一件好事，但在这种情况下，我认为将逻辑付诸行动并不是一个好方法，因为你实际上让它有点不可读，在我看来，不可读的代码比重复的代码更糟糕。此外，特别是在您的情况下，对于您实际调用 _todoListItemsRepository.Find() 两次（对于每个有效项目）的所有有效项目，如果这是一些网络服务调用或数据库查找，这可能会很昂贵。

如果代码只是在整个操作中重复，请使用它创建一个方法，例如：

private View ValidateItem(Item) {
    if (Item == null)
      return View("NotFound");
   if (!Item.IsAuthorized())
      return View("NotValidOwner");
return null; }