[EC2][Java] Problem with detecting applicable cases

[cyChop]

Describe the bug
EC2 shows issues for successive "ifs" even when the condition is not related, even without an else condition, and switch is not applicable.

Also, the rule explanation doesn't even once refer to switch in compliant code examples.

Action suggestions (splitting into several issues might make tracking easier)

• Change the rule label in code, possibly split it into two ("use a switch" and "reduce the number of conditionals")
• Reducing the number of conditionals: the conditions must be evaluated, to check that indeed the conditions can be collapsed. The plugin must not return issues as hotspots. This basically means an issue for each if in code…
• Prefering switch: only for if (a == a1) {...} else if (a == a2) {...} else {...} (at least for Java < 17)

To Reproduce

if (a == null) {
    a = "";
}
// Issue EC2: "Using a switch statement instead of multiple if-else if possible"
if (b == null) {
    b = "";
}
// Issue EC2: "Using a switch statement instead of multiple if-else if possible"

Expected behavior
The issue should only show when there is a proven way to reduce the number of conditionals.

Screenshots
If applicable, add screenshots to help explain your problem.

Software Versions

• SonarQube Version: 9.9.1
• Plugin Version: ? (I'm not SonarQube admin)

[cyChop]

@dedece35 said:

Regarding EC2 rule, sorry, I don't agree. Please check refactoring work in issue #216 and associated PR #222.
Unfortunately, this work is not released yet (see CHANGELOG.md) but soon.

I feel like I need to clarify my intents for this issue: I agree with the rule, but there are implementation errors.

About the issue itself

• Even though I agree, I still feel we don't have any indication on the environmental impact. I agree with it from a quality/maintainability perspective, and I suppose there might be some specific optimizations because switch was designed for some specific cases.

• Because switch was designed for specific cases, it can only be used in those cases. Triggering the issue for cases where switch doesn't apply is, by definition, false positives.

Why it matters

False positives are huge issue, even more with a plugin this young. ecoCode is still very young and needs to convince its audience.

This is something we won't achieve if, when we add the plugin to existing projects, we have thousands of warnings for code we cannot fix because the rule generates false positives. This is what happened when we added ecoCode to our SonarQube instance, by the way.

Why I made this an issue

Because I won't have time to submit a PR anytime soon, for personal and professional reasons.
I won't get my company or client to sponsor me for open source contribution, no matter how hard I'd love it.

Still, I thought my input and experience could be of value, so I share what I saw and know.
This is not criticizing the work that has been done, it's sharing data that may help make it even better.

This is more a "hey guys, maybe we could add this on a roadmap" than "this needs fixing, it's broken." Though in its current state, my client deactivated this rules due to the amount of false positives.

How I expect this can be solved

I give basic samples of valid code. My way to go about it would be to go TDD: prepare the tests with those examples of valid code, and then fix the rule until all tests are green.

We can disagree

I'm fallible and ready to hear it. I'm always happy to learn. Just explain and prove me; show me specific data, or counter-examples.

I can also hear a "You might be right, but we won't do it because…"

On the other hand, I have trouble hearing a flat "You're wrong" or "No." Please explain your answers when I went through the trouble of providing detailed data or explanation.

Cases I agree should be switches

Equality tests for one variable

if (a == value1) {
} else if (a == value2) {
} else  {
}

This is basically the definition of a switch case before Java 17, if a is a primitive or an enum.
Since Java 8 (I think), this also works with String.

Cases that are detected but I think are valid code

Only applies to simple equality conditions

if (checkSomethingAbout(a)) {}
// could probably be written as switch, starting Java 17, but not before

if (a == null && b == valueB || c != valueC) {}

Not all condition types can be rewritten as switches (at least not before Java 17).
So before Java 17, this rule should mainly focus on simple equality checks.

If the condition is a call to a method or a complex condition with AND or OR operators, it cannot be written as a switch.

== null

if (a == null) {}

One condition type must be excluded from this rule: switch does not allow null, as far as I know.
So if (a == null) cannot be replaced with a switch.

Conditions on different elements

if (a == valueA) {
} else if (b == valueB) {
}

switch tests different values for one argument, so the code above cannot be replaced with a switch and therefore should not raise an issue.

Cases I'm not sure about

If or if/else conditions

if (a == valueA) {}

if (a == valueA) {} else {}

I see no gain here in writing a switch, but I may be wrong.
In that case, the plugin need to provide some specific data about the gain.

[diyfr]

The rule exists in several Java IDEs Example for IntelliJ

For information The opposite rule to respect java:1301 : Minimum 3 cases for using switch)

switch statements are useful when there are many different cases depending on the value of the same expression.
For just one or two cases, however, the code will be more readable with if statements.

Noncompliant code example

switch (variable) {
  case 0:
    doSomething();
    break;
  default:
    doSomethingElse();
    break;
}

Compliant solution

if (variable == 0) {
  doSomething();
} else {
  doSomethingElse();
}

[ashvinappi]

Hello, the explanation for this rule can be improved and below is the proposed improvement for the documentation for the markdown for this rule.

A switch statement is much faster since most compilers create a jump table for switch statements. A jump table is an abstract data structure that is used by modern days compilers to transfer the flow control to another location.

Hence it has also been tested by measuring the runtime of each implementation and the difference is almost half each time.
Average time for switch statement in our case is 300 nano seconds.

[Kiiv]

I second @cyChop here. There is a lot of false positives on this one in my concern.

Other examples that rise this issue :

    if (a == null) {
      a = initA();
      if (a == null) {
        throw new Exception()
      } else {
        doSomething
      }

public int compare(MyObject o1, MyObject o2) {
    if (o1 == null && o2 == null) {
      return 0;
    } else {
      if (o1 == null) {
        return -1;
      } else if (o2 == null) {
        return 1;
      } else {
        return o1.getSomething().compareTo(o2.getSomething());
      }
    }

    if (a != null && a.size() == 1) {
       doSomething();
    } else {
      if (a == null || a.isEmpty()) {
         doSomethingElse();
      } else {
        throw new Exception();
      }
    }

Even if those pieces of code are not the prettiest, I don't think they should raise this issue.