From 873ad08b3674b06093cb05a6db43c01f48e4ec51 Mon Sep 17 00:00:00 2001 From: Renaud T <125455319+renaud-tognelli-sonarsource@users.noreply.github.com> Date: Thu, 12 Oct 2023 18:11:39 +0200 Subject: [PATCH] Modify S1104: Migrate To LayC - avoid public field (#3238) --- rules/S1104/compliant.adoc | 20 ----------- rules/S1104/csharp/rule.adoc | 64 +++++++++++++++++++++++++---------- rules/S1104/description.adoc | 4 +-- rules/S1104/impacts.adoc | 7 ++++ rules/S1104/java/rule.adoc | 58 +++++++++++++++++++++++++++++-- rules/S1104/noncompliant.adoc | 12 ------- rules/S1104/rule.adoc | 14 -------- 7 files changed, 110 insertions(+), 69 deletions(-) delete mode 100644 rules/S1104/compliant.adoc create mode 100644 rules/S1104/impacts.adoc delete mode 100644 rules/S1104/noncompliant.adoc delete mode 100644 rules/S1104/rule.adoc diff --git a/rules/S1104/compliant.adoc b/rules/S1104/compliant.adoc deleted file mode 100644 index 4f5cdb3d80a..00000000000 --- a/rules/S1104/compliant.adoc +++ /dev/null @@ -1,20 +0,0 @@ -=== Compliant solution - -[source,text] ----- -public class MyClass { - - public static final int SOME_CONSTANT = 0; // Compliant - constants are not checked - - private String firstName; // Compliant - - public String getFirstName() { - return firstName; - } - - public void setFirstName(String firstName) { - this.firstName = firstName; - } - -} ----- diff --git a/rules/S1104/csharp/rule.adoc b/rules/S1104/csharp/rule.adoc index e4589717012..5e102d80943 100644 --- a/rules/S1104/csharp/rule.adoc +++ b/rules/S1104/csharp/rule.adoc @@ -1,47 +1,75 @@ == Why is this an issue? -Public fields in public classes do not respect the encapsulation principle and has three main disadvantages: +include::../description.adoc[] -* Additional behavior such as validation cannot be added. -* The internal representation is exposed, and cannot be changed afterwards. -* Member values are subject to change from anywhere in the code and may not meet the programmer's assumptions. +Note that due to optimizations on simple properties, public fields provide only very little performance gain. + +include::../impacts.adoc[] -By using private fields and public properties (set and get), unauthorized modifications are prevented. Properties also benefit from additional protection (security) features such as Link Demands. +=== Exceptions +Fields marked as `readonly` or `const` are ignored by this rule. -Note that due to optimizations on simple properties, public fields provide only very little performance gain. +Fields inside classes or structs annotated with the `StructLayoutAttribute` are ignored by this rule. + +== How to fix it + +Depending on your needs: + +* Use auto-implemented properties: + +For common cases, where no validation is required, auto-implemented properties are a good alternative to fields: these allows fine grained access control and offers the flexibility to add validation or change internal storage afterwards. +__Note:__ as a bonus it is now possible to monitor value changes using breakpoints. -=== Noncompliant code example +* Encapsulate the fields in your class. To do so: -[source,csharp] + . Make the field private. + . Use public properties (set and get) to access and modify the field. + +* Mark field as `readonly` or `const`. + +=== Code examples + +==== Noncompliant code example + +[source,csharp,diff-id=1,diff-type=noncompliant] ---- public class Foo { - public int instanceData = 32; // Noncompliant + public int InstanceData = 32; // Noncompliant + public int AnotherInstanceData = 32; // Noncompliant + } ---- -=== Compliant solution +==== Compliant solution -[source,csharp] +[source,csharp,diff-id=1,diff-type=compliant] ---- public class Foo { - private int instanceData = 32; + // using auto-implemented properties + public int InstanceData { get; set; } = 32; - public int InstanceData + // using field encapsulation + private int _anotherInstanceData = 32; + + public int AnotherInstanceData { - get { return instanceData; } - set { instanceData = value ; } + get { return _anotherInstanceData; } + set + { + // perform validation + _anotherInstanceData = value; + } } + } ---- -=== Exceptions +=== Pitfalls -Fields marked as ``++readonly++`` or ``++const++`` are ignored by this rule. +Please be aware that changing a field by a property in a software that uses serialization could lead to binary incompatibility. -Fields inside classes or structs annotated with the ``++StructLayoutAttribute++`` are ignored by this rule. include::../see.adoc[] diff --git a/rules/S1104/description.adoc b/rules/S1104/description.adoc index 3c27e3ef4e4..f20d71225c1 100644 --- a/rules/S1104/description.adoc +++ b/rules/S1104/description.adoc @@ -1,8 +1,8 @@ -Public class variable fields do not respect the encapsulation principle and has three main disadvantages: +Public fields in public classes do not respect the encapsulation principle and have three main disadvantages: * Additional behavior such as validation cannot be added. * The internal representation is exposed, and cannot be changed afterwards. * Member values are subject to change from anywhere in the code and may not meet the programmer's assumptions. -By using private attributes and accessor methods (set and get), unauthorized modifications are prevented. +To prevent unauthorized modifications, private attributes and accessor methods (set and get) should be used. \ No newline at end of file diff --git a/rules/S1104/impacts.adoc b/rules/S1104/impacts.adoc new file mode 100644 index 00000000000..1e98c1e95c4 --- /dev/null +++ b/rules/S1104/impacts.adoc @@ -0,0 +1,7 @@ +=== What is the potential impact? + +Public fields can be modified by any part of the code and this can lead to unexpected changes and hard-to-trace bugs. + +Public fields don't hide the implementation details. As a consequence, it is no longer possible to change how the data is stored internally without impacting the client code of the class. + +The code is harder to maintain. diff --git a/rules/S1104/java/rule.adoc b/rules/S1104/java/rule.adoc index 86e118cc9d4..76d970ba115 100644 --- a/rules/S1104/java/rule.adoc +++ b/rules/S1104/java/rule.adoc @@ -2,13 +2,65 @@ include::../description.adoc[] -include::../noncompliant.adoc[] +include::../impacts.adoc[] -include::../compliant.adoc[] === Exceptions -Because they are not modifiable, this rule ignores ``++public final++`` fields. Also, annotated fields, whatever the annotation(s) will be ignored, as annotations are often used by injection frameworks, which in exchange require having public fields. +This rule ignores `public final` fields because they are not modifiable. Also, annotated fields, whatever the annotation(s) will be ignored, as annotations are often used by injection frameworks, which in exchange require having public fields. + +== How to fix it + +Depending on your need there are multiple options: + +* Encapsulate the field + . Make the field private. + . Define methods to get and set the value of the field. + + These methods are commonly known as getter and setter methods and are prefixed by `get` and `set` followed by the name of the field. __Note:__ as a bonus it is now possible to monitor value changes using breakpoints. +* Mark the field as `public final` if it is not supposed to change. + +=== Code examples + +==== Noncompliant code example + +[source,java,diff-id=1,diff-type=noncompliant] +---- +public class MyClass { + + public static final int SOME_CONSTANT = 0; // Compliant - constants are not checked + + public String firstName; // Noncompliant + +} +---- + +==== Compliant solution + +[source,java,diff-id=1,diff-type=compliant] +---- +public class MyClass { + + public static final int SOME_CONSTANT = 0; // Compliant - constants are not checked + + private String firstName; + + public String getFirstName() { + return firstName; + } + + public void setFirstName(String firstName) { + this.firstName = firstName; + } + +} +---- + +=== How does this work? + +By having a setter and a getter the code can control how the field is accessed and modified. +For example, adding validation in the setter method will ensure that only valid values are set. + +The access modifiers on the setter can also be changed to `private` or `protected` to restrain which code can modify the value. include::../see.adoc[] diff --git a/rules/S1104/noncompliant.adoc b/rules/S1104/noncompliant.adoc deleted file mode 100644 index 1e7923d922c..00000000000 --- a/rules/S1104/noncompliant.adoc +++ /dev/null @@ -1,12 +0,0 @@ -=== Noncompliant code example - -[source,text] ----- -public class MyClass { - - public static final int SOME_CONSTANT = 0; // Compliant - constants are not checked - - public String firstName; // Noncompliant - -} ----- diff --git a/rules/S1104/rule.adoc b/rules/S1104/rule.adoc deleted file mode 100644 index dbd99ae2be3..00000000000 --- a/rules/S1104/rule.adoc +++ /dev/null @@ -1,14 +0,0 @@ -== Why is this an issue? - -include::description.adoc[] - -include::noncompliant.adoc[] - -include::compliant.adoc[] - -=== Exceptions - -Because they are not modifiable, this rule ignores ``++public final++`` fields. - - -include::see.adoc[]