From bc15ffe77e53f52f22ea42bb8a028fc01148b6e8 Mon Sep 17 00:00:00 2001 From: Rudy Regazzoni <110470341+rudy-regazzoni-sonarsource@users.noreply.github.com> Date: Wed, 11 Oct 2023 13:58:56 +0200 Subject: [PATCH] Modify S109: Migrate To LayC (#3235) ## Review A dedicated reviewer checked the rule description successfully for: - [x] logical errors and incorrect information - [x] information gaps and missing content - [x] text style and tone - [x] PR summary and labels follow [the guidelines](https://github.com/SonarSource/rspec/#to-modify-an-existing-rule) --- rules/S109/abap/rule.adoc | 22 ++++++----- rules/S109/cfamily/rule.adoc | 35 +++++++----------- rules/S109/cobol/rule.adoc | 41 +++++++++++++++++++-- rules/S109/csharp/rule.adoc | 63 +++++++++++++++++--------------- rules/S109/description.adoc | 13 +++---- rules/S109/how-to-fix-it.adoc | 4 ++ rules/S109/java/rule.adoc | 40 ++++++++++---------- rules/S109/javascript/rule.adoc | 18 +++++---- rules/S109/plsql/rule.adoc | 30 ++++++++++++--- rules/S109/rpg/metadata.json | 3 -- rules/S109/rpg/rule.adoc | 46 ----------------------- rules/S109/rule.adoc | 3 -- rules/S109/vb6/rule.adoc | 25 +++++-------- rules/S109/vbnet/metadata.json | 3 -- rules/S109/vbnet/rule.adoc | 65 --------------------------------- 15 files changed, 170 insertions(+), 241 deletions(-) create mode 100644 rules/S109/how-to-fix-it.adoc delete mode 100644 rules/S109/rpg/metadata.json delete mode 100644 rules/S109/rpg/rule.adoc delete mode 100644 rules/S109/rule.adoc delete mode 100644 rules/S109/vbnet/metadata.json delete mode 100644 rules/S109/vbnet/rule.adoc diff --git a/rules/S109/abap/rule.adoc b/rules/S109/abap/rule.adoc index 173dcb1b27..78b97b89a9 100644 --- a/rules/S109/abap/rule.adoc +++ b/rules/S109/abap/rule.adoc @@ -1,23 +1,25 @@ -== Why is this an issue? - include::../description.adoc[] -=== Noncompliant code example +include::../how-to-fix-it.adoc[] -[source,abap] +=== Code examples + +==== Noncompliant code example + +[source,abap,diff-id=1,diff-type=noncompliant] ---- -IF sy-subrc EQ 42. - screen-request = 45. +IF sy-subrc EQ 42. " Noncompliant - 5 is a magic number + screen-request = 'OK'. ENDIF. ---- -=== Compliant solution +==== Compliant solution -[source,abap] +[source,abap,diff-id=1,diff-type=compliant] ---- answer = 42. -IF sy-subrc EQ answer. - screen-request = 45. +IF sy-subrc EQ answer. " Compliant + screen-request = 'OK'. ENDIF. ---- diff --git a/rules/S109/cfamily/rule.adoc b/rules/S109/cfamily/rule.adoc index d9674a0b75..3dcdb9b046 100644 --- a/rules/S109/cfamily/rule.adoc +++ b/rules/S109/cfamily/rule.adoc @@ -1,46 +1,39 @@ -== Why is this an issue? +include::../description.adoc[] -A magic number is a number that comes out of nowhere, and is directly used in a statement. Magic numbers are often used, for instance to limit the number of iterations of a loops, to test the value of a property, etc. +include::../how-to-fix-it.adoc[] +This is classically done by using a constant (`constexpr` or `const` if your compiler does not support `constexpr` yet) or an enumeration. +Note that since {cpp}20, some well known mathematical constants, such as pi, are defined in the header ``, and should be preferred over defining your own version (see S6164). -Using magic numbers may seem obvious and straightforward when you're writing a piece of code, but they are much less obvious and straightforward at debugging time. +=== Code examples +==== Noncompliant code example -That is why magic numbers must be demystified by first being assigned a name. This is classically done by using a constant (``++constexpr++`` or ``++const++`` if your compiler does not support ``++constexpr++`` yet) or an enumeration. - - --1, 0 and 1 are not considered magic numbers. - - -Note that since {cpp}20, some well known mathematical constants, such as pi, are defined in the header ``++++``, and should be preferred over defining your own version (see S6164). - -=== Noncompliant code example - -[source,cpp] +[source,cpp,diff-id=1,diff-type=noncompliant] ---- void doSomething(int var) { - for(int i = 0; i < 42; i++) { // Noncompliant - 42 is a magic number + for (int i = 0; i < 42; i++) { // Noncompliant - 42 is a magic number // ... } - if (var == 42) { // Noncompliant - magic number + if (42 == var) { // Noncompliant - magic number // ... } } ---- -=== Compliant solution +==== Compliant solution -[source,cpp] +[source,cpp,diff-id=1,diff-type=compliant] ---- enum Status { -STATUS_KO = 0, -STATUS_OK = 42, + STATUS_OK = 0, + STATUS_ERROR = 42 }; void doSomething(Status var) { constexpr int maxIterations = 42; // Compliant - in a declaration - for(int i = 0; i < maxIterations ; i++){ // Compliant: 0 is excluded, and maxIterations is a named constant + for (int i = 0; i < maxIterations; i++) { // Compliant - 0 is excluded, and maxIterations is a named constant // ... } diff --git a/rules/S109/cobol/rule.adoc b/rules/S109/cobol/rule.adoc index dea269094e..6fe89abdb2 100644 --- a/rules/S109/cobol/rule.adoc +++ b/rules/S109/cobol/rule.adoc @@ -1,10 +1,43 @@ -== Why is this an issue? +include::../description.adoc[] -A magic number is a number that comes out of nowhere, and is directly used in a statement. Magic numbers are often used, for instance to limit the number of iterations of a loops, to test the value of a property, etc. +include::../how-to-fix-it.adoc[] -Using magic numbers may seem obvious and straightforward when you're writing a piece of code, but they are much less obvious and straightforward at debugging time. +=== Code examples -That is why magic numbers must be demystified by first being assigned to clearly named variables before being used. +==== Noncompliant code example + +[source,cobol,diff-id=1,diff-type=noncompliant] +---- +DATA DIVISION. + WORKING-STORAGE SECTION. + 01 WS-COUNT PIC 9(1) VALUE 0. +PROCEDURE DIVISION. + A-PARA. + PERFORM B-PARA UNTIL WS-COUNT=5 *> Noncompliant - 5 is a magic number + STOP RUN. + + B-PARA. + DISPLAY "Count:"WS-COUNT. + ADD 1 TO WS-COUNT. +---- + +==== Compliant solution + +[source,cobol,diff-id=1,diff-type=compliant] +---- +DATA DIVISION. + WORKING-STORAGE SECTION. + 01 WS-COUNT PIC 9(1) VALUE 0. + 01 WS-NUMBER-OF-CYCLES PIC 9(1) VALUE 5. +PROCEDURE DIVISION. + A-PARA. + PERFORM B-PARA UNTIL WS-COUNT=WS-NUMBER-OF-CYCLES *> Compliant + STOP RUN. + + B-PARA. + DISPLAY "Count:"WS-COUNT. + ADD 1 TO WS-COUNT. *> Compliant - 1 is not considered a magic number +---- ifdef::env-github,rspecator-view[] diff --git a/rules/S109/csharp/rule.adoc b/rules/S109/csharp/rule.adoc index d655c536ca..8324583500 100644 --- a/rules/S109/csharp/rule.adoc +++ b/rules/S109/csharp/rule.adoc @@ -1,35 +1,5 @@ -== Why is this an issue? - include::../description.adoc[] -=== Noncompliant code example - -[source,csharp] ----- -public static void DoSomething() -{ - for(int i = 0; i < 4; i++) // Noncompliant, 4 is a magic number - { - ... - } -} ----- - -=== Compliant solution - -[source,csharp] ----- -private const int NUMBER_OF_CYCLES = 4; - -public static void DoSomething() -{ - for(int i = 0; i < NUMBER_OF_CYCLES ; i++) //Compliant - { - ... - } -} ----- - === Exceptions This rule doesn't raise an issue when the magic number is used as part of: @@ -39,6 +9,39 @@ This rule doesn't raise an issue when the magic number is used as part of: - the single argument of an attribute - a named argument for a method or attribute - a constructor call +- a default value for a method argument + +include::../how-to-fix-it.adoc[] + +=== Code examples + +==== Noncompliant code example + +[source,csharp,diff-id=1,diff-type=noncompliant] +---- +public void DoSomething() +{ + for (int i = 0; i < 4; i++) // Noncompliant, 4 is a magic number + { + ... + } +} +---- + +==== Compliant solution + +[source,csharp,diff-id=1,diff-type=compliant] +---- +private const int NUMBER_OF_CYCLES = 4; + +public void DoSomething() +{ + for (int i = 0; i < NUMBER_OF_CYCLES; i++) // Compliant + { + ... + } +} +---- ifdef::env-github,rspecator-view[] diff --git a/rules/S109/description.adoc b/rules/S109/description.adoc index adc8ccddf5..64f6c2a298 100644 --- a/rules/S109/description.adoc +++ b/rules/S109/description.adoc @@ -1,10 +1,7 @@ -A magic number is a number that comes out of nowhere, and is directly used in a statement. Magic numbers are often used, for instance to limit the number of iterations of a loop, to test the value of a property, etc. +A magic number is a hard-coded numerical value that may lack context or meaning. They should not be used because they can make the code less readable and maintainable. +== Why is this an issue? -Using magic numbers may seem obvious and straightforward when you're writing a piece of code, but they are much less obvious and straightforward at debugging time. - - -That is why magic numbers must be demystified by first being assigned to clearly named variables before being used. - - --1, 0 and 1 are not considered magic numbers. \ No newline at end of file +Magic numbers make the code more complex to understand as it requires the reader to have knowledge about the global context to understand the number itself. +Their usage may seem obvious at the moment you're writing the code, but it may not be the case for another developer or later once the context faded away. +-1, 0 and 1 are not considered magic numbers. diff --git a/rules/S109/how-to-fix-it.adoc b/rules/S109/how-to-fix-it.adoc new file mode 100644 index 0000000000..d6c3ce7983 --- /dev/null +++ b/rules/S109/how-to-fix-it.adoc @@ -0,0 +1,4 @@ +== How to fix it + +Replacing them with a constant allows us to provide a meaningful name associated with the value. +Instead of adding complexity to the code, it brings clarity and helps to understand the context and the global meaning. diff --git a/rules/S109/java/rule.adoc b/rules/S109/java/rule.adoc index b196d1007a..6c8f3b59a7 100644 --- a/rules/S109/java/rule.adoc +++ b/rules/S109/java/rule.adoc @@ -1,33 +1,35 @@ -== Why is this an issue? - include::../description.adoc[] -=== Noncompliant code example +=== Exceptions -[source,java] +This rule ignores ``++hashCode++`` methods. + +include::../how-to-fix-it.adoc[] + +=== Code examples + +==== Noncompliant code example + +[source,java,diff-id=1,diff-type=noncompliant] ---- public static void doSomething() { - for(int i = 0; i < 4; i++){ // Noncompliant, 4 is a magic number - ... - } -} ----- - -=== Compliant solution - -[source,java] ----- -public static final int NUMBER_OF_CYCLES = 4; -public static void doSomething() { - for(int i = 0; i < NUMBER_OF_CYCLES ; i++){ + for (int i = 0; i < 4; i++) { // Noncompliant, 4 is a magic number ... } } ---- -=== Exceptions +==== Compliant solution -This rule ignores ``++hashCode++`` methods. +[source,java,diff-id=1,diff-type=compliant] +---- +public static final int NUMBER_OF_CYCLES = 4; +public static void doSomething() { + for (int i = 0; i < NUMBER_OF_CYCLES ; i++) { // Compliant + ... + } +} +---- ifdef::env-github,rspecator-view[] diff --git a/rules/S109/javascript/rule.adoc b/rules/S109/javascript/rule.adoc index 046f5195cf..4903b8204f 100644 --- a/rules/S109/javascript/rule.adoc +++ b/rules/S109/javascript/rule.adoc @@ -1,25 +1,27 @@ -== Why is this an issue? - include::../description.adoc[] -=== Noncompliant code example +include::../how-to-fix-it.adoc[] -[source,javascript] +=== Code examples + +==== Noncompliant code example + +[source,javascript,diff-id=1,diff-type=noncompliant] ---- function doSomething() { - for (let i = 0; i < 4; i++) { // Noncompliant, 4 is a magic number + for (let i = 0; i < 4; i++) { // Noncompliant, 4 is a magic number // ... } } ---- -=== Compliant solution +==== Compliant solution -[source,javascript] +[source,javascript,diff-id=1,diff-type=compliant] ---- function doSomething() { const numberOfCycles = 4; - for (let i = 0; i < numberOfCycles; i++) { + for (let i = 0; i < numberOfCycles; i++) { // Compliant // ... } } diff --git a/rules/S109/plsql/rule.adoc b/rules/S109/plsql/rule.adoc index 24f9a1c9fb..9cd0428267 100644 --- a/rules/S109/plsql/rule.adoc +++ b/rules/S109/plsql/rule.adoc @@ -1,14 +1,34 @@ -== Why is this an issue? +include::../description.adoc[] -A magic number is a number that comes out of nowhere, and is directly used in a statement. Magic numbers are often used, for instance to limit the number of iterations of a loops, to test the value of a property, etc. +include::../how-to-fix-it.adoc[] +=== Code examples -Using magic numbers may seem obvious and straightforward when you're writing a piece of code, but they are much less obvious and straightforward at debugging time. +==== Noncompliant code example -That is why magic numbers must be demystified by first being assigned to clearly named variables before being used. +[source,sql,diff-id=1,diff-type=noncompliant] +---- +BEGIN + FOR l_counter IN 1..5 -- Noncompliant, 5 is a magic number + LOOP + ... + END LOOP; +END; +---- +==== Compliant solution -By default, -1, 0 and 1 are not considered magic numbers. +[source,sql,diff-id=1,diff-type=compliant] +---- +DECLARE + co_number_of_cycles CONSTANT NUMBER := 5; +BEGIN + FOR l_counter IN 1..co_number_of_cycles -- Compliant + LOOP + ... + END LOOP; +END; +---- ifdef::env-github,rspecator-view[] diff --git a/rules/S109/rpg/metadata.json b/rules/S109/rpg/metadata.json deleted file mode 100644 index 208b3a7441..0000000000 --- a/rules/S109/rpg/metadata.json +++ /dev/null @@ -1,3 +0,0 @@ -{ - "title": "Named constants should be used for indicators" -} diff --git a/rules/S109/rpg/rule.adoc b/rules/S109/rpg/rule.adoc deleted file mode 100644 index a5df0008d1..0000000000 --- a/rules/S109/rpg/rule.adoc +++ /dev/null @@ -1,46 +0,0 @@ -== Why is this an issue? - -Using a named constant to refer to an indicator makes the content of the field clearer, and therefore makes the code easier to read and maintain. - -=== Noncompliant code example - -[source,rpg] ----- - /Free - - If *In(25); // Noncompliant - // Process contents... - EndIf; ----- - -=== Compliant solution - -[source,rpg] ----- - D accountTotal c 25 - - /Free - - If *In(accountTotal); - // Process contents... - EndIf; ----- - -ifdef::env-github,rspecator-view[] - -''' -== Implementation Specification -(visible only on this page) - -=== Message - -Use a named constant for this reference to XXX. - - -''' -== Comments And Links -(visible only on this page) - -include::../comments-and-links.adoc[] - -endif::env-github,rspecator-view[] diff --git a/rules/S109/rule.adoc b/rules/S109/rule.adoc deleted file mode 100644 index 6472b78d9a..0000000000 --- a/rules/S109/rule.adoc +++ /dev/null @@ -1,3 +0,0 @@ -== Why is this an issue? - -include::description.adoc[] diff --git a/rules/S109/vb6/rule.adoc b/rules/S109/vb6/rule.adoc index deeca82d8d..3be434efb1 100644 --- a/rules/S109/vb6/rule.adoc +++ b/rules/S109/vb6/rule.adoc @@ -1,33 +1,26 @@ -== Why is this an issue? +include::../description.adoc[] -A magic number is a number that comes out of nowhere, and is directly used in a statement. Magic numbers are often used, for instance to limit the number of iterations of a loops, to test the value of a property, etc. +include::../how-to-fix-it.adoc[] +=== Code examples -Using magic numbers may seem obvious and straightforward when you're writing a piece of code, but they are much less obvious and straightforward at debugging time. +==== Noncompliant code example - -That is why magic numbers must be demystified by first being assigned to clearly named constants before being used. - - --1, 0 and 1 are not considered magic numbers. - -=== Noncompliant code example - -[source,vb6] +[source,vb6,diff-id=1,diff-type=noncompliant] ---- Function blnCheckSize(dblParameter As Double) As Boolean - If dblParameter > 1024 Then + If dblParameter > 1024 Then ' Noncompliant, 1024 is a magic number blnCheckSize = True End If End Function ---- -=== Compliant solution +==== Compliant solution -[source,vb6] +[source,vb6,diff-id=1,diff-type=compliant] ---- Function blnCheckSize(dblParameter As Double) As Boolean - Dim threshold As Integer = 1024 + Dim threshold As Integer = 1024 ' Compliant If dblParameter > threshold Then blnCheckSize = True diff --git a/rules/S109/vbnet/metadata.json b/rules/S109/vbnet/metadata.json deleted file mode 100644 index 1797133380..0000000000 --- a/rules/S109/vbnet/metadata.json +++ /dev/null @@ -1,3 +0,0 @@ -{ - -} diff --git a/rules/S109/vbnet/rule.adoc b/rules/S109/vbnet/rule.adoc deleted file mode 100644 index dad0227f91..0000000000 --- a/rules/S109/vbnet/rule.adoc +++ /dev/null @@ -1,65 +0,0 @@ -== Why is this an issue? - -A magic number is a number that comes out of nowhere, and is directly used in a statement. Magic numbers are often used, for instance to limit the number of iterations of a loops, to test the value of a property, etc. - - -Using magic numbers may seem obvious and straightforward when you're writing a piece of code, but they are much less obvious and straightforward at debugging time. - - -That is why magic numbers must be demystified by first being assigned to clearly named constants before being used. - -=== Noncompliant code example - -[source,vbnet] ----- -Class Foo - Sub DoSomething(Param As Integer) - If Param > 100 Then ' Magic Number - ' Do something - End If - End Sub -End Class ----- - -=== Compliant solution - -[source,vbnet] ----- -Class Foo - Private Const MaxOfSomething As Integer = 100 - - Sub DoSomething(Param As Integer) - If Param > MaxOfSomething Then - ' Do something - End If - End Sub -End Class ----- - -ifdef::env-github,rspecator-view[] - -''' -== Implementation Specification -(visible only on this page) - -include::../message.adoc[] - -=== Parameters - -.exceptions -**** -_STRING_ - ----- --1,0,1,2 ----- -**** - - -''' -== Comments And Links -(visible only on this page) - -include::../comments-and-links.adoc[] - -endif::env-github,rspecator-view[]