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

Complexity rules: ComplexInterface + MethodOverloading #440

Merged
merged 4 commits into from
Sep 30, 2017
Merged

Complexity rules: ComplexInterface + MethodOverloading #440

merged 4 commits into from
Sep 30, 2017

Conversation

schalkms
Copy link
Member

Should we also count static members to the LargeInterface rule?

@@ -168,9 +168,15 @@ complexity:
LargeClass:
active: true
threshold: 150
LargeInterface:
active: false
threshold: 10
Copy link
Member

Choose a reason for hiding this comment

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

I do not know if we should call it so. LargeClass threshold measurement is sloc and for LargeInterface it would be #members. ComplexInterface? What if we rename the threshold property?

Copy link
Member Author

Choose a reason for hiding this comment

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

Threshold is ok.
ComplexInterface is ok.

override fun visitClass(klass: KtClass) {
if (klass.isInterface()) {
val body = klass.getBody() ?: return
val size = body.children.count { it is KtNamedFunction || it is KtProperty }
Copy link
Member

Choose a reason for hiding this comment

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

I suspect a local variable is also a KtProperty. Can you add a test case for this please?

Copy link
Member Author

Choose a reason for hiding this comment

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

I will.

override val issue = Issue("MethodOverloading", Severity.Maintainability,
"Methods which are overloaded often might be harder to maintain. " +
"Furthermore, these methods are tightly coupled. " +
"Refactor these methods and try to use optional parameters.")
Copy link
Member

Choose a reason for hiding this comment

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

This might be argueable. Optional parameters tend to break the single responsibility principle (Uncle Bob).... Ah ok I was thinking of Java. I think Kotlin calls them default parameters ^^

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll rename it.

@schalkms
Copy link
Member Author

@arturbosch What do you think about that?

Should we also count static members to the LargeInterface rule?

@arturbosch
Copy link
Member

Thanks for the changes!
Adding a property to count static members is a good idea. I would not count them on default because often they are constants ^^

@schalkms
Copy link
Member Author

I know.
I might add that in the future.

@arturbosch arturbosch changed the title Complexity rules Complexity rules: ComplexInterface + ethodOverloading Sep 30, 2017
@arturbosch arturbosch changed the title Complexity rules: ComplexInterface + ethodOverloading Complexity rules: ComplexInterface + MethodOverloading Sep 30, 2017
@arturbosch arturbosch added this to the RC5 milestone Sep 30, 2017
@arturbosch
Copy link
Member

KK, than I merge this right now ^^

@arturbosch arturbosch merged commit 67eb41a into detekt:master Sep 30, 2017
@schalkms schalkms deleted the complexity-rules branch October 5, 2017 17:17
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

2 participants