I’ve recently caught an odd behaviour in my own coding: At some point in the last year I began creating protected static methods to implement the logic of my public methods, ensuring that the protected static version contains all of the logic, but is not affected by state.
e.g.
class Demo
{
string someAttribute;
public Demo(string someAttribute)
{
this.someAttribute = someAttribute;
}
// public method only calls the static method, passing all
// parameters sent to the public function as well as any
// of the object's attributes required by the function.
public string AppendSomeAttribute(string textToAppendTo) =>
AppendSomeAttribute(textToAppendTo, someAttribute);
// static method is not publicly exposed. It contains all
// of the function's logic, but is not affected by state
protected static string AppendSomeAttribute(string textToAppendTo, string someAttribute) =>
string.Concat(textToAppendTo, someAttribute);
/*
// this is what a normal person would do:
public string AppendSomeAttribute(string textToAppendTo) =>
string.Concat(textToAppendTo, someAttribute);
*/
}
I can’t recall reading anything that would have triggered this change. My best guess is I read something advising it which stuck in my subconscious; but it may just be a bad habit I’d unconsciously picked up.
Trying to work out why this may be advised I came up with the following:
- All logic is handled in a single place; so where there are multiple public methods taking different parameters this helps keep all logic in one routine (definitely makes sense where the version of that routine which supplies all parameters is not supposed to be publicly exposed; otherwise I can’t see a benefit).
- By removing the object’s state from the equation the logic of these methods is more easily tested / proven; either by unit tests, or when copy-pasting code to LinqPad (or similar) to try it out in a sandbox.
However, those aren’t great justifications for doing this as a de-facto coding style (i.e. where there isn’t a specific scenario to justify this, such as the many public overloads of the same method with none taking all parameters example, as outlined above).
Question:
Is there any clear benefit to writing code in this way?
2
This would be OK, provided you do have descendants that meaningfully share that logic.
The thing with protected is that it sends a message, it raises expectations. And all too often, after some digging, I find no descendants are using those protected members and it appears it was just a matter of “just in case I will need to access this later from a hypothetical future descendant” (that never saw the light of day). This is not good.
Any use of protected should fit the inheritance model in the sense that it clearly represents common logic for all child classes. Then it makes sense and helps the reader understand the application.
5
protected static methods are an odd thing to create. I suspect you mean to create private static methods. Being protected, your comment “static method is not publicly exposed” isn’t 100% correct. It can be accessed via child class as shown below:
using System;
public class Program
{
static void Main()
{
var x = new Demo2();
Console.WriteLine(x.AppendSomeAttribute("We hacked it. "));
// Prints "We hacked it. lol"
}
}
class Demo
{
private string _someAttribute;
public Demo(string someAttribute) => _someAttribute = someAttribute;
public string AppendSomeAttribute(string textToAppendTo) =>
AppendSomeAttribute(textToAppendTo, _someAttribute);
protected static string AppendSomeAttribute(string txtToAppendTo, string someAttrbute) =>
string.Concat(txtToAppendTo, someAttrbute);
}
class Demo2 : Demo
{
public Demo2() : base("lol") {}
public static string LetsHack(string textToAppendTo, string someAttribute) =>
AppendSomeAttribute(textToAppendTo, someAttribute);
}
You can see in action, here.
Private helper methods are a common pattern. Marking them as static when they do not access the object’s state is also a common pattern. protected static is just plain weird though and I’m sure this is the first time I’ve ever seen anyone use it.
1
This is common practice when writing multi-thread safe classes, although I would normally avoid inheritance as much as possible in such a class. If the static method is entirely pure (side effect free), it can be invoked from outside of mutex protected blocks, which is a big advantage when you are trying to avoid nested locks.
The main use case (according to your own explanation) is to avoid using instance values. But I see no reason to keep the method as static after it is developed.
- Develop a (non-static) method
- Put it to
staticto see if any errors pop up in the method body, which indicates you’re using instance values. - Remove the
static.
I’m all for clever methods to factcheck something, but I see no reason to persist your clever-but-unusual approach after you’ve confirmed what you wanted to.
As a secondary argument, if you already can’t keep track of the values used in a method, the method is probably handling too much to begin with.
By removing the object’s state from the equation the logic of these methods is more easily tested / proven; either by unit tests,
I’m not quite following here. Static methods (by themselves) are notoriously hard to unit test because it’s not easy to mock a state.
A static wrapped in a non-static is no different from an allround non-static setup; in terms of unit testing.
All logic is handled in a single place; so where there are multiple public methods taking different parameters this helps keep all logic in one routine (definitely makes sense where the version of that routine which supplies all parameters is not supposed to be publicly exposed; otherwise I can’t see a benefit).
This is an argument for public/protected/private, not static/instance. You can achieve the exact same goal with protected/private instance methods.
I simply don’t quite see the point of your code. Nothing changes if you remove the static keyword from your example.
In your code comments, you say that [protected] static method is not publicly exposed, which is a mislabeling. It’s not publicly exposed because it is protected, regardless of whether it’s static or not.
If you’re talking about how a static method is not exposed on an instance; that is partially true. But that is an argument about avoiding developer error, not logical correctness.
Furthermore: a protected static is still publicly available inside its class and all classes that derive from it. It’s not available as this.MyStaticMethod(), but it is still available as MyStaticMethod(). You’re not completely circumventing its availability.
What you say isn’t wrong, but I see no argument that justifies this additional level of abstraction.