Added validation of constructors to LongParameterList #2410

r4zzz4k · 2020-03-05T15:49:16Z

I've looked through existing rules, seems that nothing covers number of parameters in contructors. LongParameterList is the nearest thing, but it covers only methods.

Does this sound like a useful rule? In discussions over the Internet on argument count limit I mostly see costructors included. Checkstyle, for example, covers both methods and parameters, PMD seems to agree on that.

After adding this plugin ./gradlew detekt starts failing on the project, as some classes (e.g. CorrectableCodeSmell, see below) have 6 or more parameters in constructors.

https://github.com/arturbosch/detekt/blob/521e96856d841ec8c115a12f2db8230ce01eda86/detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/CodeSmell.kt#L44-L51

I'll try to collect proper report, as right now I'm having troubles with it (#2411).

schalkms

Thanks for contributing to detekt. This is a good idea.
I'd put this in a separate rule, since users may want to have different configuration settings for the max parameter number.

r4zzz4k · 2020-03-05T18:40:25Z

Sure, will do. Mar 5, 2020 20:37:07 M Schalk <notifications@github.com>:

…

@schalkms commented on this pull request. Thanks for contributing to detekt. I'd put this in a separate rule, since users may want to have different configuration settings for the max parameter number. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub [#2410] , or unsubscribe [https://github.com/notifications/unsubscribe-auth/ABC6WKTVZCPW5FT2VHBPRATRF7WM3ANCNFSM4LCM2BRA] . [https://github.com/notifications/beacon/ABC6WKUKRNVEJOFI7AT7MFDRF7WM3A5CNFSM4LCM2BRKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCYFPEFY.gif]

r4zzz4k · 2020-03-06T13:56:13Z

I'm playing with it for some time and don't like both abstract parent for two rules (pushed above) and code duplication without it.

Does adding new configuration parameter like constructorThreshold to existing rule sound viable? With default value like -1 or 255 meaning "don't validate constructors".

I'm open to any other suggestions, of course.

schalkms · 2020-03-06T20:52:27Z

For me this PR looks good. Please fix the issues detected issues and regenerate the documentation for detekt's homepage. Then we'll proceed with merging this PR.

...src/main/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/LongConstructorParameterList.kt

arturbosch · 2020-03-09T11:15:23Z

Both approaches have advantages:

own rule:

clear separation of concerns
does not interfere with current config properties of LongParameterList

extending LongParameterList

pmd and checkstyle have just one rule for both cases
co-location of properties (constructorThreshold, functionThreshold, threshold)
two similar rule names: LongParameterList and LongConstructorParameterList; I would prefer deprecating LongParameterList then and build extra logic for a LongFunctionParameterList.

I'm OK with both approaches but would prefer a property constructorThreshold for LPL with a default -1 value.

BraisGabin · 2020-03-09T23:02:10Z

Why the -1 as default? It doesn't seem like a good default value. If the reason is to avoid "BC" I consider this a bug fix more than a new feature.

If we go to LongParameterList we should deprecate threshold and add constructorThreshold and functionThreshold. I did this before to have a reference: #2132

r4zzz4k · 2020-03-10T12:44:32Z

Thanks for the reference! Proper deprecation and splitting property into two seems much nicer than -1.
I'll try that one out, as I'm also leaning to single rule instead of two different, but very similar ones.

Hoping to get back to that in the nearest future.

schalkms · 2020-03-10T17:15:48Z

I don’t think it’s wise to use -1 as a default value.

r4zzz4k · 2020-03-22T14:27:29Z

I've went with splitting configuration option in two as this was the most discussed option here. I've also added option to ignore data classes as these value bags are a kinda separate thing in my opinion. So one may want to both validate and not validate them to their liking.

I guess CONTRIBUTING.md should note that update of both both documentation and default-detekt-config.yml can be done via generateDocumentation Gradle task as this is not quite obvious. But of course build covers that, so I'm not sure if this is actually important for contributors.

Looking forward to your opinions on the PR.

schalkms

Thanks! That PR looks awesome!
I have two found one minor improvement regarding the documentation.

Concerning the Contributing guide, if you find something missing we would be really grateful for a PR that improves the docs. For experienced contributors it's hard to always know which documentation 1st time contributors need.

schalkms · 2020-03-22T16:36:47Z

detekt-rules/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/LongParameterList.kt

 import org.jetbrains.kotlin.psi.KtNamedFunction
 import org.jetbrains.kotlin.psi.KtParameterList
+import org.jetbrains.kotlin.psi.KtPrimaryConstructor
+import org.jetbrains.kotlin.psi.KtSecondaryConstructor

 /**
 * Reports functions which have more parameters than a certain threshold (default: 6).


Suggested change

* Reports functions which have more parameters than a certain threshold (default: 6).

* Reports functions and constructors which have more parameters than a certain threshold.

schalkms · 2020-03-22T16:41:14Z

detekt-rules/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/LongParameterList.kt

+        validateConstructor(constructor)
+    }
+
+    private fun <T : KtConstructor<T>> validateConstructor(constructor: KtConstructor<T>) {


I'm sorry, I don't understand why this function needs generics.
Does the following function declaration also suffice?

Suggested change

private fun <T : KtConstructor<T>> validateConstructor(constructor: KtConstructor<T>) {

private fun validateConstructor(constructor: KtFunction) {

I've just taken least common denominator of two types, which happens to have generic parameter. More generic KtFunction should work, will check.

.getContainingClassOrObject() is a method of KtConstructor, so it's not available on KtFunction, but if star-projected type is okay, that's more than enough for this.

...-rules/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/LongParameterListSpec.kt

schalkms · 2020-03-22T16:51:42Z

...-rules/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/LongParameterListSpec.kt

+        }
+
+        it("does not report short parameter list for secondary constructors") {
+            val code = "class LongCtor() { constructor(a: Int, b: Int, c: Int, d: Int, e: Int, f: Int) }"


...-rules/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/LongParameterListSpec.kt

schalkms · 2020-03-22T19:31:33Z

@r4zzz4k your implemented rule already found a code smell in detekt itself 😉
The rest builds fine. Thank you. Good job! 💪

Ruleset: complexity - 20min debt
4443	LongParameterList - 7/7 - [DslGradleRunner] at /home/travis/build/arturbosch/detekt/detekt-gradle-plugin/src/test/kotlin/io/gitlab/arturbosch/detekt/DslGradleRunner.kt:8:22

r4zzz4k · 2020-03-22T19:38:14Z

I'm not hundred percent sure about defaults, so it may need some tuning in future. Hoping we would be able to gather some valuable feedback on this in future.

Anyway, great that my contribution is useful to someone besides me. Thank you :) And thanks for your hard work on the project, it really changes the way one works on their codebase in a very positive way!

arturbosch · 2020-03-22T20:14:17Z

Please suppress the issue for now so we can move merging this awesome PR :)

* Validated parameter count of primary and secondary constructors * Added flag to ignore data classes as parameter count of these arguably doesn't highlight violation of SRP * Deprecated threshold in favor of functionThreshold and constructorThreshold * Suppressed found issue in existing code

codecov-io · 2020-03-22T21:26:26Z

Codecov Report

Merging #2410 into master will decrease coverage by 0.00%.
The diff coverage is 85.71%.

@@             Coverage Diff              @@
##             master    #2410      +/-   ##
============================================
- Coverage     83.26%   83.26%   -0.01%     
- Complexity     2198     2204       +6     
============================================
  Files           360      360              
  Lines          6240     6250      +10     
  Branches       1144     1146       +2     
============================================
+ Hits           5196     5204       +8     
  Misses          466      466              
- Partials        578      580       +2

Impacted Files	Coverage Δ	Complexity Δ
...bosch/detekt/rules/complexity/LongParameterList.kt	`83.33% <84.61%> (-2.39%)`	`15.00 <7.00> (+6.00)`	⬇️
...ch/detekt/api/internal/ValidatableConfiguration.kt	`100.00% <100.00%> (ø)`	`0.00 <0.00> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fbfcabf...e9f90f7. Read the comment docs.

r4zzz4k · 2020-03-22T21:33:40Z

Should I roll back rebasing or this is okay?

schalkms · 2020-03-22T21:49:55Z

@r4zzz4k this is okay. The failing AppVeyor build is not related to your change. Everything's fine.

r4zzz4k mentioned this pull request Mar 5, 2020

RuleSetProvider: Error reading configuration file #2411

Closed

schalkms reviewed Mar 5, 2020

View reviewed changes

r4zzz4k force-pushed the long-param-list-ctors branch from 96183ff to 28726f9 Compare March 6, 2020 13:54

schalkms reviewed Mar 6, 2020

View reviewed changes

...src/main/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/LongConstructorParameterList.kt Outdated Show resolved Hide resolved

r4zzz4k force-pushed the long-param-list-ctors branch from 28726f9 to 9c69ab9 Compare March 22, 2020 14:21

r4zzz4k requested a review from schalkms March 22, 2020 14:21

schalkms approved these changes Mar 22, 2020

View reviewed changes

schalkms reviewed Mar 22, 2020

View reviewed changes

...-rules/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/LongParameterListSpec.kt Outdated Show resolved Hide resolved

schalkms reviewed Mar 22, 2020

View reviewed changes

...-rules/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/LongParameterListSpec.kt Outdated Show resolved Hide resolved

arturbosch added this to the 1.8.0 milestone Mar 22, 2020

r4zzz4k force-pushed the long-param-list-ctors branch from 1f296f3 to e9f90f7 Compare March 22, 2020 20:56

arturbosch merged commit 184eaa8 into detekt:master Mar 23, 2020

0xnm mentioned this pull request Sep 17, 2021

RUMM-1621: Remove jcenter repo references DataDog/dd-sdk-android#704

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added validation of constructors to LongParameterList #2410

Added validation of constructors to LongParameterList #2410

r4zzz4k commented Mar 5, 2020 •

edited

Loading

schalkms left a comment •

edited

Loading

r4zzz4k commented Mar 5, 2020 via email

r4zzz4k commented Mar 6, 2020

schalkms commented Mar 6, 2020 •

edited

Loading

arturbosch commented Mar 9, 2020

BraisGabin commented Mar 9, 2020

r4zzz4k commented Mar 10, 2020

schalkms commented Mar 10, 2020 •

edited

Loading

r4zzz4k commented Mar 22, 2020

schalkms left a comment

schalkms Mar 22, 2020

schalkms Mar 22, 2020

r4zzz4k Mar 22, 2020

r4zzz4k Mar 22, 2020

schalkms Mar 22, 2020

schalkms commented Mar 22, 2020

r4zzz4k commented Mar 22, 2020

arturbosch commented Mar 22, 2020

codecov-io commented Mar 22, 2020 •

edited

Loading

r4zzz4k commented Mar 22, 2020

schalkms commented Mar 22, 2020

	* Reports functions which have more parameters than a certain threshold (default: 6).
	* Reports functions and constructors which have more parameters than a certain threshold.

	private fun <T : KtConstructor<T>> validateConstructor(constructor: KtConstructor<T>) {
	private fun validateConstructor(constructor: KtFunction) {

Added validation of constructors to LongParameterList #2410

Added validation of constructors to LongParameterList #2410

Conversation

r4zzz4k commented Mar 5, 2020 • edited Loading

schalkms left a comment • edited Loading

Choose a reason for hiding this comment

r4zzz4k commented Mar 5, 2020 via email

r4zzz4k commented Mar 6, 2020

schalkms commented Mar 6, 2020 • edited Loading

arturbosch commented Mar 9, 2020

BraisGabin commented Mar 9, 2020

r4zzz4k commented Mar 10, 2020

schalkms commented Mar 10, 2020 • edited Loading

r4zzz4k commented Mar 22, 2020

schalkms left a comment

Choose a reason for hiding this comment

schalkms Mar 22, 2020

Choose a reason for hiding this comment

schalkms Mar 22, 2020

Choose a reason for hiding this comment

r4zzz4k Mar 22, 2020

Choose a reason for hiding this comment

r4zzz4k Mar 22, 2020

Choose a reason for hiding this comment

schalkms Mar 22, 2020

Choose a reason for hiding this comment

schalkms commented Mar 22, 2020

r4zzz4k commented Mar 22, 2020

arturbosch commented Mar 22, 2020

codecov-io commented Mar 22, 2020 • edited Loading

Codecov Report

r4zzz4k commented Mar 22, 2020

schalkms commented Mar 22, 2020

r4zzz4k commented Mar 5, 2020 •

edited

Loading

schalkms left a comment •

edited

Loading

schalkms commented Mar 6, 2020 •

edited

Loading

schalkms commented Mar 10, 2020 •

edited

Loading

codecov-io commented Mar 22, 2020 •

edited

Loading