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

LongMethod should not consider parameters while calculating the number of lines #2804

Closed
niraj8 opened this issue Jun 16, 2020 · 5 comments · Fixed by #2806
Closed

LongMethod should not consider parameters while calculating the number of lines #2804

niraj8 opened this issue Jun 16, 2020 · 5 comments · Fixed by #2806
Assignees
Milestone

Comments

@niraj8
Copy link
Contributor

niraj8 commented Jun 16, 2020

Expected Behavior

LongMethod should consider the method definition without the input parameters.
Example:
If LongMethod threshold is set to 7, then the below method shouldn't be tagged as the method implementation is actually 3 loc and detekt already has LongParameterList to check the param list

fun fetch(
        user: User,
        org: Org,
        @QueryValue startDate: String,
        @QueryValue endDate: String
    ) {
        val data = service1.get(startDate, endDate, context.user, context.org)
        val moreData = service2.get(startDate)
        ok(mapOf("data" to data+moreData))
    }

Observed Behavior

The above snippet of code gets tagged with LongMethod code smell

Steps to Reproduce

#2806

Context

More LongMethod code smell then expected

Your Environment

  • Version of detekt used:1.9.1
  • Version of Gradle used (if applicable):6.1.1
  • Operating System and version:macOS 10.15.5
  • Link to your project (if it's a public repository):
@cortinico
Copy link
Member

Agree, this sounds like a false positive. Would you be able to work on a fix and make the tests in #2806 green (thank you for doing so btw)? We can support you in that if needed 👍

@niraj8
Copy link
Contributor Author

niraj8 commented Jun 16, 2020

Yes, I am working on it. Will make some noise if I get stuck.

@cortinico
Copy link
Member

Assigning the issue to you so that we don't step on your toes 👍

@arturbosch arturbosch added this to the 1.10.0 milestone Jun 16, 2020
@niraj8
Copy link
Contributor Author

niraj8 commented Jun 19, 2020

I wanted to get opinions from others before making any further changes.

For a top-level method, should the lines used by a nested method parameter list be considered in calculating the method length.

In the example below, the top-level body has 4 lines if we ignore the nested method.

fun longMethod(
    param1: String
) { // 4 lines
    println()
    println()

    fun nestedLongMethod(
        param1: String
    ) { // 5 lines
        println()
        println()
        println()
    }
}

@arturbosch
Copy link
Member

It is a good practice to decompose a large function into smaller ones. In kotlin we have local functions. So we should exclude them completely (whole signature).

I've answered in #2806; so basically #loc of longMethod.body - #loc of whole nestedLongMethod

schalkms pushed a commit that referenced this issue Jun 22, 2020
* failing tests for #2804 LongMethod with params

* fix test

* skipTrees for KtParameterList for linesOfCode

* use test snippets that compile

* [ci ckip] use fun body for LongMethod
- revert changes in LinesOfCode.kt
- still need to update nested methods

* remove unused import

* add functionToBodyLinesCache to track fun body length

* remove spaces from test asserts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants