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.

Sign up for GitHub

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

Jump to bottom

CognitiveComplexity: count else/else-if as one complexity #5458

Merged
merged 3 commits into from Oct 24, 2022
Merged

CognitiveComplexity: count else/else-if as one complexity #5458

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c1f4912
CognitiveComplexity: count else/else-if as one complexity
t-kameyama Oct 21, 2022
693fa8a
Don't increment nesting in else/else-if
t-kameyama Oct 22, 2022
6b06079
Add +1 complexity and increase nesting level
t-kameyama Oct 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 11 additions & 0 deletions detekt-metrics/src/main/kotlin/io/github/detekt/metrics/CognitiveComplexity.kt
View file
Open in desktop
@@ -1,6 +1,7 @@
package io.github.detekt.metrics

import io.gitlab.arturbosch.detekt.api.DetektVisitor
import io.gitlab.arturbosch.detekt.rules.isElseIf
import org.jetbrains.kotlin.com.intellij.openapi.util.Key
import org.jetbrains.kotlin.com.intellij.psi.tree.IElementType
import org.jetbrains.kotlin.lexer.KtTokens
Expand Down Expand Up @@ -89,8 +90,18 @@ class CognitiveComplexity private constructor() : DetektVisitor() {
}

override fun visitIfExpression(expression: KtIfExpression) {
val isElseIf = expression.isElseIf()

if (isElseIf) nesting--

addComplexity()
val elseBranch = expression.`else`
if (elseBranch != null && elseBranch !is KtIfExpression) {
addComplexity()
}
nestAround { super.visitIfExpression(expression) }

if (isElseIf) nesting++
}

override fun visitBreakExpression(expression: KtBreakExpression) {
Expand Down
100 changes: 97 additions & 3 deletions detekt-metrics/src/test/kotlin/io/github/detekt/metrics/CognitiveComplexitySpec.kt
View file
Open in desktop
Expand Up @@ -60,7 +60,7 @@ class CognitiveComplexitySpec {
""".trimIndent()
)

assertThat(CognitiveComplexity.calculate(code)).isEqualTo(2)
assertThat(CognitiveComplexity.calculate(code)).isEqualTo(3)
}

@Test
Expand All @@ -72,7 +72,7 @@ class CognitiveComplexitySpec {
""".trimIndent()
)

assertThat(CognitiveComplexity.calculate(code)).isEqualTo(2)
assertThat(CognitiveComplexity.calculate(code)).isEqualTo(3)
}

@Test
Expand All @@ -85,7 +85,7 @@ class CognitiveComplexitySpec {
""".trimIndent()
)

assertThat(CognitiveComplexity.calculate(code)).isEqualTo(1)
assertThat(CognitiveComplexity.calculate(code)).isEqualTo(2)
}
}

Expand Down Expand Up @@ -282,4 +282,98 @@ class CognitiveComplexitySpec {
}
}
}

@Nested
inner class `if-else expressions` {
@Test
fun `should count else as complexity`() {
val code = compileContentForTest(
"""
fun test(condition: Boolean) {
if (condition) { // +1
} else { // +1
}
}
""".trimIndent()
)
assertThat(CognitiveComplexity.calculate(code)).isEqualTo(2)
}

@Test
fun `should count else-if as 1 complexity`() {
val code = compileContentForTest(
"""
fun test(condition: Boolean) {
if (condition) { // +1
} else if (condition) { // +1
}
}
""".trimIndent()
)
assertThat(CognitiveComplexity.calculate(code)).isEqualTo(2)
}

@Test
fun `should count else-if and else correctly`() {
val code = compileContentForTest(
"""
fun test(condition: Boolean) {
if (condition) { // +1
} else if (condition) { // +1
} else if (condition) { // +1
} else { // + 1
}
}
""".trimIndent()
)
assertThat(CognitiveComplexity.calculate(code)).isEqualTo(4)
}

@Test
fun `should count nested else-if correctly`() {
val code = compileContentForTest(
"""
fun test(condition: Boolean) {
if (condition) { // +1
if (condition) { // +2
while(true) { // +3
}
} else if (condition) { // +2
while(true) { // +3
}
} else { // +2
while(true) { // +3
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

From #5447 and paper of Cognitive Complexity,

No nesting increment is assessed for these structures because the mental cost has already been paid when reading the if.

else and else if seem to be counted as +1 complexity only (do not include nesting level increments).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

693fa8a

Copy link
Contributor

@sanggggg sanggggg Oct 22, 2022 •
edited

Choose a reason for hiding this comment

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

I'm sorry @t-kameyama, I said something that could be misunderstood.
What I'm trying to say is

else if and else

  • should add +1 complexity (Increments)
  • should increase nesting level (Nesting Level)
  • should not add complexity by nesting level (no Nesting increments)

Screen Shot 2022-10-23 at 1 02 10 AM

Thus, the complexity of the test code should be 39

                fun test(condition: Boolean) {
                    if (condition) { // +1
                        if (condition) { // +2
                            while(true) { // +3
                            }
                        } else if (condition) { // +1
                            while(true) { // +3
                            }
                        } else if (condition) { // +1
                            while(true) { // +3
                            }
                        } else { // +1
                            while(true) { // +3
                            }
                        }
                    } else if (condition) { // +1
                        if (condition) { // +2
                            while(true) { // +3
                            }
                        } else if (condition) // +1
                            while(true) { // +3
                            }
                    } else { // + 1
                        if (condition) { // +2
                            while(true) { // +3
                            }
                        } else // +1
                            while(true) { // +3
                            }
                    }
                    if (condition) { // +1
                    }
                }
                """
                // total 39 comple

The complexity suddenly increased, but I think this is a more real complexity (from the perspective of Cognitive Complexity)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

6b06079

} else if (condition) { // +1
if (condition) { // +2
while(true) { // +3
}
} else if (condition) { // +2
while(true) { // +3
}
} else { // +2
while(true) { // +3
}
}
} else { // + 1
if (condition) { // +2
while(true) { // +3
}
} else if (condition) { // +2
while(true) { // +3
}
} else { // +2
while(true) { // +3
}
}
}
if (condition) { // +1
}
}
""".trimIndent()
)
assertThat(CognitiveComplexity.calculate(code)).isEqualTo(49)
}
}
}
2 changes: 1 addition & 1 deletion ...s/src/test/kotlin/io/github/detekt/metrics/processors/CognitiveComplexityProcessorSpec.kt
View file
Open in desktop
Expand Up @@ -14,6 +14,6 @@ class CognitiveComplexityProcessorSpec {
val value = MetricProcessorTester(file)
.test(ProjectCognitiveComplexityProcessor(), CognitiveComplexity.KEY)

assertThat(value).isEqualTo(46)
assertThat(value).isEqualTo(60)
}
}
8 changes: 4 additions & 4 deletions detekt-metrics/src/test/kotlin/io/github/detekt/metrics/processors/package.kt
View file
Open in desktop
Expand Up @@ -86,7 +86,7 @@ class ComplexClass {// McCabe: 44, LLOC: 20 + 20 + 4x4
while (true) {
if (false) {
println()
} else {
} else { // 4
println()
}
}
Expand All @@ -98,7 +98,7 @@ class ComplexClass {// McCabe: 44, LLOC: 20 + 20 + 4x4
while (true) {
if (false) {
println()
} else {
} else { // 3
println()
}
}
Expand Down Expand Up @@ -131,7 +131,7 @@ class ComplexClass {// McCabe: 44, LLOC: 20 + 20 + 4x4
while (true) {
if (false) {
println()
} else {
} else { // 4
println()
}
}
Expand All @@ -143,7 +143,7 @@ class ComplexClass {// McCabe: 44, LLOC: 20 + 20 + 4x4
while (true) {
if (false) {
println()
} else {
} else { // 3
println()
}
}
Expand Down
4 changes: 4 additions & 0 deletions detekt-psi-utils/api/detekt-psi-utils.api
View file
Open in desktop
Expand Up @@ -87,6 +87,10 @@ public final class io/gitlab/arturbosch/detekt/rules/KtCallExpressionKt {
public static final fun isCallingWithNonNullCheckArgument (Lorg/jetbrains/kotlin/psi/KtCallExpression;Lorg/jetbrains/kotlin/name/FqName;Lorg/jetbrains/kotlin/resolve/BindingContext;)Z
}

public final class io/gitlab/arturbosch/detekt/rules/KtIfExpressionKt {
public static final fun isElseIf (Lorg/jetbrains/kotlin/psi/KtIfExpression;)Z
}

public final class io/gitlab/arturbosch/detekt/rules/KtLambdaExpressionKt {
public static final fun firstParameter (Lorg/jetbrains/kotlin/psi/KtLambdaExpression;Lorg/jetbrains/kotlin/resolve/BindingContext;)Lorg/jetbrains/kotlin/descriptors/ValueParameterDescriptor;
public static final fun hasImplicitParameterReference (Lorg/jetbrains/kotlin/psi/KtLambdaExpression;Lorg/jetbrains/kotlin/descriptors/ValueParameterDescriptor;Lorg/jetbrains/kotlin/resolve/BindingContext;)Z
Expand Down
9 changes: 9 additions & 0 deletions detekt-psi-utils/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/KtIfExpression.kt
View file
Open in desktop
@@ -0,0 +1,9 @@
package io.gitlab.arturbosch.detekt.rules

import org.jetbrains.kotlin.KtNodeTypes
import org.jetbrains.kotlin.psi.KtContainerNodeForControlStructureBody
import org.jetbrains.kotlin.psi.KtIfExpression

fun KtIfExpression.isElseIf(): Boolean =
parent.node.elementType == KtNodeTypes.ELSE &&
parent.safeAs<KtContainerNodeForControlStructureBody>()?.expression == this
4 changes: 1 addition & 3 deletions ...ules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UseIfEmptyOrIfBlank.kt
View file
Open in desktop
Expand Up @@ -8,7 +8,7 @@ import io.gitlab.arturbosch.detekt.api.Issue
import io.gitlab.arturbosch.detekt.api.Rule
import io.gitlab.arturbosch.detekt.api.Severity
import io.gitlab.arturbosch.detekt.api.internal.RequiresTypeResolution
import org.jetbrains.kotlin.KtNodeTypes
import io.gitlab.arturbosch.detekt.rules.isElseIf
import org.jetbrains.kotlin.builtins.KotlinBuiltIns
import org.jetbrains.kotlin.lexer.KtTokens
import org.jetbrains.kotlin.name.FqName
Expand Down Expand Up @@ -84,8 +84,6 @@ class UseIfEmptyOrIfBlank(config: Config = Config.empty) : Rule(config) {
report(CodeSmell(issue, Entity.from(conditionCalleeExpression), message))
}

private fun KtExpression.isElseIf(): Boolean = parent.node.elementType == KtNodeTypes.ELSE

private fun KtIfExpression.condition(): Pair<KtExpression, Boolean>? {
val condition = this.condition ?: return null
return if (condition is KtPrefixExpression) {
Expand Down