Today I decided to give optional parameters in C# a try. I’ve created a unit-test helper method that allows me to create a new build definition in Team Foundation Server. The method itself accepts three parameters, two of them are optional. So I created a method signature like this:
1: public void CreateLegacyBuild(string teamProjectName,string buildDefinitionName,string buildDefinitionPath = "...")
2: {
3: // Implementation of the method
4: }
This saves you the trouble creating an extra overload to provide a value for the optional parameter. If you do this however, you will get a code analysis warning with the code: CA1026. This warning tells you not to use default parameters, but instead supply an overload for the method.
CA1026 : Microsoft.Design : Replace method ‘…’ with an overload that supplies all default arguments.
It looks like optional parameters are not so great after all. Instead of saving time it only gives FxCop more material to start arguing about. Or is there more to this warning than meets the eye?
What are optional parameters
Before I start complaining about FxCop doing a bad job, I will tell you about optional parameters and how they work. This is important, because it tells that little bit extra that you need to understand what is really going on with the code analysis warning I talked about in the previous section.
Default parameters are best explained using an example:
1: public void DoSomething(int firstParameter, int secondParameter = 10, int thirdParameter = 20)
2: {
3: //TODO: Do something interesting
4: }
As you can see for two parameters of the method there’s a default value specified in the method signature. There three ways in which you can invoke a method that has optional parameters. One way is by skipping the optional parameters altogether like this:
1: DoSomething(10);
The second way to invoke a method with optional parameters is to supply values for the optional parameters, as if you were using the maximum overloaded version of the method.
1: DoSomething(20,30,40);
The last way in which you can invoke a method with optional parameters is to name the optional parameters. This allows you to skip certain optional parameters while supplying a value for the ones that you need.
1: DoSomething(10, thirdParameter: 60);
What you can’t do with optional parameters is skipping them like you would in VB6. For example, the following call would be invalid in C#.
1: DoSomething(10,,60);
Compiler semantics
Now that you know how optional parameters work, let’s take a closer look at how the compiler is handling optional parameters.
When the compiler compiles the source file containing the method with the optional parameters, it will embed the default value as a .param instruction in the method itself. This .param instruction is NOT used during runtime, but is there as a form of metadata.
If you add a call to a method in your own code, the compiler will bake the default value for optional parameters into the MSIL code for the method call as if you were providing values for them.
The way the compiler processes optional parameters is somewhat tricky. If you change the default values for the optional parameters and recompile the code for the method, the actual default values in your code that invokes the method will not get updated.
Best practices
In the previous section if shown what happens in the compiler when you use optional parameters. An important conclusion from this is that the default value for an optional parameter becomes part of the “contract” of your assembly. This is however invisible to the user. Also, if you change the default value of an optional parameter you are most likely introducing bugs into your own code.
The quotes around the contract bit are on purpose, because after changing the default values for the optional parameters change, the contract doesn’t look any different to the user. The user doesn’t get any hints telling him the default value has changed. In fact, if the user doesn’t compile his code, the implementation of your method will break.
My advice for using optional parameters: Only use them for methods that have an accessibility modifier of internal or less. For other scenarios I suggest you introduce overloads to make the API more accessible to developers.
Conclusion
So what’s the verdict on code analysis warning CA1026? The problem identified with this rule is real and you shouldn’t exclude it without giving your code a closer inspection. If you get this warning you should consider changing your code, because there’s nothing worse than implementations that break because of invisible changes.
One other thing that I’ve learned from this experience is this: New features are cool, but more often than not make things more complicated. We really should be really careful introducing them.
I hope this article helps getting more insight into why FxCop is a good idea and what goes behind some of the seemingly less relevant rules.
6 comments
Great post! The devil is in the details (read compiler semantics). I think you might have saved some future time for me 🙂
Jos van Dixhoorn
We found out it can be pretty useful for unittesting. A method creates and return some “dataset” from the parameters using the default values. You can than use the named parameters to test specific scenarios inside a unittest.
But I agree this is not very useful when writing a public API.
manuelr
Just checked the same code in VB.NET and found the same “issue” there too. Makes sense since VB.NET has to generate the same MSIL code as the C# compiler would be generating. For those interested in the nasty bits, here’s the MSIL of the generated method.
.method public instance void Greet([opt] string Name) cil managed
{
.param [1] = “World”
// Code size 20 (0x14)
.maxstack 8
IL_0000: nop
IL_0001: ldstr “Hello ”
IL_0006: ldarg.1
IL_0007: call string [mscorlib]System.String::Concat(string,
string)
IL_000c: call void [mscorlib]System.Console::WriteLine(string)
IL_0011: nop
IL_0012: nop
IL_0013: ret
}
willemm
Interesting find. Maybe we should make stuff internal and private by default, unless there is a specific reason for things to be public (or protected of course).
Jonathan Mezach
We should definitely pay attention to public/protected bits in our API’s, but it’s more important for things like the software factory than it is for applications we put out to customers.
Keep in mind though, that there might be API’s that you never intended to be reused, but that are reused nonetheless. So try to avoid things like optional parameters if you can. Or at least, know what you’re in for when you do use them 😉
willemm
“In fact, if the user doesn’t compile his code, the implementation of your method will break.”
Technically “Break” is wrong, what will actually happen:
The dependent code will function EXACTLY as it did before!
In fact, I believe most people would evaluate that as good thing. I would see it as a break, much later in the process when I recompile my code and deploy again. but I would be regression testing at that point and find the error, before the deploy actually occurs. So it’s no loss.
Which is, in a way, great. I can drop in a new version of a class library I depend on, and I won’t have to do a full regression on my code. I am safe knowing that that all my “Literal” values have remained unchanged.
I believe this to not be considered a bug, but correct. If you want client literals to change or need the values to be calculated at runtime, use overloads. If you want the literals to remain the same, because the are generated at compile time, then you use Optional parameters. It’s a subtle difference, but it gives API designer’s a bit more flexibility.
Example:
public TRecord[] FindiItems(int pageSize, int pageIndex = 0)
{
}
which makes it nice. you can say FindItems(20); FindItems(21, 0); FindItems(22, 1);
And in most cases, I believe most API designer’s would only use the default value of a data type as optional parameters’ value.
Examples: default(string) = null; default(int) = 0; whenever there is an optional Int32, its value “should be” 0, and string’s value “should be” null.
In the above example making PageSize an optional parameter is probably what the FxCop warning is trying to prevent. It’s not intuitive to the consumer what the value “should” be. At least not without inspecting the MSIL First, (which is what VS and the compiler do, but notepad and people don’t).
With that commentary in mind… I suppress them if the default value == default(TItem), and only makes sense to ever be the default, as is the case with the page Index example. And I change them when the criteria isn’t met.
Leat Hakkor