java:S1067 Expression complexity in method chains

[reitzmichnicht (Michael Düsterhus)]

We are using Sonarqube 8.7 and I tried to active rule java:S1067.

My expectation was to find issues where to many expressions are concatenated, but I now have this false positive:

Reduce the number of conditional operators (6) used in the expression (maximum allowed 5).

  @Override
  public String toString() {
    return MoreObjects.toStringHelper(this)
                      .add("id", id)
                      .add("name", name)
                      .add("foo", foo)
                      .add("bar", bar)
                      .add("foo1", (foo1!= null) ? (foo1.length + " Byte") : "null")
                      .add("foo2", foo2)
                      .add("foo3", (foo3!= null) ? (foo3.length + " Byte") : "null")
                      .add("foo4", foo4)
                      .add("foo5", foo5)
                      .add("foo6", (foo6 != null) ? (foo6.length + " Byte") : "null")
                      .add("foo7", foo7)
                      .add("foo8", (foo8 != null) ? (foo8.length + " Byte") : "null")
                      .add("foo9", (foo9!= null) ? foo9.toUpperCase(Locale.ROOT) : "null")
                      .add("foo10", (foo10 != null) ? foo10.toUpperCase(Locale.ROOT) : "null")
                      .add("foo11", foo11)
                      .add("overallChecksum", overallChecksum)
                      .toString();
}

I think in chained method calls it should not sum up the expressions inside single methods.

Kind regards,
Michael

[Josh_Nunez]

This is another example of exposing Cyclomatic Complexity, or as Sonar likes to use Cognitive Complexity. It has been shown time and time again in practice and scholarly research that following the principles laid out by McCabe, no matter how trivial it seems, does result in easier to read and understand code that has less errors.

What the above warning is encouraging you to do is modularize your code so the same code is not copied in multiple places. The straight forward way to do this is to create a private helper function to generate the “labels” you are creating. So maybe something like this:

MoreObjects.toStringHelper(this)
    .add("id", id)
    . . .
    .add("foo1", toLabel(foo1, LABEL_BYTE))
    . . .
    .add("foo9", toLabel(foo9, LABEL_UPCASE))
    . . .
    .toString();

This eliminates all the repeated conditionals in your expression and pulls the logic about how to create these strings into one place in a function that can be reused. Now if you decide to change the business logic of how these “labels” are created you do that just in one place.

Doesn’t seem like a false positive to me.

[reitzmichnicht (Michael Düsterhus)]

In general your recommendation is correct. I was misguided by the rule examples about the term expression and how it is defined in the java language.