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

S1694: Promote C# to SonarWay and remove protected constructor #3960

Merged
merged 6 commits into from
Jun 10, 2024

Conversation

pavel-mikula-sonarsource
Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-sonarsource commented Jun 4, 2024

Related to SonarSource/sonar-dotnet#8795

Review

A dedicated reviewer checked the rule description successfully for:

  • logical errors and incorrect information
  • information gaps and missing content
  • text style and tone
  • PR summary and labels follow the guidelines

Copy link
Contributor

@CristianAmbrosini CristianAmbrosini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a How to Fix it and Resources section

rules/S1694/csharp/rule.adoc Outdated Show resolved Hide resolved
rules/S1694/csharp/rule.adoc Outdated Show resolved Hide resolved
Copy link
Contributor

@CristianAmbrosini CristianAmbrosini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Left a few minor comments and 1 question


* https://en.wikipedia.org/wiki/Multiple_inheritance[**Multiple Inheritance**]: Unlike classes, an interface doesn't count towards the single inheritance limit in C#. This means a class can implement multiple interfaces, which can be useful when you need to define behavior that can be shared across multiple classes.
* https://en.wikipedia.org/wiki/Loose_coupling#In_programming[**Loose Coupling**]: Interfaces provide a way to achieve loose coupling between classes. This is because an interface only specifies what methods a class must have, but not how they are implemented. This makes it easier to swap out implementations without changing the code that uses them.
* https://en.wikipedia.org/wiki/Polymorphism_(computer_science)[**Polymorphism**]: Interfaces allow you to use polymorphism, which means you can use an interface type to refer to any object that implements that interface. This can be useful when you want to write code that can work with any class that implements a certain interface, _without knowing what the actual class is_.
Copy link
Contributor

@CristianAmbrosini CristianAmbrosini Jun 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this the same for abstract classes? (referring to Polymorphism) Example

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not any class, only classes that don't derive from anything, and as such can derive from the pure abstract class.
In your example, if Dog derived from the abstract class Pet, it could not derive from Animal.


=== Noncompliant code example
`abstract` classes that contain non-abstract methods, in addition to `abstract` ones, cannot easily be converted to interfaces, and are not the subject of this rule:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we mention fields as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

abstract void Move();
abstract void Feed();
}
private bool switchLamp = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this field makes it compliant

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mentioned in a dedicated paragraph on field.

Copy link
Contributor

@zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few questions/polishing.

rules/S1694/csharp/rule.adoc Show resolved Hide resolved
rules/S1694/csharp/rule.adoc Outdated Show resolved Hide resolved
rules/S1694/csharp/rule.adoc Show resolved Hide resolved
Copy link

Quality Gate passed Quality Gate passed for 'rspec-tools'

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

Copy link

Quality Gate passed Quality Gate passed for 'rspec-frontend'

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

Copy link
Contributor

@zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@antonioaversa antonioaversa merged commit c871969 into master Jun 10, 2024
10 of 11 checks passed
@antonioaversa antonioaversa deleted the Pavel/S1694 branch June 10, 2024 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants