Skip to content
This repository was archived by the owner on Feb 2, 2026. It is now read-only.

Commit 9783228

Browse files
vilchik-elenavilchik-elena
authored
SONARJS-67 Rule: Variables should not be declared and then immediately returned or thrown (SonarSource#429)
1 parent 8683cc7 commit 9783228

File tree

8 files changed

+835
-0
lines changed

8 files changed

+835
-0
lines changed

‎its/ruling/src/test/expected/javascript-S1488.json‎

Lines changed: 540 additions & 0 deletions
Large diffs are not rendered by default.

‎javascript-checks/src/main/java/org/sonar/javascript/checks/CheckList.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,7 @@ public static List<Class> getChecks() {
115115
IdChildrenSelectorCheck.class,
116116
IdenticalExpressionOnBinaryOperatorCheck.class,
117117
IfConditionalAlwaysTrueOrFalseCheck.class,
118+
ImmediatelyReturnedVariableCheck.class,
118119
IncrementDecrementInSubExpressionCheck.class,
119120
IndexOfCompareToPositiveNumberCheck.class,
120121
InOperatorTypeErrorCheck.class,

‎javascript-checks/src/main/java/org/sonar/javascript/checks/ImmediatelyReturnedVariableCheck.java‎

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
/*
2+
* SonarQube JavaScript Plugin
3+
* Copyright (C) 2011-2016 SonarSource SA
4+
* mailto:contact AT sonarsource DOT com
5+
*
6+
* This program is free software; you can redistribute it and/or
7+
* modify it under the terms of the GNU Lesser General Public
8+
* License as published by the Free Software Foundation; either
9+
* version 3 of the License, or (at your option) any later version.
10+
*
11+
* This program is distributed in the hope that it will be useful,
12+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
13+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
14+
* Lesser General Public License for more details.
15+
*
16+
* You should have received a copy of the GNU Lesser General Public License
17+
* along with this program; if not, write to the Free Software Foundation,
18+
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
19+
*/
20+
package org.sonar.javascript.checks;
21+
22+
import java.util.List;
23+
import javax.annotation.Nullable;
24+
import org.sonar.check.Rule;
25+
import org.sonar.javascript.tree.impl.SeparatedList;
26+
import org.sonar.plugins.javascript.api.tree.Tree.Kind;
27+
import org.sonar.plugins.javascript.api.tree.declaration.BindingElementTree;
28+
import org.sonar.plugins.javascript.api.tree.declaration.InitializedBindingElementTree;
29+
import org.sonar.plugins.javascript.api.tree.expression.ExpressionTree;
30+
import org.sonar.plugins.javascript.api.tree.expression.IdentifierTree;
31+
import org.sonar.plugins.javascript.api.tree.statement.BlockTree;
32+
import org.sonar.plugins.javascript.api.tree.statement.ReturnStatementTree;
33+
import org.sonar.plugins.javascript.api.tree.statement.StatementTree;
34+
import org.sonar.plugins.javascript.api.tree.statement.ThrowStatementTree;
35+
import org.sonar.plugins.javascript.api.tree.statement.VariableDeclarationTree;
36+
import org.sonar.plugins.javascript.api.tree.statement.VariableStatementTree;
37+
import org.sonar.plugins.javascript.api.visitors.DoubleDispatchVisitorCheck;
38+
39+
@Rule(key = "S1488")
40+
public class ImmediatelyReturnedVariableCheck extends DoubleDispatchVisitorCheck {
41+
42+
private static final String MESSAGE = "Immediately %s this expression instead of assigning it to the temporary variable \"%s\".";
43+
44+
@Override
45+
public void visitBlock(BlockTree tree) {
46+
List<StatementTree> statements = tree.statements();
47+
48+
if (statements.size() > 1) {
49+
50+
StatementTree lastButOneStatement = statements.get(statements.size() - 2);
51+
StatementTree lastStatement = statements.get(statements.size() - 1);
52+
53+
if (lastButOneStatement.is(Kind.VARIABLE_STATEMENT)) {
54+
checkStatements(((VariableStatementTree) lastButOneStatement).declaration(), lastStatement);
55+
}
56+
}
57+
58+
super.visitBlock(tree);
59+
}
60+
61+
private void checkStatements(VariableDeclarationTree variableDeclaration, StatementTree lastStatement) {
62+
SeparatedList<BindingElementTree> variables = variableDeclaration.variables();
63+
64+
if (variables.size() == 1 && variables.get(0).is(Kind.INITIALIZED_BINDING_ELEMENT)) {
65+
InitializedBindingElementTree initializedBindingElementTree = (InitializedBindingElementTree) variables.get(0);
66+
67+
if (initializedBindingElementTree.left().is(Kind.BINDING_IDENTIFIER)) {
68+
String name = ((IdentifierTree) initializedBindingElementTree.left()).name();
69+
70+
if (returnsVariableInLastStatement(lastStatement, name)) {
71+
addIssue(initializedBindingElementTree.right(), String.format(MESSAGE, "return", name));
72+
73+
} else if (throwsVariableInLastStatement(lastStatement, name)) {
74+
addIssue(initializedBindingElementTree.right(), String.format(MESSAGE, "throw", name));
75+
76+
}
77+
}
78+
}
79+
}
80+
81+
private static boolean returnsVariableInLastStatement(StatementTree lastStatement, String variableName) {
82+
if (lastStatement.is(Kind.RETURN_STATEMENT)) {
83+
ReturnStatementTree returnStatement = (ReturnStatementTree) lastStatement;
84+
85+
return isVariable(returnStatement.expression(), variableName);
86+
}
87+
88+
return false;
89+
}
90+
91+
private static boolean throwsVariableInLastStatement(StatementTree lastStatement, String variableName) {
92+
if (lastStatement.is(Kind.THROW_STATEMENT)) {
93+
ThrowStatementTree throwStatement = (ThrowStatementTree) lastStatement;
94+
return isVariable(throwStatement.expression(), variableName);
95+
}
96+
97+
return false;
98+
}
99+
100+
private static boolean isVariable(@Nullable ExpressionTree expressionTree, String variableName) {
101+
if (expressionTree != null && expressionTree.is(Kind.IDENTIFIER_REFERENCE)) {
102+
String thrownName = ((IdentifierTree) expressionTree).name();
103+
return thrownName.equals(variableName);
104+
}
105+
106+
return false;
107+
}
108+
109+
110+
}

‎javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S1488.html‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
<p>Declaring a variable only to immediately return or throw it is a bad practice.</p>
2+
<p>Some developers argue that the practice improves code readability, because it enables them to explicitly name what is being returned. However, this
3+
variable is an internal implementation detail that is not exposed to the callers of the method. The method name should be sufficient for callers to
4+
know exactly what will be returned.</p>
5+
<h2>Noncompliant Code Example</h2>
6+
<pre>
7+
function computeDurationInMilliseconds() {
8+
var duration = (((hours * 60) + minutes) * 60 + seconds ) * 1000 ;
9+
return duration;
10+
}
11+
</pre>
12+
<h2>Compliant Solution</h2>
13+
<pre>
14+
function computeDurationInMilliseconds() {
15+
return (((hours * 60) + minutes) * 60 + seconds ) * 1000 ;
16+
}
17+
</pre>
18+

‎javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S1488.json‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
{
2+
"title": "Local Variables should not be declared and then immediately returned or thrown",
3+
"type": "CODE_SMELL",
4+
"status": "ready",
5+
"remediation": {
6+
"func": "Constant\/Issue",
7+
"constantCost": "2min"
8+
},
9+
"tags": [
10+
"clumsy"
11+
],
12+
"defaultSeverity": "Minor"
13+
}

‎javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/Sonar_way_profile.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@
4141
"S1301",
4242
"S1442",
4343
"S1472",
44+
"S1488",
4445
"S1656",
4546
"S1751",
4647
"S1764",

‎javascript-checks/src/test/java/org/sonar/javascript/checks/ImmediatelyReturnedVariableCheckTest.java‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
/*
2+
* SonarQube JavaScript Plugin
3+
* Copyright (C) 2011-2016 SonarSource SA
4+
* mailto:contact AT sonarsource DOT com
5+
*
6+
* This program is free software; you can redistribute it and/or
7+
* modify it under the terms of the GNU Lesser General Public
8+
* License as published by the Free Software Foundation; either
9+
* version 3 of the License, or (at your option) any later version.
10+
*
11+
* This program is distributed in the hope that it will be useful,
12+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
13+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
14+
* Lesser General Public License for more details.
15+
*
16+
* You should have received a copy of the GNU Lesser General Public License
17+
* along with this program; if not, write to the Free Software Foundation,
18+
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
19+
*/
20+
package org.sonar.javascript.checks;
21+
22+
import java.io.File;
23+
import org.junit.Test;
24+
import org.sonar.javascript.checks.verifier.JavaScriptCheckVerifier;
25+
26+
public class ImmediatelyReturnedVariableCheckTest {
27+
28+
@Test
29+
public void test() {
30+
JavaScriptCheckVerifier.verify(new ImmediatelyReturnedVariableCheck(), new File("src/test/resources/checks/ImmediatelyReturnedVariable.js"));
31+
}
32+
}

‎javascript-checks/src/test/resources/checks/ImmediatelyReturnedVariable.js‎

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,120 @@
1+
function var_returned() {
2+
var x = 42; // Noncompliant {{Immediately return this expression instead of assigning it to the temporary variable "x".}}
3+
// ^^
4+
return x;
5+
}
6+
7+
function let_returned() {
8+
let x = 42; // Noncompliant
9+
return x;
10+
}
11+
12+
function const_returned() {
13+
const x = 42; // Noncompliant
14+
return x;
15+
}
16+
17+
function code_before_declaration() {
18+
foo();
19+
var x = 42; // Noncompliant
20+
return x;
21+
}
22+
23+
function thrown_nok() {
24+
const x = new Exception(); // Noncompliant {{Immediately throw this expression instead of assigning it to the temporary variable "x".}}
25+
throw x;
26+
}
27+
28+
function thrown_ok() {
29+
throw new Exception();
30+
}
31+
32+
function thrown_expression() {
33+
const x = new Exception();
34+
throw foo(x);
35+
}
36+
37+
function thrown_different_variable() {
38+
const x = new Exception();
39+
throw y;
40+
}
41+
42+
function code_between_declaration_and_return() {
43+
let x = 42;
44+
foo();
45+
return x;
46+
}
47+
48+
function return_expression() {
49+
let x = 42;
50+
return x + 5;
51+
}
52+
53+
function return_without_value() {
54+
let x = 42;
55+
return;
56+
}
57+
58+
function not_return_statement() {
59+
let x = 42;
60+
foo(x);
61+
}
62+
63+
function no_init_value() {
64+
let x;
65+
return x;
66+
}
67+
68+
function pattern_declared() {
69+
let {x} = foo();
70+
return x;
71+
}
72+
73+
function two_variables_declared() {
74+
let x = 42, y;
75+
return x;
76+
}
77+
78+
function different_variable_returned() {
79+
let x = 42;
80+
return y;
81+
}
82+
83+
function only_return() {
84+
return 42;
85+
}
86+
87+
function one_statement() {
88+
foo();
89+
}
90+
91+
function empty_block() {
92+
}
93+
94+
function different_blocks() {
95+
if (foo) {
96+
let x = foo(); // Noncompliant
97+
return x;
98+
}
99+
100+
try {
101+
let x = foo(); // Noncompliant
102+
return x;
103+
104+
} catch (e) {
105+
let x = foo(); // Noncompliant
106+
return x;
107+
108+
} finally {
109+
let x = foo(); // Noncompliant
110+
return x;
111+
}
112+
113+
114+
}
115+
116+
var arrow_function_ok = (a, b) => {
117+
return a + b;
118+
}
119+
120+
var arrow_function_no_block = (a, b) => a + b;

0 commit comments

Comments
 (0)