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
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion rules/S1694/csharp/metadata.json
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
{

"defaultQualityProfiles": [
"Sonar way"
]
}
102 changes: 56 additions & 46 deletions rules/S1694/csharp/rule.adoc
Original file line number Diff line number Diff line change
@@ -1,77 +1,84 @@
A `class` with only `abstract` methods and no inheritable behavior should be converted to an https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/interfaces/[`interface`].

== Why is this an issue?

The purpose of an abstract class is to provide some heritable behaviors while also defining methods which must be implemented by sub-classes.
The purpose of an https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/abstract-and-sealed-classes-and-class-members[`abstract` class] is to provide some overridable behaviors while also defining methods that are required to be implemented by sub-classes.

A class that contains only `abstract` methods, often called pure abstract class, is effectively an interface, but with the disadvantage of not being able to be implemented by multiple classes.

A ``++class++`` with no abstract methods that was made ``++abstract++`` purely to prevent instantiation should be converted to a concrete ``++class++`` (i.e. remove the ``++abstract++`` keyword) with a ``++protected++`` constructor.
Using interfaces over pure abstract classes presents multiple advantages:

* 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.
zsolt-kolbay-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
* 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.

* https://en.wikipedia.org/wiki/Design_by_contract[**Design by contract**]: Interfaces provide a clear contract of what a class should do, without specifying how it should do it. This makes it easier to understand the intended behavior of a class, and to ensure that different implementations of an interface are consistent with each other.

A ``++class++`` with only ``++abstract++`` methods and no inheritable behavior should be converted to an ``++interface++``.
=== Exceptions

=== 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

zsolt-kolbay-sonarsource marked this conversation as resolved.
Show resolved Hide resolved

[source,csharp]
----
public abstract class Animal // Noncompliant; should be an interface
public abstract class Lamp // Compliant: Glow is abstract, but FlipSwitch is not
{
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.


public abstract class Color // Noncompliant; should be concrete with a protected constructor
{
private int red = 0;
private int green = 0;
private int blue = 0;
public abstract void Glow();

public int GetRed()
public void FlipSwitch()
{
return red;
switchLamp = !switchLamp;
if (switchLamp)
{
Glow();
}
}
}
----

Notice that, since C# 8.0, you can also define https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-8.0/default-interface-methods[default implementations for interface methods], which is yet another reason to prefer interfaces over abstract classes when you don't need to provide any inheritable behavior.

== How to fix it

Convert the `abstract` class to an `interface` with the same methods.

=== Code examples

==== Noncompliant code example

[source,csharp,diff-id=1,diff-type=noncompliant]
----
public abstract class Animal // Noncompliant: should be an interface
{
abstract void Move();
abstract void Feed();
zsolt-kolbay-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
}
----

=== Compliant solution

[source,csharp]
[source,csharp,diff-id=1,diff-type=compliant]
----
public interface Animal
{
void Move();
void Feed();
}
----

public class Color
{
private int red = 0;
private int green = 0;
private int blue = 0;

protected Color()
{}

public int GetRed()
{
return red;
}
}

public abstract class Lamp
{
private bool switchLamp = false;
== Resources

public abstract void Glow();
=== Documentation

public void FlipSwitch()
{
switchLamp = !switchLamp;
if (switchLamp)
{
Glow();
}
}
}
----
* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/abstract-and-sealed-classes-and-class-members[Abstract and Sealed Classes and Class Members (C# Programming Guide)]
* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/interfaces/[Interfaces - define behavior for multiple types]
* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/interfaces/default-interface-methods[Default Interface Methods]
* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/csharp/advanced-topics/interface-implementation/default-interface-methods-versions[Tutorial: Update interfaces with default interface methods]
* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/csharp/fundamentals/object-oriented/inheritance[Inheritance - derive types to create more specialized behavior]
* Wikipedia - https://en.wikipedia.org/wiki/Multiple_inheritance[Multiple Inheritance]
* Wikipedia - https://en.wikipedia.org/wiki/Loose_coupling#In_programming[Loose Coupling - In programming]
* Wikipedia - https://en.wikipedia.org/wiki/Polymorphism_(computer_science)[Polymorphism (computer science)]
* Wikipedia - https://en.wikipedia.org/wiki/Design_by_contract[Design by contract]

ifdef::env-github,rspecator-view[]

Expand All @@ -81,8 +88,11 @@ ifdef::env-github,rspecator-view[]

=== Message

Convert this "abstract" (class|record) to (an interface|a concrete type with a private constructor).
Convert this "abstract" (class|record) to an interface.

=== Highlighting

The identifier of the "abstract" class.

'''
== Comments And Links
Expand Down