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>
This commit is contained in:
Angelo Buono
GitHub
parent
5ed9a4f18a
commit
c24d13b88a
@ -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[]
|
||||
@ -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[]
|
||||
@ -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.
|
||||
|
||||
@ -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<Integer> 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<Integer> 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]
|
||||
|
||||
@ -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]
|
||||
=== 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]
|
||||
@ -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]
|
||||
Loading…
x
Reference in New Issue
Block a user