Remove CyclomaticComplexity checks

[pkoenig10]

This check is almost always a false positive that is blindly suppressed.

Here are some common examples of code that adheres to good design patterns and is conceptually simple, but has high cyclomatic complexity. I routinely find code like this that triggers the CyclomaticComplexity check.

• A method with many if-return blocks

  String foo = getFoo();
  if (foo == nul) {
      return foo;
  }
  
  Bar bar = getBar(foo);
  if (bar == null) {
      return;
  }
  
  ...

• Switch statements with many cases, especially if those cases contains additional branches

  return switch (foo) {
      case CASE_1 -> checkArgument(bar == 1 && baz == 2, ...);
      case CASE_2 -> checkArgument(bar == 3 && baz == 4, ...);
      ...
  }

[changelog-app]

Generate changelog in changelog/@unreleased

Type
See change types. Select one:

• Feature
• Improvement
• Fix
• Break
• Deprecation
• Manual task
• Migration

Description

The CyclomaticComplexity checkstyle check is no longer enabled by default.

Check the box to generate changelog(s)

• Generate changelog entry

[schlosna]

Wondering if we might want to tweak the config before removing this check completely. See https://checkstyle.sourceforge.io/apidocs/com/puppycrawl/tools/checkstyle/checks/metrics/CyclomaticComplexityCheck.html for defaults & definitions

I have seen a lot of suppressions, most of them are reasonably justified to avoid extracting methods in complex code flows (e.g. often in parsers), but there are also cases where it helps to extract some of the logic bits for simplicity, testing, JIT hot paths and not loading cold paths, etc.

Suggested change

	<module name="DesignForExtension"> <!-- Java Coding Guidelines: Design for extension -->
	<module name="CyclomaticComplexity"> <!-- Java Coding Guidelines: Reduce Cyclomatic Complexity -->
	<property name="max" value="15"/> <!-- Increase from default 10 to accommodate more complexity -->
	<property name="switchBlockAsSingleDecisionPoint" value="true"/>
	</module>
	<module name="DesignForExtension"> <!-- Java Coding Guidelines: Design for extension -->

[pkoenig10]

simplicity, testing

I think this is mostly a matter of taste. If a method is so complex that it is hurting readability, I'll probably refactor it without this check. And if I don't refactor it, that means I think the method is more readable as-is, at which point this check just becomes annoying.

JIT hot paths and not loading cold paths

I would prefer to not try and be smarter than the compiler and let it take care of the optimizations. The cases where performance is so important that these kinds of changes are meaningful are very few and far between and I'm not sure this complexity check is going to really be the motivating factor in those cases.

[schlosna]

In general I think we want the default gradle-baseline rules to cover 99%+ of code and if there's an exception we can suppress at the method or class level with reasonable comment as to why.

simplicity, testing

I think this is mostly a matter of taste. If a method is so complex that it is hurting readability, I'll probably refactor it without this check. And if I don't refactor it, that means I think the method is more readable as-is, at which point this check just becomes annoying.

Agree it's an element of taste, as are most of the style guide rules and recommendations, checkstyle rules, and error-prone checks to have the compiler nudge folks to do the right thing from the start. There are often exceptions, including this specific checkstyle rule.

I'm curious how often you hit this threshold in your own PRs and ones you're reviewing. If I'm reviewing code and someone has @SuppressWarnings("CyclomaticComplexity") I will probably take a closer look at the logic, branches, error handling, and test coverage to gain confidence that we're good. If the compiler can encourage folks to do that same thing and maybe refactor to simplify in both a more readable and testable fashion, that helps scale folks and increase iteration speed.

JIT hot paths and not loading cold paths

I would prefer to not try and be smarter than the compiler and let it take care of the optimizations. The cases where performance is so important that these kinds of changes are meaningful are very few and far between and I'm not sure this complexity check is going to really be the motivating factor in those cases.

In many cases writing smaller methods that are readable will help the runtime JIT compiler do the right thing leading both to better performance and more maintainable code in the long run. There are some cases where this isn't true, but those tend to be the small minority where it's worth documenting why a method is more complex both through comments and tests.

Also note that the readme would also need to be updated:

gradle-baseline/docs/best-practices/java-coding-guidelines/readme.md

Lines 555 to 566 in c0e8302

	### Reduce Cyclomatic Complexity
	Code with Cyclomatic Complexity above 10 will be rejected by
	Checkstyle's CyclomaticComplexity check. Its documentation:
	The complexity is measured by the number of if, while, do, for, ?:,
	catch, switch, case statements, and operators && and || (plus one) in
	the body of a constructor, method, static initializer, or instance
	initializer. It is a measure of the minimum number of possible paths
	through the source and therefore the number of required tests. Generally
	1-4 is considered good, 5-7 OK, 8-10 consider re-factoring, and 11+
	re-factor now!

[pkoenig10]

Looking at our large internal authentication product, we suppress this warning in 21 places.

Roughly a third of those appears to be cases where we probably should refactor the code.

The other two thirds look like one of the following:

• A method with a lot of if-return blocks

  Foo foo = getFoo();
  if (foo == null) {
      return;
  }
  
  if (!foo.isValid()) {
      return;
  }
  
  ...

• Switch statements, either on a enum with many values or with some short conditional logic in some case branches

  switch (this) {
      case FOO -> {
          if (isAllowed()) {
              setFooAllowed();
          }
      }
      case BAR -> { ... }
      ...

In these cases, it doesn't make sense to refactor this because:
- This logic is not reused in other methods
- Creating helper methods makes logic harder to follow
- Creating helper methods requires I come up with, likely useless, names for parts of this logic
- We can't create helper methods when control flow (return, break, continue, etc.) is involved

[schlosna]

@pkoenig10 for those two thirds of cases, I'm curious how many are switch related in which case we could relax the defaults to enable switchBlockAsSingleDecisionPoint (especially in more recent JDK where switch expressions and destructuring becomes much richer). For the others where refactoring would be not possible or net-negative, do you feel like suppressing for the method and/or class is too onerous? My general sense is that the CyclomaticComplexity check initially flags a good 80% of places where we'd want folks to better factor code, and in places where we're ok with the variance that team can suppress as needed.

[pkoenig10]

Yup that seems very reasonable to me. Didn't know about switchBlockAsSingleDecisionPoint, that sounds exactly like what I want!

[pkoenig10]

#2383

[pkoenig10]

Closing in favor of #2383