Leave a Reply

6 comments

  1. 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 Reply

  2. 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 Reply

  3. 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 Reply

  4. 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 Reply

  5. 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 Reply

  6. “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 Reply