This keyword

[Dave]

Hi,

I need a little help in understanding why FxCop complains about this.

If I have a basic class like:


class Class1
{
static void Main()
{
Class1 class1 = new Class1();
class1.Do("Something");
}

public void Do(string PassedInString)
{
Console.WriteLine("String passed in is " + PassedInString);
}
}


Problem is that although this compiles OK and works as expected (most of my
C# books are full of examples just like this) FxCop complains because of an
unused parameter in the Do method. It basically says to use the "this"
keyword or make the method static.

In a project I have FxCop reports some methods and not others so I am unsure
what it is actually reporting, whether it is a problem and what the best
solution should be.

Thanks,
Dave.

 

[Guest]

As good as it is, FxCop is not perfect, an example of that would be the
following warning that I received when I ran it over an assembly built with
your code:

'Class1' is an internal class that is apparently never instantiated. If so,
remove the code from the assembly. If this class is intended to contain only
static methods, consider adding a private constructor to prevent the compiler
from generating a default constructor.

From the looks of it, because of the static constructor it thinks that more
of the class should be static which given the example you gave would not be a
bad idea, unless you really need to create an instance of the class within
itself.

Brendan

 

[Mattias Sjögren]

From the looks of it, because of the static constructor it thinks that more
of the class should be static which given the example you gave would not be a
bad idea, unless you really need to create an instance of the class within
itself.

I don't think it's because of the static constructor, but rather
becuase Do() doesn't operate on any instance fields. If you don't use
any instance data you may as well make the method static.


Mattias

 

[Dave]

I don't think it's because of the static constructor, but rather
becuase Do() doesn't operate on any instance fields. If you don't use
any instance data you may as well make the method static.

OK, so it sounds like its just saying that I'm not using any instance fields
and therefore its better for performance reasons to make it static and save
the class instance creation, rather than the parameter is unused and
requires This?

 

[Bruce Wood]

Yes and no.

Yes, FxCop is suggesting that you make the method static because it
doesn't use any instance fields.

No, it's not for efficiency reasons.

FxCop is occasionally concerned with efficiency (viz its constant
admonishments to use str.Length == 0 rather than str == ""). However,
more than that it's concerned with standards and, ultimately,
readability.

In this case, what FxCop is telling you is that static methods are
inherently easier to read and internalize than instance methods. The
complaint doesn't really hold for one-line methods, but for larger
methods, here is why.

A large instance method is free to use class members anywhere inside
it. If you want to find out whether an instance method uses any object
state, or modifies any object state, then you have to read all of the
code. There is no clue in the method declaration that it might use or
change object state (as held in the object's instance fields).

A static method, on the other hand, guarantees by its very nature that
the only things it can get its hands on are the arguments passed to it,
and any static fields, properties, or methods that happen to be out
there. It can't (directly) modify object state. So, as soon as you see
the word "static" against a method, you trust it not to monkey with
your object's state. Sometimes, that's all you wanted to know: which
method is messing up the this._frobnitz field? You can automatically
rule out all static methods as potential culprits.

That's why, if you have an instance method that doesn't reference any
object state, and isn't virtual (and so cannot be overridden to refer
to any object state), then FxCop recommends that you make it static. In
the end, it comes down to a style thing: is this really a little
"helper" method that does a small job and doesn't need object state? If
so, why not make it static? I even make some private methods static if
they read only one or two instance fields: I just change the methods to
take the values as arguments and then make the methods static.

What I ask myself is: is this method really an _operation on the
object_, or is it a helper? If it's a helper, then I think it's clearer
to make it static. If it really is, logically speaking, an operation on
the object itself, then I leave it as an instance method.

 

[Dave]

Yes and no.

Yes, FxCop is suggesting that you make the method static because it
doesn't use any instance fields.

No, it's not for efficiency reasons.

FxCop is occasionally concerned with efficiency (viz its constant
admonishments to use str.Length == 0 rather than str == ""). However,
more than that it's concerned with standards and, ultimately,
readability.

In this case, what FxCop is telling you is that static methods are
inherently easier to read and internalize than instance methods. The
complaint doesn't really hold for one-line methods, but for larger
methods, here is why.

A large instance method is free to use class members anywhere inside
it. If you want to find out whether an instance method uses any object
state, or modifies any object state, then you have to read all of the
code. There is no clue in the method declaration that it might use or
change object state (as held in the object's instance fields).

A static method, on the other hand, guarantees by its very nature that
the only things it can get its hands on are the arguments passed to it,
and any static fields, properties, or methods that happen to be out
there. It can't (directly) modify object state. So, as soon as you see
the word "static" against a method, you trust it not to monkey with
your object's state. Sometimes, that's all you wanted to know: which
method is messing up the this._frobnitz field? You can automatically
rule out all static methods as potential culprits.

That's why, if you have an instance method that doesn't reference any
object state, and isn't virtual (and so cannot be overridden to refer
to any object state), then FxCop recommends that you make it static. In
the end, it comes down to a style thing: is this really a little
"helper" method that does a small job and doesn't need object state? If
so, why not make it static? I even make some private methods static if
they read only one or two instance fields: I just change the methods to
take the values as arguments and then make the methods static.

What I ask myself is: is this method really an _operation on the
object_, or is it a helper? If it's a helper, then I think it's clearer
to make it static. If it really is, logically speaking, an operation on
the object itself, then I leave it as an instance method.

Thanks for the info. I said efficiency really because FxCop flagged this
under the Performance category but I understand now why these particular
methods may be better as static.

Dave.