From cb03cd9bdf9977d8ea8b3ff2f479b4cd4e6830e7 Mon Sep 17 00:00:00 2001 From: Pavel Mikula Date: Tue, 4 Jun 2024 11:14:57 +0200 Subject: [PATCH 1/6] S1694: Promote C# to SonarWay and remove protected constructor --- rules/S1694/csharp/metadata.json | 4 +++- rules/S1694/csharp/rule.adoc | 35 ++------------------------------ 2 files changed, 5 insertions(+), 34 deletions(-) diff --git a/rules/S1694/csharp/metadata.json b/rules/S1694/csharp/metadata.json index 17971333806..d860fd4691d 100644 --- a/rules/S1694/csharp/metadata.json +++ b/rules/S1694/csharp/metadata.json @@ -1,3 +1,5 @@ { - + "defaultQualityProfiles": [ + "Sonar way" + ] } diff --git a/rules/S1694/csharp/rule.adoc b/rules/S1694/csharp/rule.adoc index 5e5a12e8da7..3e3e7a9b78f 100644 --- a/rules/S1694/csharp/rule.adoc +++ b/rules/S1694/csharp/rule.adoc @@ -2,11 +2,7 @@ The purpose of an abstract class is to provide some heritable behaviors while also defining methods which must be implemented by sub-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. - - -A ``++class++`` with only ``++abstract++`` methods and no inheritable behavior should be converted to an ``++interface++``. +A `class` with only `abstract` methods and no inheritable behavior should be converted to an `interface`. === Noncompliant code example @@ -17,18 +13,6 @@ public abstract class Animal // Noncompliant; should be an interface abstract void Move(); abstract void Feed(); } - -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 int GetRed() - { - return red; - } -} ---- === Compliant solution @@ -41,21 +25,6 @@ public interface Animal 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; @@ -81,7 +50,7 @@ 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. ''' From eef334b1e70f6f919a80211de8cc4ea824074a78 Mon Sep 17 00:00:00 2001 From: Pavel Mikula Date: Tue, 4 Jun 2024 11:17:23 +0200 Subject: [PATCH 2/6] Empty commit to rerun CI From 4b66d48fa0e7498c68e49eedebdcac7e255ff70b Mon Sep 17 00:00:00 2001 From: Antonio Aversa Date: Fri, 7 Jun 2024 17:05:45 +0200 Subject: [PATCH 3/6] Adapt to LaYC format --- rules/S1694/csharp/rule.adoc | 79 +++++++++++++++++++++++++++--------- 1 file changed, 60 insertions(+), 19 deletions(-) diff --git a/rules/S1694/csharp/rule.adoc b/rules/S1694/csharp/rule.adoc index 3e3e7a9b78f..1f7bf0c92a7 100644 --- a/rules/S1694/csharp/rule.adoc +++ b/rules/S1694/csharp/rule.adoc @@ -1,31 +1,25 @@ +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` with only `abstract` methods and no inheritable behavior should be converted to an `interface`. +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. -=== Noncompliant code example +Using interfaces over pure abstract classes presents multiple advantages: -[source,csharp] ----- -public abstract class Animal // Noncompliant; should be an interface -{ - abstract void Move(); - abstract void Feed(); -} ----- +* 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_. +* 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. -=== Compliant solution +=== Exceptions + +`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: [source,csharp] ---- -public interface Animal -{ - void Move(); - void Feed(); -} - -public abstract class Lamp +public abstract class Lamp // Compliant: Glow is abstract, but FlipSwitch is not { private bool switchLamp = false; @@ -42,6 +36,50 @@ public abstract class Lamp } ---- +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(); +} +---- + +=== Compliant solution + +[source,csharp,diff-id=1,diff-type=compliant] +---- +public interface Animal +{ + void Move(); + void Feed(); +} +---- + +== Resources + +=== Documentation + +* 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[] ''' @@ -52,6 +90,9 @@ ifdef::env-github,rspecator-view[] Convert this "abstract" (class|record) to an interface. +=== Highlighting + +The identifier of the "abstract" class. ''' == Comments And Links From 098ac29fa32c9f19f38aa987cab53509305e9066 Mon Sep 17 00:00:00 2001 From: Antonio Aversa Date: Mon, 10 Jun 2024 12:34:41 +0200 Subject: [PATCH 4/6] Code review --- rules/S1694/csharp/rule.adoc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/rules/S1694/csharp/rule.adoc b/rules/S1694/csharp/rule.adoc index 1f7bf0c92a7..34022eb712f 100644 --- a/rules/S1694/csharp/rule.adoc +++ b/rules/S1694/csharp/rule.adoc @@ -38,6 +38,8 @@ public abstract class Lamp // Compliant: Glow is abstract, but FlipSwitch is not 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. +Notice that interfaces, cannot have fields, even in C# 8.0 and upwards. That can be a valid reason to still prefer an abstract class over an interface. + == How to fix it Convert the `abstract` class to an `interface` with the same methods. From 353d9d8211400145dd6877adbef736fd47c95985 Mon Sep 17 00:00:00 2001 From: Antonio Aversa Date: Mon, 10 Jun 2024 12:36:36 +0200 Subject: [PATCH 5/6] Code review 2 --- rules/S1694/csharp/rule.adoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rules/S1694/csharp/rule.adoc b/rules/S1694/csharp/rule.adoc index 34022eb712f..26d3ce7f5f6 100644 --- a/rules/S1694/csharp/rule.adoc +++ b/rules/S1694/csharp/rule.adoc @@ -38,7 +38,7 @@ public abstract class Lamp // Compliant: Glow is abstract, but FlipSwitch is not 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. -Notice that interfaces, cannot have fields, even in C# 8.0 and upwards. That can be a valid reason to still prefer an abstract class over an interface. +However, interfaces cannot have fields (such as `switchLamp` in the example above), and that remains true even in C# 8.0 and upwards. This can be a valid reason to still prefer an abstract class over an interface. == How to fix it From aabbd70167a13095f5205018a00ace724c545cec Mon Sep 17 00:00:00 2001 From: Antonio Aversa Date: Mon, 10 Jun 2024 12:38:31 +0200 Subject: [PATCH 6/6] Code review 3 --- rules/S1694/csharp/rule.adoc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rules/S1694/csharp/rule.adoc b/rules/S1694/csharp/rule.adoc index 26d3ce7f5f6..a4588b18dfc 100644 --- a/rules/S1694/csharp/rule.adoc +++ b/rules/S1694/csharp/rule.adoc @@ -52,8 +52,8 @@ Convert the `abstract` class to an `interface` with the same methods. ---- public abstract class Animal // Noncompliant: should be an interface { - abstract void Move(); - abstract void Feed(); + public abstract void Move(); + public abstract void Feed(); } ----