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.

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

Added UnnecessaryConversionTemporary rule #279

Merged
merged 2 commits into from Aug 9, 2017
Merged

Added UnnecessaryConversionTemporary rule #279

merged 2 commits into from Aug 9, 2017

Conversation

schalkms
Copy link
Member

@schalkms schalkms commented Aug 6, 2017

No description provided.

private val types: Set<String> = hashSetOf("Boolean", "Byte", "Short", "Integer", "Long", "Float", "Double")

override fun visitCallExpression(expression: KtCallExpression) {
if (types.contains(expression.calleeExpression?.text) && expression.nextSibling?.nextSibling?.text == "toString()") {
Copy link
Member

Choose a reason for hiding this comment

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

Please extract both conditons to functions with a meaningful name.

override val issue: Issue = Issue("UnnecessaryConversionTemporary", Severity.Performance,
"Avoid temporary objects when converting primitive types to String")

private val types: Set<String> = hashSetOf("Boolean", "Byte", "Short", "Integer", "Long", "Float", "Double")
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 setOf()?

Copy link
Member

Choose a reason for hiding this comment

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

Because he uses types.contains later which is O(1). setOf is a LinkedSet O(n) ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought it was a HashSet by default, my bad 👍

import org.assertj.core.api.Assertions
import org.junit.jupiter.api.Test

class UnnecessaryConversionTemporaryTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the rest of the project used Spek?

import io.gitlab.arturbosch.detekt.api.Severity
import org.jetbrains.kotlin.psi.KtCallExpression

class UnnecessaryConversionTemporary(config: Config = Config.empty) : Rule(config) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What about UnnecessaryTemporaryInstantiation? "conversion temporary" doesn't seem very clear to me

@schalkms
Copy link
Member Author

schalkms commented Aug 7, 2017

Thanks for the code review.
Which test framework should be used for what exactly?

@arturbosch
Copy link
Member

Spek is the preferred. Junit5 is still used because spek had some issues before 1.0 where I started using it

@schalkms
Copy link
Member Author

schalkms commented Aug 8, 2017

@arturbosch ok
Thanks for clarifying. Review notes were taken into account.

Copy link
Contributor

@rock3r rock3r left a comment

Choose a reason for hiding this comment

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

Nice one! Like this

@arturbosch
Copy link
Member

Looks good, will be added to RC2 soon 👍

@arturbosch arturbosch merged commit 2526975 into detekt:master Aug 9, 2017
@arturbosch arturbosch added this to the RC2 milestone Aug 9, 2017
@schalkms schalkms deleted the unnecessary-conversion-tmp branch September 20, 2017 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants