SonarQube - Java rule "S128" - Why the rule complains about the fact that a break statement is missing when this is obviously not necessary?

Question

I can't really figure out why Sonar keeps complaining about the fact that I "don't have a break statement" even if it's not needed..

My switch:

    public static String lookupVoyageId(String referenceNumber, String sender) {
    switch (sender) {
        case "400_HGENT":
        case "200_HAPEN":
        case "500_HOOST":
            Preconditions.checkArgument(referenceNumber.contains("-"));
            return referenceNumber.split("-")[0];
        case "600_HZEEB":
            Preconditions.checkArgument(referenceNumber.length() >= 6);
            return referenceNumber.substring(0, 6);
        case "800_BVL":
            throw new TransferException("This reference number for IBIS isn't according to the requirements. Can't implement it yet.");
        case "MCCD":
            throw new TransferException("This reference number for MCCD isn't according to the requirements. Can't implement it yet.");
        default:
            throw new TransferException("The sender (" + sender + ") couldn't be identified.");
    }
}

and sonar keeps giving me the critical: "A switch statement does not contain a break"

Why is this? I don't need any breaks in this switch?

I know it might be a specific case, but I can't find anything on the web.

For which line does it give you the error? Isn't it for the fall-through cases? Personally, I'd like to see a `// fall-through` comment in there, but I doubt that't what Sonar is complaining about... — Petr Janeček, Jun 30 '15 at 11:06
@Slanec It's just on the first line of the switch. And I've got the same critical in another switch without fall through. — GregD, Jun 30 '15 at 11:08
@Slanec Actually, the comment might very well fix the warning. According to http://stackoverflow.com/questions/5479019/is-sonar-replacement-for-checkstyle-pmd-findbugs sonar uses, among other libs, checkstyle. And checkstyle (http://checkstyle.sourceforge.net/config_coding.html#FallThrough) expects a comment if fall through is intended. — Magnilex, Jun 30 '15 at 11:39
It would be really great to precise the rule key on which this issue is raised. If this issue is raised by rule S128 then this will detect every fallthrough even intentional. — benzonico, Jun 30 '15 at 11:45
@Magnilex SonarQube relies mainly on its own analyzer unless specifically instructed otherwise :) — benzonico, Jun 30 '15 at 11:46
@Magnilex I've tried adding the comment but it didn't change anything. The critical is still there — GregD, Jun 30 '15 at 12:37
@benzonico How could I find the precise rule key? It seems it isn't displayed on sonar? — GregD, Jun 30 '15 at 12:39
This should be precised in the documentation. Given that issue is critical I would say that this is squid:S128 — benzonico, Jun 30 '15 at 12:46

score 1 · Answer 1 · answered Aug 05 '19 at 16:38

1

If you can't decrease the number of switch case or can't refactor the code you can suppress the warning with

 @SuppressWarnings({"squid:S128", "squid:S1479"}

Sample usage here

answered Aug 05 '19 at 16:38

Melad

1,184
14
18

Somnath Musib · Answer 2 · 2016-01-07T18:17:09.763

Note: I am not trying to answer the question. But lets see what this particular rule says.

Rule S128 says:

Switch cases should end with an unconditional "break" statement

When the execution is not explicitly terminated at the end of a switch case, it continues to execute the statements of the following case. While this is sometimes intentional, it often is a mistake which leads to unexpected behavior.

Noncompliant Code Example

switch (myVariable) {
  case 1:                              
    foo();
    break;
  case 2:  // Both 'doSomething()' and 'doSomethingElse()' will be executed. Is it on purpose ?
    doSomething();
  default:                               
    doSomethingElse();
    break;
}

Compliant Solution

switch (myVariable) {
  case 1:                              
    foo();
    break;
  case 2: 
    doSomething();
    break;
  default:                               
    doSomethingElse();
    break;
}

Exceptions

This rule is relaxed in the following cases:

switch (myVariable) {
  case 0: // Empty case used to specify the same behavior for a group of cases.
  case 1:                               
    doSomething();
    break;
  case 2:  // Use of return statement
    return;
  case 3:   // Use of throw statement
    throw new IllegalStateException();
  default:  // For the last case, use of break statement is optional
    doSomethingElse();
}

References: https://sonar.spring.io/rules/show/squid:S128?layout=false

This is not really an answer or a solution to the OP's issue… — Didier L, Jan 07 '16 at 15:12
It IS an answer. Obviously, sonarqube cries, because it is not common to use the fall through. But this is written in the second sentence inside the yellow quote. — Koshinae, Jan 07 '16 at 18:24

score 0 · Answer 3 · answered Jun 11 '16 at 00:23

Sonar can't know if the code snippet is working as intended. It cannot understand the business logic of your application, so it doesn't know that your code is supposed to work like that.

What Sonar can know is that this pattern (i.e., switch statements that fall-through) is a common source of hard to find bugs. For that reason, Sonar as a code-quality tool discourages working in this way, as part of an overall goal of reducing common mistakes.

SonarQube - Java rule "S128" - Why the rule complains about the fact that a break statement is missing when this is obviously not necessary?

3 Answers3