Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor default parameters on interface, virtual methods #104

Open
JeremyCaney opened this issue Mar 21, 2022 · 1 comment
Open

Refactor default parameters on interface, virtual methods #104

JeremyCaney opened this issue Mar 21, 2022 · 1 comment
Assignees
Labels
Priority: 1 Severity 1: Minor Status 0: Discussion Needs further evaluation of requirements and prioritization. Status 2: Scheduled Planned for an upcoming release. Type: Improvement Improves the functionality or interface of an existing feature.
Milestone

Comments

@JeremyCaney
Copy link
Member

In “Framework Design Patterns”, under 5.1 “General Member Design Guidelines”, the following recommendation should be followed:

  • DO NOT use default parameters, for any parameter type other than CancellationToken, on interface methods or virtual methods on classes.

This is inconsistent with how OnTopic is written. The recommended resolution is to instead implement the following:

Default parameters can be provided for interface methods via extension methods.

This relies on the method resolution logic of the C# compiler.

At minimum, this should be implemented on classes intended for dependency injection. For classes not intended for dependency injection, this may not be appropriate. See considerations below regarding using scope.

@JeremyCaney JeremyCaney added Severity 1: Minor Priority: 1 Type: Improvement Improves the functionality or interface of an existing feature. Status 0: Discussion Needs further evaluation of requirements and prioritization. Status 2: Scheduled Planned for an upcoming release. labels Mar 21, 2022
@JeremyCaney JeremyCaney added this to the OnTopic 5.2.0 milestone Mar 21, 2022
@JeremyCaney JeremyCaney self-assigned this Mar 21, 2022
@JeremyCaney
Copy link
Member Author

The downside of this is that these overloads are only available if the namespace of the extension method is in scope.

That can be a problem when derived classes relying on those overloads are in a different namespace. E.g.,

  • OnTopic.Repositories.ITopicRepository
  • OnTopic.Data.Sql.SqlTopicRepository

In this case, if the caller only applies a using statement for OnTopic.Data.Sql.SqlTopicRepository, then the defaults won’t be available.

That said, this issue is at least mitigated for interfaces and/or abstract classes intended for use with dependency injection, such as ITopicRepository, since code references for dependencies should only be aware of the interface or abstract class, and not the concrete implementations, and will thus use the using statement for the interface (e.g., OnTopic.Repositories).

The only exception to this is in the composition root, when assembling the dependency graph, where there will be references to the concrete class. (This also applies when registering dependencies via a container.) Since constructors for dependency injection shouldn’t have optional or default parameters, however, this doesn’t affect that case.

This concern, however, may impact classes which are subject to extensibility, but not subject to dependency injection.

The alternative in those cases is to continue to rely on defaults on the interface and virtual methods, as we do today. As “Framework Design Guidelines” warns, however:

A disagreement between an interface declaration and the implementation on a default value creates an unnecessary source of confusion for your users.

As such, this should be carefully considered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: 1 Severity 1: Minor Status 0: Discussion Needs further evaluation of requirements and prioritization. Status 2: Scheduled Planned for an upcoming release. Type: Improvement Improves the functionality or interface of an existing feature.
Projects
None yet
Development

No branches or pull requests

1 participant