Should C# methods that can be static be static?
We were discussing this today and I'm kind of on the fence. Imagine you have a long method that you refactor a few lines out of. The new method probably takes a few local variables from the parent method and returns a value. This means it could be static.
The question is: should it be static? It's not static by design or choice, simply by its nature in that it doesn't reference any instance values.
Static methods are faster than the non-static ones so yes, they should be static if they can and there is no special reason for leaving them nonstatic.
I think it would make it a bit more readable if you marked it as static...Then someone who comes along would know that it doesn't reference any instance variables without having to read the entire function...
It depends but generally I do not make those methods static. Code is always changing and perhaps someday I will want to make that function virtual and override it in a subclass. Or perhaps some day it will need to reference instance variables. It will be harder to make those changes if every call site has to be changed.
Not necessarily.
Moving public methods from static to non-static is a breaking change, and would require changes to all of your callers or consumers. If a method seems like an instance method, but happens to not use any instance members, I would suggest making it an instance method as a measure of future-proofing.
In those cases, i tend to move the method to a static or utils library, so i don't be mixing the concept of the "object" with the concept of "class"
I would not make it a public static member of that class. The reason is that making it public static is saying something about the class' type: "This type knows how to do this behavior." And odds are the behavior no longer has any real relationship with the larger type.
That doesn't mean I wouldn't make it static at all, though. Ask yourself this: could the new method logically belong elsewhere? If you can answer "yes" to that, you probably do want to make it static (and move it as well). Even if that's not true, you could still make it static. Just don't mark it public.
As a matter of convenience, you could at least mark it internal. This typically avoids needing to move the method if you don't have easy access to a more appropriate type, but still leaves it accessible where needed in a way that it won't show up as part of the public interface to users of your class.
I suggest that the best way to think about it is this: If you need a class method that needs to be called when no instances of the class are instantioated, or maintains some kind of global state, then static is a good idea. But in general, I suggest you should prefer making members non-static.
Making something static just because you can is not a good idea. Static methods should be static due to their design, not due to happenstance.
Like Michael said, changing this later will break code that's using it.
With that said, it sounds like you are creating a private utility function for the class that is, in fact, static by design.
Yes. The reason "it can be static" is that it does not operate on the state of the object upon which it is called. Therefore it is not an instance method, but a class method. If it can do what it needs to do without ever accessing the data for the instance, then it should be static.
Yes, it should. There are various metrics of coupling that measure how your class depends on other things, like other classes, methods, etc. Making methods static is a way to keep the degree of coupling down, since you can be sure a static method does not reference any members.
If you don't need multiple instances I would recommend using a singleton class instead of a static class.
Then you can allow multiple instances in the future without having to change the code in the clients.
Personally, I'm a great fan of statelessness. Does your method need access to the state of the class? If the answer is no (and it is probably no, otherwise you wouldn't consider making it a static method), then yeah, go for it.
No access to state is less headache. Just as it is a good idea to hide private members that are not needed by other classes, it is a good idea to hide the state from members that don't need it. Reduced access can mean less bugs. Also, it makes threading easier as it is much easier to keep static members thread-safe. There is also a performance consideration as the runtime does not need to pass a reference to this as a parameter for static methods.
Of course the downside is that if you ever find that your previously static method will have to access the state for some reason, then you have to change it. Now I understand that this can be a problem for public APIs so if this is a public method in a public class, then perhaps you should think about the implications of this a bit. Still, I've never faced a situtation in the real world where this actually caused a problem, but maybe I'm just lucky.
So yeah, go for it, by all means.
Personally I would have no choice but to make it static. Resharper issues a warning in this case and our PM has a rule "No warnings from the Resharper".
If you were able to refactor a few lines out and the resulting method could be static, it is probably an indication that the lines you pulled out of that method don't belong in the containing class at all, and you should consider moving them into their own class.
It depends. There are really 2 types of static methods:
In a small to medium size code base you can really treat the two methods interchangeably.
If you have a method that is in the first category (can-be-static), and you need to change it to access class state, it's relatively straight forward to figure out if it's possible to turn the static method into a instance method.
In a large code base, however, the sheer number of call sites might make searching to see if it's possible to convert a static method to a non static one too costly. Many times people will see the number of calls, and say "ok... I better not change this method, but instead create a new one that does what I need".
That can result in either:
Both of those things are bad.
So, my advice would be that if you have a code base over 200K LOC, that I would only make methods static if they are must-be-static methods.
The refactoring from non-static to static is relatively easy (just add a keyword), so if you want to make a can-be-static into an actual static later (when you need it's functionality outside of an instance) then you can. However, the inverse refactoring, turning a can-be-static into a instance method is MUCH more expensive.
With large code bases it's better to error on the side of ease of extension, rather than on the side of idealogical purity.
So, for big projects don't make things static unless you need them to be. For small projects, just do what ever you like best.
You should think about your methods and classes:
If the last two are 'yes', then your method/class should probably be static.
The most used example is probably the Math class. Every major OO language has it and all the methods are static. Because you need to be able to use them anywhere, anytime, without making an instance.
Another good example is the Reverse() method in C#.
This is a static method in the Array class. It reverses the order of your array.
Code:
public static void Reverse(Array array)
It doesn't even return anything, your array is reversed, because all arrays are instances of the Array class.
As long as you make the new method private static it is not a breaking change. In fact, FxCop includes this guidance as one of its rules (http://msdn.microsoft.com/en-us/library/ms245046(VS.80).aspx), with the following information:
After you mark the methods as static, the compiler will emit non-virtual call sites to these members. Emitting non-virtual call sites will prevent a check at runtime for each call that ensures that the current object pointer is non-null. This can result in a measurable performance gain for performance-sensitive code. In some cases, the failure to access the current object instance represents a correctness issue.
That being said, the first comment from David Kean more succinctly summarizes the concerns by saying this is actually more about being correct than about the performance gain:
Although this rule is classified as a performance issue, the performance improvement of making a method static is only around 1%. Rather, it is more a correctness issue that could indicate an either an incomplete or a bug in the member by its failure to use other instance members. Marking a method static (Shared in Visual Basic) makes it clear on its intention not to touch instance state.
I would definitely turn anything I can into static for a different reason:
Static functions, when JIT'd, are called without a "this" parameter. That means, for example, that a 3 parameter non-static function (member method) gets pushed with 4 params on the stack.
The same function compiled as a static function would get called with 3 parameters. This can free up registers for the JIT and conserve stack space...
I'm in the "only make private methods static" camp. Making a public method can introduce coupling that you don't want and may decrease testability: You can't stub a public static method.
If you want to unit test a method that uses a public static method, you end up testing the static method as well, which might not be what you want.
I am surprised that so few are mentioning encapsulation here in fact. An instance method will automatically have access to all private (instance) fields, properties and methods. In addition to all protected ones inherited from base classes.
When you write code you should write it so that you expose as little as possible and also so that you have access to as little as possible.
So yes, it might be important to make your code fast which would happen if you're making your methods static, but usually more important then that is to make your code as incapable of creating bugs as possible too. One way to achieve that is to have your code have access to as little as possible of "private stuff".
This might seem irrelevant at first glance since the OP is obviously talking about refactoring which can not go wrong in this scenario and create any new bugs, however this refactored code must be maintained in the future and modified which makes your code have a bigger "attack surface" in regards to new bugs if it has access to private instance members. So in general I think the conclusion here is that "yes mostly your methods should be static" unless there are any other reasons for not having them static. And this simply because it's "better use of encapsulation and data hiding and creates 'safer' code"...
I look at it generally from a functional perspective of pure functions. Does it need to be an instance method? If not, you may benefit from forcing the user to pass in the variables and not mangling the current instance's state. (Well, you could still mangle state, but the point is to, by design, not do so.) I generally design instance methods as public members and do my best to make private members static. If necessary (you can then more easily extract them into other classes later.