How can I remove repeated code in my actions?

Question

I have the following code repeated several times in a mvc app.

    public ActionResult AnAction(int Id)
    {           
        var claim = GetClaim(Id);
        if (claim == null)
        {
            return View("ClaimNotFound");
        }

        // do stuff here
        ....
        return ....;
    }

So far this pattern is used 4 times and it's getting ugly. What is the best way to refactor it?

Edit:
A couple of example usages

    public ActionResult Claim(int Id)
    {           
        var claim = GetClaim(Id);
        if (claim == null)
        {
            return View("ClaimNotFound");
        }

        return View("Claim", claim);
    }

    public ActionResult MedicalPV(int Id)
    {
        var claim = GetClaim(Id);
        if (claim == null)
        {
            return View("ClaimNotFound");
        }

        return PartialView(claim.MedCerts.AsQueryable<MedCert>());
    }

Typically I am requiring access to the object in the view. This particular code is only used in one controller, but I am may need something similar in other controllers with different objects and views.

Answer 1

If all of the actions require a claim, then you could try to retrieve it in OnActionExecuting and set the Result to the ViewResult on failure. If only some actions, require it, perhaps, an ActionFilter that checks to make sure that a claim is available before executing the method and, if not, sets the proper View.

private Claim Claim { get; set; }

public override void OnActionExecuting( ActionExecutingContext context )
{
    this.Claim = GetClaim( int.Parse( context.RouteData["id"] ) );
    if (this.Claim == null)
    {
        context.Result = View( "ClaimNotFound" );
    }
}

OR

public class RequiresClaimIdAttribute : ActionFilterAttribute
{
    public override void OnActionExecuting( ActionExecutingContext context )
    {
         var claim = GetClaim( int.Parse( context.RouteData["id"] ) );
         if (claim == null)
         {
              context.Result = new ViewResult
              {
                   ViewName = "ClaimNotFound",
                   ViewData = context.Controller.ViewData
              };
         }
         else
         {

              var property = context.Controller
                                    .GetType()
                                    .GetProperty( "Claim",
                                                   BindingFlags.Public
                                                   | BindingFlags.NonPublic
                                                   | BindingFlags.Instance);
              if (property != null)
              {
                   property.SetValue(context.Controller,claim);
              }
         }
    }
}

[RequiresClaimId]
public ActionResult AnAction( int id )
{
    this.Claim.Updated = DateTime.Now;
    ...
}

Answer 2

If it's the claim checking logic that you want to refactor out, your best option would be to write a custom authorization filter. You'd do this by creating a class that inherits from FilterAttribute and implements IAuthorizationFilter.

IAuthorizationFilter has one method: OnAuthorization, which is where you'd put the claim checking logic. Instead of returning a ViewResult (As you do above), you'd set the Result property of the AuthorizationContext object that gets passed into the OnAuthorization method. It would look something like this:

public class RequiresClaim : FilterAttribute, IAuthorizationFilter
{
    public void OnAuthorization(AuthorizationContext filterContext)
    {
        var claim = GetClaim(filterContext.RouteData["id"]);

        if (claim == null) { filterContext.Result = new ViewResult { ViewName = "ClaimNotFound" }; }
    }
}

Once you have that filter, you could then simply put the attribute on every action method you wanted to have that claim check occur. If you wanted the claim check to occur for every action in a controller, you could just put it on the controller.

Answer 3

Update 2: I didn't realize that this question was about MVC. Sorry about that. The answer below is still valid from a more generic point of view anyway.

• The first refactoring I would do is taking the GetClaim call out of the method. Looks like AnAction could not (and, therefore should not) depend on GetClaim. It should directly receive a Claim instead;

• Secondly, I would apply the same rule to the View call. Instead, AnAction should simply not allow a null Claim.

The code would look like this:

public ActionResult AnAction(Claim claim)
{           
    if (claim == null) throw new ArgumentNullException("cliam");

    // do stuff here
    ....
    return ....;
}

You might say I simply moved out the responsibility of handling null Claims to another place. And that's exactly what I suggest you to do. The original problem was that all your several action methods were having too much responsibility. I'm only applying the Single responsibility principle.

---

Update 1: After making the refactorings I suggested, you could substitute all calls to the action methods by a call to the following method. It might not be the solution you're looking for, but it looks like a good start (at least to me).

public static ActionResult InvokeAction(Func<Claim,ActionResult> action, int id)
{
    var claim = GetClaim(Id);
    if (claim == null) return View("ClaimNotFound");
    return action(claim);
}

Answer 4

You can use custom model binder.

    public ActionResult AnAction(Claim claim)
    {
      if (!ModelState.IsValid)
        return View("Invalid claim"); 
    }

    public class ClaimModelBinder: IModelBinder
    {
      public object BindModel(BindingContext context) // don't remember exactly
      {
          var id = context.ValueProvider.GetRawValue(context.ModelName);
          if (string.IsNullOrEmpty(id))
          {
               context.ModelState.AddModelError(context.ModelName, "Empty id");
               return null;
          }
          var claim = GetClaim(id);
          if (claim == null)
          {
               context.ModelState.AddModelError(context.ModelName, "Invalid claim");
               return null;
          }
          return claim;
      }
    }

    // in global.asax.cs
    ModelBinders.Binders.Add(typeof(Claim), new ClaimCustomerBinder());

Why it's better than action filter:

• binder automatically picked up, you don't need attribute
• you can have several Claim parameters

• works for Claim properties of some other class, and even IList in case you want to pass multiple Claim objects

  But you can't handle specific actions, i.e. return View("NotFound") for this error. However, it's easy to do if you develop your own convention: for example your ModelBinder can do

     context.ModelState.AddModelError(context.ModelName, new NotFoundException("Invalid claim", "ClaimNotFound"));
  
  
     public override void OnActionExecuting(ActionExecutingContext context)
     {
         var notfound = from o in ModelState 
                        from e in o.Value.Errors where 
                        where e.Exception is NotFoundException
                        select e.Exception;
         if (notfound.Count() == 1)
             context.Result = View { Name = notfound.First().ViewName };
     }
  

Alternative just do

  if (!ModelState.IsValid)
       // this view will just show all ModelState errors
       return View("IncorrectInput");

It may seem harder, but it is a good separation of concerns - model binder maps the data from HTTP to your classes, then you are free to either check ModelState manually or rely on some automatic handling. It may look an overkill for simple apps, but if you start getting duplicates here and there, and your models become complex, model binders can greatly simplify your life, since your controller actions will get ready-to-use objects, and you can concentrate on the real business code instead of the infrastructure/plumbing.

This is (I believe) what ASP.NET MVC is for - you're free to build your own set of conventions around it.