From c24d13b88ada4163e9ee0c9b4b697e031258f58f Mon Sep 17 00:00:00 2001 From: Angelo Buono Date: Wed, 11 Oct 2023 08:59:57 +0200 Subject: [PATCH] Modify S1199: Migrate to LayC - Nested code blocks should not be used (#3233) Co-authored-by: Marco Borgeaud <89914223+marco-antognini-sonarsource@users.noreply.github.com> --- rules/S1199/cfamily/rule.adoc | 30 ++++++---- rules/S1199/csharp/rule.adoc | 27 ++++++--- rules/S1199/description.adoc | 7 ++- rules/S1199/java/rule.adoc | 94 ++++++++++++++++++++------------ rules/S1199/javascript/rule.adoc | 84 +++++++++++++++------------- rules/S1199/rule.adoc | 19 +++++-- 6 files changed, 164 insertions(+), 97 deletions(-) diff --git a/rules/S1199/cfamily/rule.adoc b/rules/S1199/cfamily/rule.adoc index 968d13850d..64c90cca34 100644 --- a/rules/S1199/cfamily/rule.adoc +++ b/rules/S1199/cfamily/rule.adoc @@ -1,16 +1,20 @@ == Why is this an issue? -Nested code blocks can be used to create a new scope: variables declared within that block cannot be accessed from the outside, and their lifetime end at the end of the block. +include::../description.adoc[] -While this might seem convenient, using this feature in a function often indicates that it has too many responsibilities and should be refactored into smaller functions. +However, nested code blocks are acceptable when they encapsulate all statements within a `switch` (a `case xxx:` or a `default:`) +to prevent variable declarations from interfering with other `cases`. -A nested code block is acceptable when it surrounds all the statements inside an alternative of a `switch` (a `case xxx:` or a `default:`) because it prevents variable declarations from polluting other `cases`. +== How to fix it -=== Noncompliant code example +The nested code blocks should be extracted into separate methods. -[source,cpp] +=== Code examples + +==== Noncompliant code example + +[source,cpp,diff-id=1,diff-type=noncompliant] ---- - void f(Cache &c, int data) { int value; { // Noncompliant @@ -37,9 +41,9 @@ void f(Cache &c, int data) { } ---- -=== Compliant solution +==== Compliant solution -[source,cpp] +[source,cpp,diff-id=1,diff-type=compliant] ---- int getValue(Cache &c, int data) { std::scoped_lock l(c.getMutex()); @@ -66,8 +70,14 @@ void f(Cache &c, int data) { case 2: // ... } +} ---- +== Resources + +=== Documentation +* Wikipedia - https://en.wikipedia.org/wiki/Single-responsibility_principle[Single Responsibility Principle] + ifdef::env-github,rspecator-view[] ''' @@ -80,6 +90,4 @@ include::../message.adoc[] == Comments And Links (visible only on this page) -include::../comments-and-links.adoc[] - -endif::env-github,rspecator-view[] +include::../comments-and-links.adoc[] \ No newline at end of file diff --git a/rules/S1199/csharp/rule.adoc b/rules/S1199/csharp/rule.adoc index f1c441ba99..45b288cb61 100644 --- a/rules/S1199/csharp/rule.adoc +++ b/rules/S1199/csharp/rule.adoc @@ -2,9 +2,19 @@ include::../description.adoc[] -=== Noncompliant code example +=== Exceptions -[source,csharp] +The usage of a code block after a `case` is allowed. + +== How to fix it + +The nested code blocks should be extracted into separate methods. + +=== Code examples + +==== Noncompliant code example + +[source,csharp,diff-id=1,diff-type=noncompliant] ---- public void Evaluate() { @@ -19,9 +29,9 @@ public void Evaluate() } ---- -=== Compliant solution +==== Compliant solution -[source,csharp] +[source,csharp,diff-id=1,diff-type=compliant] ---- public void Evaluate() { @@ -39,9 +49,10 @@ private void StackAdd() } ---- -=== Exceptions +== Resources -The usage of a code block after a "case" is allowed for this rule. +=== Documentation +* Wikipedia - https://en.wikipedia.org/wiki/Single-responsibility_principle[Single Responsibility Principle] ifdef::env-github,rspecator-view[] @@ -55,6 +66,4 @@ include::../message.adoc[] == Comments And Links (visible only on this page) -include::../comments-and-links.adoc[] - -endif::env-github,rspecator-view[] +include::../comments-and-links.adoc[] \ No newline at end of file diff --git a/rules/S1199/description.adoc b/rules/S1199/description.adoc index 6bc32d89d3..9ac1fab139 100644 --- a/rules/S1199/description.adoc +++ b/rules/S1199/description.adoc @@ -1 +1,6 @@ -Nested code blocks can be used to create a new scope and restrict the visibility of the variables defined inside it. Using this feature in a method typically indicates that the method has too many responsibilities, and should be refactored into smaller methods. +Nested code blocks create new scopes where variables declared within are inaccessible from the outside, and their lifespan ends with the block. + +Although this may appear beneficial, their usage within a function often suggests that the function is overloaded. +Thus, it may violate the Single Responsibility Principle, and the function needs to be broken down into smaller functions. + +The presence of nested blocks that don't affect the control flow might suggest possible mistakes in the code. diff --git a/rules/S1199/java/rule.adoc b/rules/S1199/java/rule.adoc index 3b9be50fc0..ed2d2408cc 100644 --- a/rules/S1199/java/rule.adoc +++ b/rules/S1199/java/rule.adoc @@ -2,50 +2,74 @@ include::../description.adoc[] -=== Noncompliant code example +=== Exceptions -[source,java] +The usage of a code block after a `case` is allowed. + +== How to fix it + +The nested code blocks should be extracted into separate methods. + +=== Code examples + +==== Noncompliant code example + +[source,java,diff-id=1,diff-type=noncompliant] ---- -public void evaluate(int operator) { - // Do some computation... - { - int a = stack.pop(); - int b = stack.pop(); - int result = a + b; - stack.push(result); - } +class Example { + + private final Deque stack = new LinkedList<>(); + + public void evaluate(int operator) { + switch (operator) { + case ADD: { + /* ... */ + { // Noncompliant - Extract this nested code block into a method + int a = stack.pop(); + int b = stack.pop(); + int result = a + b; + stack.push(result); + } + /* ... */ + break; + } + /* ... */ + } + } } ---- -=== Compliant solution +==== Compliant solution -[source,java] +[source,java,diff-id=1,diff-type=compliant] ---- -public void evaluate(int operator) { - // Do some computation... - evaluateAdd(); -} +class Example { -private void evaluateAdd() { - int a = stack.pop(); - int b = stack.pop(); - int result = a + b; - stack.push(result); + private final Deque stack = new LinkedList<>(); + + public void evaluate(int operator) { + switch (operator) { + case ADD: { + /* ... */ + evaluateAdd(); + /* ... */ + break; + } + /* ... */ + } + } + + private void evaluateAdd() { + int a = stack.pop(); + int b = stack.pop(); + int result = a + b; + stack.push(result); + } } ---- -ifdef::env-github,rspecator-view[] +== Resources -''' -== Implementation Specification -(visible only on this page) - -include::../message.adoc[] - -''' -== Comments And Links -(visible only on this page) - -include::../comments-and-links.adoc[] - -endif::env-github,rspecator-view[] +=== Documentation +* Wikipedia - https://en.wikipedia.org/wiki/Single-responsibility_principle[Single Responsibility Principle] +* Baeldung - https://www.baeldung.com/java-single-responsibility-principle[Single Responsibility Principle] diff --git a/rules/S1199/javascript/rule.adoc b/rules/S1199/javascript/rule.adoc index dda7fae4af..f17dda919c 100644 --- a/rules/S1199/javascript/rule.adoc +++ b/rules/S1199/javascript/rule.adoc @@ -1,37 +1,13 @@ == Why is this an issue? -Nested code blocks can be used to create a new scope: variables declared within that block cannot be accessed from the outside, and their lifetime end at the end of the block. However this only happens when you use ES6 `let` or `const` keywords, a class declaration or a function declaration (in strict mode). Otherwise the nested block is redundant and should be removed. - -The presense of redundant blocks (the ones which are not part of control flow and do not create a new scope) is confusing and may point to errors in the code. - -[source,javascript,diff-id=1,diff-type=noncompliant] ----- -{ // Noncompliant: redundant code block - var foo = bar(); -} - -if (condition) { - doSomething(); - { // Noncompliant: redundant code block - doOtherStuff(); - } -} ----- - -To fix your code remove redundant blocks. - -[source,javascript,diff-id=1,diff-type=compliant] ----- -var foo = bar(); - -if (condition) { - doSomething(); - doOtherStuff(); -} ----- +Nested code blocks can be used to create a new scope: variables declared within that block cannot be accessed from the outside, +and their lifetime end at the end of the block. However, this only happens when you use ES6 `let` or `const` keywords, +a class declaration or a function declaration (in strict mode). Otherwise, the nested block is redundant and should be removed. === Exceptions +The rule does not apply to the following cases: + * Block statements containing variable declarations using `let` or `const` keywords or class declarations are not redundant as they create a new scope. [source,javascript] @@ -60,14 +36,48 @@ if (condition) { } ---- +== How to fix it + +The nested code blocks should be extracted into separate methods. + +=== Code examples + +==== Noncompliant code example + +[source,javascript,diff-id=1,diff-type=noncompliant] +---- +{ // Noncompliant: redundant code block + var foo = bar(); +} + +if (condition) { + doSomething(); + { // Noncompliant: redundant code block + doOtherStuff(); + } +} +---- + +==== Compliant solution + +[source,javascript,diff-id=1,diff-type=compliant] +---- +var foo = bar(); + +if (condition) { + doSomething(); + doOtherStuff(); +} +---- == Resources -=== Documentation -* https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/block[MDN - block statement] -* https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/var[MDN - var] -* https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let[MDN - let] -* https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/const[MDN - const] -* https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/class[MDN - class declaration] -* https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/function[MDN - function declaration] -* https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode[MDN - strict mode] \ No newline at end of file +=== Documentation +* Wikipedia - https://en.wikipedia.org/wiki/Single-responsibility_principle[Single Responsibility Principle] +* MDN web docs - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/block[block statement] +* MDN web docs - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/var[var] +* MDN web docs - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let[let] +* MDN web docs - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/const[const] +* MDN web docs - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/class[class declaration] +* MDN web docs - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/function[function declaration] +* MDN web docs - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode[strict mode] \ No newline at end of file diff --git a/rules/S1199/rule.adoc b/rules/S1199/rule.adoc index f47a161a52..57844e8cbf 100644 --- a/rules/S1199/rule.adoc +++ b/rules/S1199/rule.adoc @@ -2,9 +2,15 @@ include::description.adoc[] -=== Noncompliant code example +== How to fix it -[source,text] +The superfluous code blocks should be extracted into separate methods + +=== Code examples + +==== Noncompliant code example + +[source,text,diff-id=1,diff-type=noncompliant] ---- public void evaluate(int operator) { switch (operator) { @@ -21,10 +27,11 @@ public void evaluate(int operator) { } ---- +==== Compliant solution -=== Compliant solution +To fix your code remove redundant blocks. -[source,text] +[source,text,diff-id=1,diff-type=compliant] ---- public void evaluate(int operator) { switch (operator) { @@ -44,3 +51,7 @@ private void evaluateAdd() { } ---- +== Resources + +=== Documentation +* https://en.wikipedia.org/wiki/Single-responsibility_principle[Single Responsibility Principle | Wikipedia] \ No newline at end of file