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

Mistake From LongParameterList.kt #2202

Closed
mmandril opened this issue Dec 27, 2019 · 3 comments · Fixed by #2204
Closed

Mistake From LongParameterList.kt #2202

mmandril opened this issue Dec 27, 2019 · 3 comments · Fixed by #2204

Comments

@mmandril
Copy link

Hi there.
If my DEFAULT_ACCEPTED_PARAMETER_LENGTH is 6, and this, for me, means that 6 is the max parameter length that a method must have at total, why detekt trigger if i have a method that receives 6 parameters ? This have no sense for me.

For me, this if (parameters != null && parameters >= threshold) must be replace for this if (parameters != null && parameters > threshold)

If anyone could explain to me. Thanks.

@cortinico
Copy link
Member

Hi @mmandril

I assume the variable is wrongly named. DEFAULT_ACCEPTED_PARAMETER_LENGTH should probably be renamed to THRESHOLD_PARAMETER_LENGTH.

The current behavior of the rule you're referring to (LongParameterList) is consistent with all the other rules. Detekt reports when threshold is met (>= or <=).

This is also consistent with the documentation of the LongParameterList rule:


/**
 * Reports functions which have more parameters than a certain threshold (default: 6).
 *
 * @configuration threshold - number of parameters required to trigger the rule (default: `6`)
...

@mmandril
Copy link
Author

I imagined that. I think the same problem is in:

  • ComplexMethod : DEFAULT_ACCEPTED_METHOD_COMPLEXITY = 15
  • NestedBlockDepth: DEFAULT_ACCEPTED_NESTING = 4

This classes were some that i could see that the attributes names makes no sense. I think changing the names for one that could explain better what it means(you gave a better name for example) will help.

Thanks for the reply.

@cortinico
Copy link
Member

Agree @mmandril, the variable name is confusing over there. I'm fixing it here: #2204

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.

4 participants