Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added StringLiteralDuplication rule #300

Merged
merged 5 commits into from Aug 19, 2017
Merged

Added StringLiteralDuplication rule #300

merged 5 commits into from Aug 19, 2017

Conversation

schalkms
Copy link
Member

No description provided.

vanniktech
vanniktech approved these changes Aug 13, 2017
View reviewed changes
Copy link
Contributor

@vanniktech vanniktech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also add a regex property to have the possibility to ignore this check on all test files.

...les/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/StringLiteralDuplication.kt Outdated
increment()
}

private fun increment() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not inline this function?

...les/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/StringLiteralDuplication.kt Outdated
}
}

private const val DEFAULT_DUPLICATION = 2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find 2 quite low as a default.

@schalkms
Copy link
Member Author

schalkms commented Aug 13, 2017 •
edited

@vanniktech thanks for giving feedback

The default value is of course discussable.
I think the string duplication rule should also exclude whitespaces and common strings like

  • "."
  • ","

I am waiting for feedback. I am also willing to fix the following duplicates after giving feedback.

StringLiteralDuplication - 5/2 - [CodeSmell.kt] at detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/CodeSmell.kt:1:1
	
StringLiteralDuplication - 3/2 - [Location.kt] at detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/Location.kt:1:1
	
StringLiteralDuplication - 3/2 - [Signatures.kt] at detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/Signatures.kt:1:1
	
StringLiteralDuplication - 3/2 - [SpacingAfterKeyword.kt] at detekt-formatting/src/main/kotlin/io/gitlab/arturbosch/detekt/formatting/SpacingAfterKeyword.kt:1:1
	
StringLiteralDuplication - 4/2 - [SpacingAroundBraces.kt] at detekt-formatting/src/main/kotlin/io/gitlab/arturbosch/detekt/formatting/SpacingAroundBraces.kt:1:1
	
StringLiteralDuplication - 7/2 - [DetektPlugin.kt] at detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/DetektPlugin.kt:1:1
	
StringLiteralDuplication - 3/2 - [NamingConventionViolation.kt] at detekt-rules/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/NamingConventionViolation.kt:1:1
	
StringLiteralDuplication - 13/2 - [NamingConventionViolation.kt] at detekt-rules/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/NamingConventionViolation.kt:1:1

@vanniktech
Copy link
Contributor

How about passing a list of values that should be ignored and by default default to . and ,

@arturbosch
Copy link
Member

arturbosch commented Aug 14, 2017 •
edited

An ignore list sounds good to me.
I like the rule! Maybe a name like ConsiderConstantString would suit it better?
Also add active: false to the config, thanks!

arturbosch
arturbosch reviewed Aug 14, 2017
View reviewed changes
detekt-rules/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/providers/ComplexityProvider.kt Outdated
@@ -25,6 +26,7 @@ class ComplexityProvider : RuleSetProvider {
LongMethod(config),
LargeClass(config),
ComplexMethod(config),
ConsiderConstantString(config),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry my bad. ConsiderStringConstant is a better name.

Why ComplexityProvider? I think style suits it better?

Copy link
Member Author

@schalkms schalkms Aug 14, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree. It is basically code duplication. I think multiple occurrences of hard coded strings are making maintenance more difficult and thus the kt file more complex.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm now I'm thinking about that the name StringLiteralDuplication was good though :/ and the description should suggest using a constant.

@arturbosch
Copy link
Member

I like the configuration options!

I will wait some more days so we can figure out the threshold value. 2 seems to be too low, but also reasonable ...

@arturbosch
Copy link
Member

i think some of the StringLiteralDuplication findings you posted above are false positive. I do not see any duplicated strings in CodeSmell, Location, NamingConvention?

@schalkms
Copy link
Member Author

schalkms commented Aug 16, 2017 •
edited

@arturbosch maybe it's an outdated version now? I'll check it.

@schalkms
Copy link
Member Author

I also did some research concerning this rule. SonarJava also this rule. They have a threshold of 3. However, this threshold comparing it to detekts means setting the threshold for this rule to 2.
IMHO the threshold for detekt rules is confusing and thus should be refactored.

Furthermore, SonarJava also doesn't detect strings with less than 5 characters.

reference:
https://www.sonarsource.com/products/codeanalyzers/sonarjava/rules.html#RSPEC-1192

What do you think? @arturbosch @vanniktech @Mauin

vanniktech
vanniktech reviewed Aug 16, 2017
View reviewed changes
Copy link
Contributor

@vanniktech vanniktech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think PMD has a default threshold of 5. I still think that 2 is a bit low. Probably we should settle for something between 3 and 5.

@arturbosch
Copy link
Member

arturbosch commented Aug 17, 2017 •
edited

I think we should follow the sonarqube way and exclude strings with size < 5 and have a threshold of 3 (2 in detekt).

Seems like that sonar uses value >= threshold. Detekt (well I introduced it) uses value > threshold. So 2 seems low, effectlively it is 3. I will open a issue to discuss the which threshold definition we should use.

Edit: I think it is now ok to merge this and change the threshold if needed in #313. Sorry for that but @schalkms can you change the name of the rule back to StringLiteralDuplication :/ ? Thanks!

Edit2: Oh the option for strings smaller than 5 would be nice!

@arturbosch arturbosch mentioned this pull request Aug 17, 2017
Closed
@arturbosch arturbosch added this to the next milestone Aug 17, 2017
@arturbosch arturbosch merged commit 648751d into detekt:master Aug 19, 2017
@arturbosch
Copy link
Member

👍

@schalkms schalkms deleted the string-literal-dup branch September 20, 2017 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vanniktech vanniktech vanniktech approved these changes

@arturbosch arturbosch arturbosch left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
RC3
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@schalkms @vanniktech @arturbosch