When should we implement a custom MVC ActionFilter?

Should we move logic that supposes to be in Controller (like the data to render the partial view) to ActionFilter?

For example, I'm making a CMS web site. There should be a advertisement block to be rendered on several pages but not all the pages. Should I make an ActionFilter attribute like [ShowAd(categoryId)] and decorate the action methods with this attribute?

The implementation of this controller would include service calls to retrieve information from database, build开发者_高级运维up view models and put in the ViewData. There would be a HtmlHelper to render the partial view using the data in ViewData if it exists.

That just seems yucky to me.

When I'm trying to figure out whether I need an ActionFilter, the first question I have is, Is this a cross-cutting concern?. Your particular use-case doesn't fit this, at first blush. The reason is, is that an ad is just another thing to render on a page. There's nothing special about it that makes it cross-cutting. If you replaced the word 'Ad' with 'Product' in your question, all the same facts would be true.

So there's that, and then there's the separation of concerns and testability. How testable are your controllers once you have this ActionFilter in place? It's something else you've got to mock out when testing, and what's worse is that you have to mock out those dependencies with every controller you add the ActionFilter to.

The second question I ask is, "How can I do this in a way that seems most idiomatic in the platform I'm using?"

For this particular problem, it sounds like a RenderAction and an AdController is the way to go.

Here's why:

1. An Ad is its own resource; it normally isn't closely tied to anything else on the page; it exists in its own little world, as it were.
2. It has its own data-access strategy
3. You don't really want to repeat the code to generate an Ad in every place you could use it (which is where a RenderPartial approach would take you)

So here's what such a beast would look like:

public AdController : Controller
{
    //DI'd in
    private AdRepository AdRepository;

    [ChildActionOnly]
    public ActionResult ShowAd(int categoryId)
    {
        Ad ad = Adrepository.GetAdByCategory(categoryId);
        AdViewModel avm = new AdViewModel(ad);
        return View(avm);
    }
}

Then you could have a custom partial view that is set up around this, and there's no need to put a filter on every action (or every controller), and you don't have try to fit a square peg (an action filter) in a round hole (a dynamic view).

Adding an Ad to an existing page then becomes really easy:

<% Html.RenderAction("ShowAd", "Ad" new { categoryId = Model.CategoryId }); %>

If your ad system is simple enough, there is no reason you could/should not use an action filter to insert enough info into the view data to generate the ad in your view code.

For a simple ad system, say.. a single ad of a specific category shows up in the same place in the layout on every page and that's it, then there is no real argument of a better way except to prepare for future changes to the system. While those concerns may be legitimate, you may also have it on good authority that requirement will never change. But, even if requirements do change, having wrapped all the code that generates ads in one place is the most important aspect and will save you much more time up front than a more robust solution might. Obviously there are more than a few ways to wrap this code in a single place.

As for the way you are choosing to do it, I would keep your action filter cleaner to only have it insert the category into the view data and have all the magic happen inside your html helper which would take the category in as a parameter. Building up view models to shove into the view data is going to require a bit of extra work, and put code all over the place when it doesn't need to be there. Keep it simple and do all of the html generation inside of the html helper which is responsible for...building html.