False postive for fb-contrib:SEO_SUBOPTIMAL_EXPRESSION_ORDER?

Question

SonarQube thinks the following code violates rule fb-contrib:SEO_SUBOPTIMAL_EXPRESSION_ORDER (note, the code example is simplified and not logical):

class Foo {

   boolean baz;

   boolean foo() { 
      return bar() && baz==Baz.VALUE; //Violation of fb-contrib:SEO_SUBOPTIMAL_EXPRESSION_ORDER
   }

   boolean bar() { 
      return baz == Baz.VALUE_2; 
   }

}

enum Baz {
    VALUE, VALUE2
}

Performance - Method orders expressions in a conditional in a sub optimal way (fb-contrib:SEO_SUBOPTIMAL_EXPRESSION_ORDER)

This method builds a conditional expression, for example, in an if or while statement where the expressions contain both simple local variable comparisons, as well as comparisons on method calls. The expression orders these so that the method calls come before the simple local variable comparisons. This causes method calls to be executed in conditions when they do not need to be, and thus potentially causes a lot of code to be executed for nothing. By ordering the expressions so that the simple conditions containing local variable conditions are first, you eliminate this waste. This assumes that the method calls do not have side effects. If the method do have side effects, it is probably a better idea to pull these calls out of the condition and execute them first, assigning a value to a local variable. In this way you give a hint that the call may have side effects.

I think it's reasonable for the rule implementation to look inside the actual expression and if the content is a value check then not trigger a violation.

Is this a bug or am I missing something?

Answer 1

You almost gave the answer in your question already:

This causes method calls to be executed in conditions when they do not need to be, and thus potentially causes a lot of code to be executed for nothing. By ordering the expressions so that the simple conditions containing local variable conditions are first, you eliminate this waste.

FB-Contrib wants you to turn around the expression:

boolean foo() { 
    return baz==Baz.VALUE && bar();
}

This way, bar() needs to be executed only if baz==Baz.value. Of course this is something which the compiler or the JVM might optimize, so it would come down to a micro-benchmark to figure out if this precaution is actually necessary.

But it makes sense on a syntactical level, because calling a method is generally more expensive than a value check. So you don't need to look inside the method to tell that. Any guesses on the inlining behavior of the compiler / JVM are probably wrong anyway.