Can anyone explain the purpose and resolution of CA1822 on a Controller Action?

Question

Error CA1822 : Microsoft.Performance : The 'this' parameter (or 'Me' in Visual Basic) of 'SomeController.AnAction(string, string)' is never used. Mark the member as static (or Shared in Visual Basic) or use 'this'/'Me' in the method body or at least one property accessor, if appropriate.

A static action yields 404 not found when requested via URL. The action is working as expected with code analysis turned off. What's the point of this and what's the appropriate remedy?

Note that the return type of the action is PartialViewResult, it doesn't seem as though code analysis complains about this if the return type is ActionResult.

    [HttpPost]
    public PartialViewResult BlankEditorRow(string formId, Common.Entities.Common.ObjTypeEnum objType)
    {
        if (objType == Common.Entities.Common.ObjTypeEnum.Regular)
            return new AjaxPartialViewResult("_RowEditor", new ProcedureEntryEntity()) { UpdateValidationForFormId = formId };
        else
            return new AjaxPartialViewResult("_TemplateRowEditor", new ProcedureEntryEntity()) { UpdateValidationForFormId = formId };
    } 

Update: Looks like changing the return type to ActionResult resolves the error, and PartialViewResult is an ActionResult so it should work.

Answer 1

I doubt that changing the return type without calling using any instance members really resolves the problem. I suspect that in order to change the return type, you changed the return statement to something which accessed an instance member.

Now I don't know whether the route handling in MVC will let you mark the method as static, but it's worth investigating. Even though the warning is given in terms of performance, I would think of it in terms of intent and readability.

Typically there are two reasons for a method or property to be an instance member (rather than static):

• It needs to access another instance member, because the way it behaves depends on the state of the object
• It needs to behave polymorphically based on the actual type of the instance it's called on, so that the behaviour can be overridden

If neither of these is the case, then the method can be made static which indicates that there's no polymorphism expected and no instance state required. A static member effectively advertises that the only state it depends upon is the state of the type itself (and the parameters), and that it won't behave polymorphically. Aside from anything else, that means you can test it without creating an instance at all, too.

Of course, if MVC's infrastructure requires it to be an instance method, then you should just suppress the warning, with a comment to indicate that fact.