Return to Answer

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

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

(this is just copy of my answer to your question from SO) http://stackoverflow.com/questions/27687208/looking-for-best-practice-to-handle-conditional-logic-inside-controller-actions/27697379#27697379

usually you don't use action filter 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; }

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