Do not specify default parameters on virtual methods

[amanda-mitchell]

Default parameters work by using OptionalAttribute and DefaultParameterValueAttribute, and the defaults are specified at compile-time based on the exact type of the object on which the method is called. That is, if you have default parameters specified both on an interface IWidget and a concrete class Widget, the defaults that you get are dependent on whether your variable is declared as IWidget or as Widget.

public interface IWidget
{
  void DoSomething(int parameter = 0);
}

public sealed class Widget : IWidget
{
  public void DoSomething(int parameter = 1)
  {
    Console.WriteLine(parameter);
  }
}

…

IWidget a = new Widget();
Widget b = new Widget();

a.DoSomething(); // prints 0
b.DoSomething(); // prints 1

Similarly, if IWidget has a method with default parameters specified and Widget does not declare those default parameters, then you cannot use default parameters if you have a variable of type Widget.

public interface IWidget
{
  void DoSomething(int parameter = 0);
}

public sealed class Widget : IWidget
{
  public void DoSomething(int parameter)
  {
    Console.WriteLine(parameter);
  }
}

…

IWidget a = new Widget();
Widget b = new Widget();

a.DoSomething(); // prints 0
b.DoSomething(); // compiler error

These rules are exactly the same for virtual methods of classes.

Because of the potential for confusion, this leads me to the conclusion that default parameters should not be used for virtual members. (including interfaces)

[EthanRutherford]

An alternate suggestion for consideration:
Since the issue here is declarations and definitions having inconsistent default parameters (and even different parameter names), Add an analyzer that throws errors if the parameter names and defaults of a method definition do not match their declarations.

[amanda-mitchell]

For those that have access to our corporate VPN, this was originally discussed here: https://git/Logos/Discourse/blob/a90f4a343b42270bcde9ce4acf86a114504a2d1f/guideline-do-not-use-default-parameters-on-virtual-members.md

[amanda-mitchell]

As I consider this more, I'm somewhat inclined to ban default parameters on all public members, too.

I've seen way too many poor API decisions based on overuse of default parameters, and only allowing them on non-public, non-virtual members would at least contain poor interface decisions to a manageable scope.

Interested in hearing from @bgrainger, @ejball, and @ddunkin on this one. Am I overly biased against default parameters?

[ejball]

I am extremely biased against default parameters set to anything but null or default. An analyzer error for that scenario would be great.

[bgrainger]

My biggest objection is when existing public APIs get updated with new default parameters (I'm guessing because that's the "easiest" way to do it without having to update all calling code). I make myself ask "If I were designing this API from scratch, with no existing callers, would I do it this way?" Often, the answer is "no" and the right approach is to implement the right API (in a binary-backwards compatible way) even if it's more difficult.

[EthanRutherford]

I'm a fan of the ideology of providing "sensible defaults"; that is, when a parameter or other thing which could be classified as "configuration" has one value which is used in the majority of cases, provide that value by default.

The easiest (and in my opinion, most readable) way to express this is with a default value, though it can technically be achieved with an overload. I'd prefer not to have to do this using overloads, but I suppose I could learn to live with it.

[bgrainger]

I'm somewhat inclined to ban default parameters on all public members, too.

This seems a little extreme. AFAIK, the corefx team is permitting them on new APIs.

[amanda-mitchell]

@bgrainger, is there a different guideline/rule that you would propose regarding default parameters? The criticism "seems a little extreme" doesn't provide much that I can engage with.

That being said, here are a few more thoughts on this topic.

The corefx team is permitting them on new APIs.

The corefx team also has, AFAIK, a rigorous design process that is applied to new APIs.

Without such a process in place, good API design depends much more on individual discipline and wisdom. Based on the default parameter usages I've encountered, I'm not persuaded that we, as a department, are well-equipped to use them responsibly.

I've encountered several instances in which usage of default parameters was poorly thought out—and where I suspect that not having them available might have pushed developers to create better designs. I cannot recall any instances in which the use of the feature struck me as clearly cleaner/superior to a solution that did not use it.

The guideline "Don't use default parameters on public or virtual methods" is in line with the principles

• Simple guidelines should be preferred over complex guidelines, and
• Objective guidelines should be preferred over subjective guidelines

Allowing them for non-public, concrete methods still leaves a lot of room for experimentation, and if we discover patterns that we would like to use in public APIs, it would be less disruptive to make an "extreme" rule more permissive than to make a permissive rule more restrictive.

I am extremely biased against default parameters set to anything but null or default. An analyzer error for that scenario would be great.

I'm totally on board with this, regardless of the direction that we go with the other proposals on the table.

[amanda-mitchell]

To provide a little bit more concrete justification on my end:

My biggest concern with default parameters is that each new default parameter exponentially increases the surface area of a method, as opposed to method overloads, which provide linear increases: a method with four overloads tends to be more easily understood than a method with four default parameters. (especially if each of the overloads delegates to a private helper, as is common)

Visual Studio's tooling around default parameters also leaves much to be desired. Upon discovering a method that has "too many" default parameters, it is extremely tedious to discover which sets of parameters are actually used/valid.

[bgrainger]

The corefx team also has, AFAIK, a rigorous design process that is applied to new APIs.

True; everything in corefx has to go through manual API review. Lack of that has led to some poorly-designed APIs.

The concerns that you've outlined are valid, and enforcing a strict, simple guideline seems reasonable (except for existing code that violates it).

In some cases, a Settings parameter is a simple "workaround" for this rule. It solves the "find usages" problem, but doesn't really solve the exponential complexity problem.

[amanda-mitchell]

One exception that was pointed out to me at lunch:

I’m totally on board with default parameters on public controller methods.

[ddunkin]

I avoid default parameter values other than default because any other value is usually not obvious, but I don't have a strong opinion on it. I don't recall ever debugging a problem that resulted from default parameter values.