#1131 complex method should be less strict with simple when entries #1135

rock3r · 2018-09-17T11:02:28Z

This fixes #1131 by weighing simple when entries to 0.5 (was 1.0) by default, and allowing users to customise the weight if they want to.

This requires the McCabe visitor to internally keep a floating-point sum of the complexities, but we still emit an integer value to keep backwards compatibility (and for simplicity).

This PR also improves ComplexMethod's tests, and adds better facilities to test Findings and ThresholdedCodeSmells.

rock3r · 2018-09-17T11:02:36Z

CC @Mauin

rock3r · 2018-09-17T14:21:37Z

Hm, we may have hit a bug with this PR. I get a green Travis, but on AppVeyor, I have these flagged:

Ruleset: potential-bugs - 1h debt
	UnsafeCast - [isThresholded] at C:\projects\detekt\detekt-test\src\main\kotlin\io\gitlab\arturbosch\detekt\test\ThresholdedCodeSmellAssert.kt:12:36
	UnsafeCast - [smell] at C:\projects\detekt\detekt-test\src\main\kotlin\io\gitlab\arturbosch\detekt\test\ThresholdedCodeSmellAssert.kt:26:15
	UnsafeCast - [smell] at C:\projects\detekt\detekt-test\src\main\kotlin\io\gitlab\arturbosch\detekt\test\ThresholdedCodeSmellAssert.kt:35:15

But none of those three is an unsafe cast as there are asserts on front of those casts. How is this not flagged in Travis but we get false positived on AppVeyor?

For now I'll suppress and add an issue tracking the problem.

rock3r · 2018-09-17T14:27:11Z

Ticket opened: #1137

Suppressions pushed.

rock3r · 2018-09-17T17:01:35Z

@Mauin can you re-trigger the JDK 9 build on AppVeyor? It failed with some random socket error.

…ict_with_simple_when_entries # Conflicts: # detekt-test/src/main/kotlin/io/gitlab/arturbosch/detekt/test/FindingsAssertions.kt

schalkms

Thanks for the rework and the idea with the when entry weight! I think this option is a good compromise. I vote for the simpleWhenEntryWeight=1 as the default value.

schalkms · 2018-09-18T20:35:41Z

detekt-cli/src/main/resources/default-detekt-config.yml

@@ -77,6 +77,7 @@ complexity:
    active: true
    threshold: 10
    ignoreSingleWhenExpression: false
+    simpleWhenEntryWeight: 0.5


I just want to raise the question, if this should be the default value. I would argue for the value 1. Otherwise, detekt would not comply with the McCabe metric. The class McCabeVisitor would also need another name.

I'm voting for 0.5 simple when entries are simple and one liners.

schalkms · 2018-09-18T20:37:16Z

detekt-rules/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/ComplexMethod.kt

@@ -23,6 +23,7 @@ import org.jetbrains.kotlin.psi.KtWhenExpression
 * @configuration threshold - MCC threshold for a method (default: 10)
 * @configuration ignoreSingleWhenExpression - Ignores a complex method if it only contains a single when expression.
 * (default: false)
+ * @configuration simpleWhenEntryWeight - The weight used for simple (braceless) when entries. (default: 0.5)


As a user of detekt I would like want to have a bit more documentation for this config option. (range, what effect has value x)

schalkms · 2018-09-18T20:39:00Z

detekt-rules/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/ComplexMethodSpec.kt

+			val subject = ComplexMethod(config)
+			val code = """
+				internal fun Map<String, Any>.asBundle(): Bundle {
+					val bundle = Bundle(size)


Can we simplify this function to not have any dependencies to Android dev?
Can we also just have the when brace, since this test case should only test the weight of the when clause?

There is no dependency in here, it's just the sample code taken 1:1 from the ticket. Nothing is going to depend on Android because a string contains an Android class — we don't even have imports for it :)

schalkms · 2018-09-18T20:39:58Z

detekt-rules/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/ComplexMethodSpec.kt

+		}
+
+		it("does not trip for a reasonable amount of simple when entries") {
+			val config = TestConfig(mapOf(ComplexMethod.SIMPLE_WHEN_ENTRY_WEIGHT to "0.5"))


0.5 is the default value atm, isn't it? One could remove this statement..

schalkms · 2018-09-18T20:40:20Z

detekt-test/src/main/kotlin/io/gitlab/arturbosch/detekt/test/ThresholdedCodeSmellAssert.kt

+	return ThresholdedCodeSmellAssert(actual as ThresholdedCodeSmell?)
+}
+
+class ThresholdedCodeSmellAssert(actual: ThresholdedCodeSmell?) :


Good! Much needed assert class

arturbosch · 2018-09-19T04:32:06Z

To be honest, I do not like this change. As @schalkms mentioned, McCabe is all about "number of linearly independent paths" within a program. When using floating values this should be another rule then.

I propose to not count the complexity of simple paths within when when a config property is true. If all paths are simple, just count the whole when expressions as mccabe=1.

rock3r · 2018-09-19T08:17:19Z

Happy to make the change if there's a consensus, I take the point.

…ict_with_simple_when_entries

rock3r · 2018-09-19T12:54:11Z

I have created a PR on my fork comparing this solution and an ignore-based solution which adheres better to the definition of MCC. Please review it here: rock3r#2

arturbosch · 2018-09-20T05:13:03Z

@rock3r jap that's 90% how I would want this rule to be :). The other 10% is when if (entries.filter { it is KtBlockExpression}.size == 0) return 1 :)

arturbosch · 2018-09-23T18:38:35Z

I guess we go with the simple=0 variant?
Can you please open a PR with your new branch?/

rock3r · 2018-09-24T09:24:05Z

@arturbosch I need to finish up the change you asked me (got sidetracked by urgent things), then I'll merge those changes into this branch. Coming soon!

arturbosch · 2018-09-24T18:07:38Z

Thank you!

…_strict_with_simple_when_entries # Conflicts: # detekt-test/src/main/kotlin/io/gitlab/arturbosch/detekt/test/FindingsAssertions.kt

rock3r · 2018-10-02T16:46:17Z

Still WIP but have merged master in, should at least not have conflicts anymore

…ches 1131/Switch to ignoring simple branches

rock3r · 2018-10-06T07:18:51Z

Finally managed to get the changes @arturbosch requested in.

arturbosch · 2018-10-07T01:08:03Z

Thank you for the changes!
I rebased and merged this manually. Please use rebase when merging the master branch :)
We rebase everything to get a cleaner history ^^

rock3r added 4 commits September 17, 2018 11:26

Create ThresholdedCodeSmellAssertions

3aba53a

Weight simple when entries .5 in McCabeVisitor, allow overriding

75f237d

Weight braceless when entries .5 in ComplexMethod, add config for weight

645dac9

Fix outstanding test & detekt issues

fab854a

Mauin approved these changes Sep 17, 2018

View reviewed changes

Suppress false positive UnsafeCast in ThresholdedCodeSmellAssert.kt

909cc86

Merge branch 'master-mine' into 1131_ComplexMethod_should_be_less_str…

43ac734

…ict_with_simple_when_entries # Conflicts: # detekt-test/src/main/kotlin/io/gitlab/arturbosch/detekt/test/FindingsAssertions.kt

schalkms approved these changes Sep 18, 2018

View reviewed changes

Merge branch 'master-mine' into 1131_ComplexMethod_should_be_less_str…

2f3e875

…ict_with_simple_when_entries

rock3r mentioned this pull request Sep 19, 2018

1131/Switch to ignoring simple branches rock3r/detekt#2

Merged

rock3r added 3 commits October 2, 2018 18:44

Merge branch 'master-upstream' into 1131_ComplexMethod_should_be_less…

3886588

…_strict_with_simple_when_entries # Conflicts: # detekt-test/src/main/kotlin/io/gitlab/arturbosch/detekt/test/FindingsAssertions.kt

Convert ComplexMethod to a boolean flag for ignoring simple when entries

c896dc8

Add tests for McCabe visitor

5fff4f8

rock3r and others added 4 commits October 5, 2018 17:54

Add tests for MCC when expressions

7cf4d39

Use private setter instead of separate backing field

d1f94c3

Update documentation

fd097cc

Merge pull request #2 from rock3r/1131_switch_to_ignoring_simple_bran…

2e5a906

…ches 1131/Switch to ignoring simple branches

arturbosch approved these changes Oct 7, 2018

View reviewed changes

arturbosch closed this Oct 7, 2018

rock3r deleted the 1131_ComplexMethod_should_be_less_strict_with_simple_when_entries branch October 10, 2018 11:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#1131 complex method should be less strict with simple when entries #1135

#1131 complex method should be less strict with simple when entries #1135

rock3r commented Sep 17, 2018

rock3r commented Sep 17, 2018

rock3r commented Sep 17, 2018

rock3r commented Sep 17, 2018 •

edited

rock3r commented Sep 17, 2018

schalkms left a comment

schalkms Sep 18, 2018

vanniktech Sep 18, 2018

schalkms Sep 18, 2018

schalkms Sep 18, 2018

rock3r Sep 19, 2018

schalkms Sep 18, 2018

schalkms Sep 18, 2018

arturbosch commented Sep 19, 2018

rock3r commented Sep 19, 2018

rock3r commented Sep 19, 2018

arturbosch commented Sep 20, 2018

arturbosch commented Sep 23, 2018

rock3r commented Sep 24, 2018

arturbosch commented Sep 24, 2018

rock3r commented Oct 2, 2018

rock3r commented Oct 6, 2018

arturbosch commented Oct 7, 2018

#1131 complex method should be less strict with simple when entries #1135

#1131 complex method should be less strict with simple when entries #1135

Conversation

rock3r commented Sep 17, 2018

rock3r commented Sep 17, 2018

rock3r commented Sep 17, 2018

rock3r commented Sep 17, 2018 • edited

rock3r commented Sep 17, 2018

schalkms left a comment

Choose a reason for hiding this comment

schalkms Sep 18, 2018

Choose a reason for hiding this comment

vanniktech Sep 18, 2018

Choose a reason for hiding this comment

schalkms Sep 18, 2018

Choose a reason for hiding this comment

schalkms Sep 18, 2018

Choose a reason for hiding this comment

rock3r Sep 19, 2018

Choose a reason for hiding this comment

schalkms Sep 18, 2018

Choose a reason for hiding this comment

schalkms Sep 18, 2018

Choose a reason for hiding this comment

arturbosch commented Sep 19, 2018

rock3r commented Sep 19, 2018

rock3r commented Sep 19, 2018

arturbosch commented Sep 20, 2018

arturbosch commented Sep 23, 2018

rock3r commented Sep 24, 2018

arturbosch commented Sep 24, 2018

rock3r commented Oct 2, 2018

rock3r commented Oct 6, 2018

arturbosch commented Oct 7, 2018

rock3r commented Sep 17, 2018 •

edited