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

Rule for method nesting #4780

Closed
WildOrangutan opened this issue Apr 26, 2022 · 6 comments · Fixed by #4788
Closed

Rule for method nesting #4780

WildOrangutan opened this issue Apr 26, 2022 · 6 comments · Fixed by #4788
Assignees

Comments

@WildOrangutan
Copy link
Contributor

Hey. I'm wondering if it would be possible to add a rule, to avoid nesting certain methods/functions.

For example:

A().apply {
    B().apply {
        C().apply {
            foo()
        }
    }
}

This kind of practice is often confusing, since you don't know where foo() belongs.

I've considered using ComplexMethod, but it seems like you need to pass certain threshold, in order for detekt to fail. I would like to fail instantly in my example.

@BraisGabin
Copy link
Member

Extract from: https://kotlinlang.org/docs/scope-functions.html#function-selection

Avoid nesting scope functions and be careful when chaining them: it's easy to get confused about the current context object and the value of this or it.

I agree that we should have a rule that flags if any of the scoped functions are scoped.

@schalkms
Copy link
Member

I really like this idea for a new rule. Maybe, the rule should be configurable with a threshold. But, I wouldn't see this as strictly mandatory for a first inclusion in detekt.

@WildOrangutan may I ask you to take this issue and submit a first PR for this new rule?

@WildOrangutan
Copy link
Contributor Author

I will give it a go, but won't promise anything :)

I'm thinking of a rule like below. Let me know if that sounds ok.

complexity:
  NestingFunctions:
    active: true
    threshold: 1
    functions:
      - 'apply'
      - 'run'
      - 'with'

@BraisGabin
Copy link
Member

Sounds great. I would vote to add let to the list too.

@marschwar
Copy link
Contributor

How is that different than NestedBlockDepth? In theory that rule should pick up the same issues (and more). There is no test that verifies this behavior, but the code looks like it is intended to do so.

// ...
override fun visitCallExpression(expression: KtCallExpression) {
    val lambdaArguments = expression.lambdaArguments
    if (expression.isUsedForNesting()) {
        insideLambdaDo(lambdaArguments) { inc() }
        super.visitCallExpression(expression)
        insideLambdaDo(lambdaArguments) { dec() }
    }
}

fun KtCallExpression.isUsedForNesting(): Boolean = when (getCallNameExpression()?.text) {
    "run", "let", "apply", "with", "use", "forEach" -> true
    else -> false
}

@BraisGabin
Copy link
Member

The problem of that rule is that if you set the threshold to 2 and you nes two ifs it will yield. And that's not the point of this rule. I don't care about nesting ifs or fors. I care about messing scoped functions. But we could find a way to change the behavior.

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