Do not specify default parameters on virtual methods about faithlifeanalyzers HOT 12 OPEN

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.

from faithlifeanalyzers.

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

from faithlifeanalyzers.

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?

from faithlifeanalyzers.

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

from faithlifeanalyzers.

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.

from faithlifeanalyzers.

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.

from faithlifeanalyzers.

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.

from faithlifeanalyzers.

@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.

from faithlifeanalyzers.

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.

from faithlifeanalyzers.

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.

from faithlifeanalyzers.

One exception that was pointed out to me at lunch:

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

from faithlifeanalyzers.

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.

from faithlifeanalyzers.