Skip to content

Commit 87dd8df

Browse files
fourlscirras
fourls
authored and
cirras
committed
Update TooManyParameters to check routine declarations instead
1 parent ec49737 commit 87dd8df

File tree

2 files changed

+177
-8
lines changed

2 files changed

+177
-8
lines changed

‎delphi-checks/src/main/java/au/com/integradev/delphi/checks/TooManyParametersCheck.java‎

Lines changed: 35 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,9 @@
2020

2121
import org.sonar.check.Rule;
2222
import org.sonar.check.RuleProperty;
23+
import org.sonar.plugins.communitydelphi.api.ast.RoutineDeclarationNode;
2324
import org.sonar.plugins.communitydelphi.api.ast.RoutineImplementationNode;
25+
import org.sonar.plugins.communitydelphi.api.ast.RoutineNameNode;
2426
import org.sonar.plugins.communitydelphi.api.check.DelphiCheck;
2527
import org.sonar.plugins.communitydelphi.api.check.DelphiCheckContext;
2628
import org.sonarsource.analyzer.commons.annotations.DeprecatedRuleKey;
@@ -42,18 +44,45 @@ public class TooManyParametersCheck extends DelphiCheck {
4244
defaultValue = "" + DEFAULT_MAXIMUM)
4345
public int constructorMax = DEFAULT_MAXIMUM;
4446

47+
@Override
48+
public DelphiCheckContext visit(RoutineDeclarationNode routine, DelphiCheckContext context) {
49+
checkRoutine(
50+
routine.getRoutineNameNode(),
51+
routine.getParameters().size(),
52+
routine.isConstructor(),
53+
context);
54+
return super.visit(routine, context);
55+
}
56+
4557
@Override
4658
public DelphiCheckContext visit(RoutineImplementationNode routine, DelphiCheckContext context) {
47-
int count = routine.getParameters().size();
48-
int limit = routine.isConstructor() ? constructorMax : max;
49-
if (count > limit) {
59+
var declaration = routine.getRoutineNameDeclaration();
60+
if (declaration == null || !declaration.isImplementationDeclaration()) {
61+
// Don't duplicate issues between the declaration and implementation
62+
return super.visit(routine, context);
63+
}
64+
65+
// A routine implementation's default parameters MUST match its declaration's, or not have any
66+
// default parameters at all. This means that we need to check the declaration to get the
67+
// authoritative number of default parameters.
68+
checkRoutine(
69+
routine.getRoutineNameNode(),
70+
declaration.getParameters().size(),
71+
routine.isConstructor(),
72+
context);
73+
return super.visit(routine, context);
74+
}
75+
76+
private void checkRoutine(
77+
RoutineNameNode node, int count, boolean isConstructor, DelphiCheckContext context) {
78+
var thisMax = isConstructor ? constructorMax : max;
79+
if (count > thisMax) {
5080
reportIssue(
5181
context,
52-
routine.getRoutineNameNode(),
82+
node,
5383
String.format(
5484
"%s has %d parameters, which is greater than %d authorized.",
55-
routine.isConstructor() ? "Constructor" : "Routine", count, limit));
85+
isConstructor ? "Constructor" : "Routine", count, thisMax));
5686
}
57-
return super.visit(routine, context);
5887
}
5988
}

‎delphi-checks/src/test/java/au/com/integradev/delphi/checks/TooManyParametersCheckTest.java‎

Lines changed: 142 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@
2424

2525
class TooManyParametersCheckTest {
2626
@Test
27-
void testOneVariableShouldNotAddIssue() {
27+
void testImplOkParamsShouldNotAddIssue() {
2828
CheckVerifier.newVerifier()
2929
.withCheck(new TooManyParametersCheck())
3030
.onFile(
@@ -37,7 +37,7 @@ void testOneVariableShouldNotAddIssue() {
3737
}
3838

3939
@Test
40-
void testTooManyVariablesShouldAddIssue() {
40+
void testImplOnlyTooManyParamsShouldAddIssue() {
4141
CheckVerifier.newVerifier()
4242
.withCheck(new TooManyParametersCheck())
4343
.onFile(
@@ -57,4 +57,144 @@ void testTooManyVariablesShouldAddIssue() {
5757
.appendImpl("end;"))
5858
.verifyIssues();
5959
}
60+
61+
@Test
62+
void testImplAndDeclTooManyParamsShouldAddIssue() {
63+
CheckVerifier.newVerifier()
64+
.withCheck(new TooManyParametersCheck())
65+
.onFile(
66+
new DelphiTestUnitBuilder()
67+
.appendDecl("procedure Foo( // Noncompliant")
68+
.appendDecl(" Param1: Boolean;")
69+
.appendDecl(" Param2: Boolean;")
70+
.appendDecl(" Param3: Boolean;")
71+
.appendDecl(" Param4: Boolean;")
72+
.appendDecl(" Param5: Boolean;")
73+
.appendDecl(" Param6: Boolean;")
74+
.appendDecl(" Param7: Boolean;")
75+
.appendDecl(" Param8: Boolean")
76+
.appendDecl(");")
77+
.appendImpl("procedure Foo(")
78+
.appendImpl(" Param1: Boolean;")
79+
.appendImpl(" Param2: Boolean;")
80+
.appendImpl(" Param3: Boolean;")
81+
.appendImpl(" Param4: Boolean;")
82+
.appendImpl(" Param5: Boolean;")
83+
.appendImpl(" Param6: Boolean;")
84+
.appendImpl(" Param7: Boolean;")
85+
.appendImpl(" Param8: Boolean")
86+
.appendImpl(");")
87+
.appendImpl("begin")
88+
.appendImpl(" // do nothing")
89+
.appendImpl("end;"))
90+
.verifyIssues();
91+
}
92+
93+
@Test
94+
void testImplAndDeclOkParamsShouldNotAddIssue() {
95+
CheckVerifier.newVerifier()
96+
.withCheck(new TooManyParametersCheck())
97+
.onFile(
98+
new DelphiTestUnitBuilder()
99+
.appendDecl("procedure Foo(")
100+
.appendDecl(" Param1: Boolean;")
101+
.appendDecl(" Param2: Boolean")
102+
.appendDecl(");")
103+
.appendImpl("procedure Foo(")
104+
.appendImpl(" Param1: Boolean;")
105+
.appendImpl(" Param2: Boolean")
106+
.appendImpl(");")
107+
.appendImpl("begin")
108+
.appendImpl(" // do nothing")
109+
.appendImpl("end;"))
110+
.verifyNoIssues();
111+
}
112+
113+
@Test
114+
void testDeclOnlyTooManyParamsShouldAddIssue() {
115+
CheckVerifier.newVerifier()
116+
.withCheck(new TooManyParametersCheck())
117+
.onFile(
118+
new DelphiTestUnitBuilder()
119+
.appendDecl("procedure Foo( // Noncompliant")
120+
.appendDecl(" Param1: Boolean;")
121+
.appendDecl(" Param2: Boolean;")
122+
.appendDecl(" Param3: Boolean;")
123+
.appendDecl(" Param4: Boolean;")
124+
.appendDecl(" Param5: Boolean;")
125+
.appendDecl(" Param6: Boolean;")
126+
.appendDecl(" Param7: Boolean;")
127+
.appendDecl(" Param8: Boolean")
128+
.appendDecl(");"))
129+
.verifyIssues();
130+
}
131+
132+
@Test
133+
void testInterfaceTooManyParamsShouldAddIssue() {
134+
CheckVerifier.newVerifier()
135+
.withCheck(new TooManyParametersCheck())
136+
.onFile(
137+
new DelphiTestUnitBuilder()
138+
.appendDecl("type IMyIntf = interface")
139+
.appendDecl(" procedure Foo( // Noncompliant")
140+
.appendDecl(" Param1: Boolean;")
141+
.appendDecl(" Param2: Boolean;")
142+
.appendDecl(" Param3: Boolean;")
143+
.appendDecl(" Param4: Boolean;")
144+
.appendDecl(" Param5: Boolean;")
145+
.appendDecl(" Param6: Boolean;")
146+
.appendDecl(" Param7: Boolean;")
147+
.appendDecl(" Param8: Boolean")
148+
.appendDecl(" );")
149+
.appendDecl("end;"))
150+
.verifyIssues();
151+
}
152+
153+
@Test
154+
void testConstructorTooManyParamsShouldAddIssue() {
155+
var check = new TooManyParametersCheck();
156+
check.max = 9999;
157+
158+
CheckVerifier.newVerifier()
159+
.withCheck(new TooManyParametersCheck())
160+
.onFile(
161+
new DelphiTestUnitBuilder()
162+
.appendDecl("type TMyObj = class(TObject)")
163+
.appendDecl(" constructor Create( // Noncompliant")
164+
.appendDecl(" Param1: Boolean;")
165+
.appendDecl(" Param2: Boolean;")
166+
.appendDecl(" Param3: Boolean;")
167+
.appendDecl(" Param4: Boolean;")
168+
.appendDecl(" Param5: Boolean;")
169+
.appendDecl(" Param6: Boolean;")
170+
.appendDecl(" Param7: Boolean;")
171+
.appendDecl(" Param8: Boolean")
172+
.appendDecl(" );")
173+
.appendDecl("end;"))
174+
.verifyIssues();
175+
}
176+
177+
@Test
178+
void testNonConstructorTooManyParamsShouldAddIssue() {
179+
var check = new TooManyParametersCheck();
180+
check.constructorMax = 9999;
181+
182+
CheckVerifier.newVerifier()
183+
.withCheck(new TooManyParametersCheck())
184+
.onFile(
185+
new DelphiTestUnitBuilder()
186+
.appendDecl("type TMyObj = class(TObject)")
187+
.appendDecl(" procedure Create( // Noncompliant")
188+
.appendDecl(" Param1: Boolean;")
189+
.appendDecl(" Param2: Boolean;")
190+
.appendDecl(" Param3: Boolean;")
191+
.appendDecl(" Param4: Boolean;")
192+
.appendDecl(" Param5: Boolean;")
193+
.appendDecl(" Param6: Boolean;")
194+
.appendDecl(" Param7: Boolean;")
195+
.appendDecl(" Param8: Boolean")
196+
.appendDecl(" );")
197+
.appendDecl("end;"))
198+
.verifyIssues();
199+
}
60200
}

0 commit comments

Comments
 (0)