csharpsquid:S1301 ("switch" statements should have at least 3 "case" clauses") should be changed to "at least 2 'case' clauses"

[m...@soloplan.de]

Sometimes I see code that checks first on one enum value, then on another enum value:

if (person.PetPreference == PetType.Cat)
{
  // do something
}
else if (person.PetPreference == PetType.Dog)
{
  // do something
}

In that case I recommend people to use a switch instead as it is in my opinion better readable and, as we are checking an enum, chances are that later even more cases will follow:

switch (person.PetPreference)
{
  case PetType.Cat:
    // do something
    break;
  case PetType.Dog:
    // do something
    break;  
}

But this code triggers csharpsquid:S1301.

I agree that one single switch statement should be written as "if". And a switch statement with one "case" and the "default" should be written as if/else, which is the example of that rule. But in scenarios where I have at least two specific "case" checks (and the replacement would be "if/else if"), I think the switch statement is the better choice.

Rule description:
http://www.sonarlint.org/visualstudio/rules/index.html#sonarLintVersion=2.6.0&ruleId=S1301

Thanks,
Markus

[Tamas Vajk]

Hello Markus, 

I agree with you, I think this rule should not trigger at all when the switch is based on enums.

I created a ticket for it: https://jira.sonarsource.com/browse/SLVS-1006

Regards,

Tamas

Tamas VAJK | SonarSource
Language Team

http://sonarsource.com

--
You received this message because you are subscribed to the Google Groups "SonarQube" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sonarqube+unsubscribe@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/0ea0591e-2195-46e9-a91f-f318a948cee1%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[m...@soloplan.de]

Hello Tamas,

first of all: thanks for considering chaning this rule :-)
However, I'm not sure that ignoring enums altogether is the right way to got. Let's have a look at some examples:

1. sometimes, one enum value stand out of the others, for instance:

if (person.PetPreference == PetType.None)
{
  // person doesn't like pets at all
}

If the rule simply ignores switch statements on enums, you could just as well write

switch (person.PetPreference)
{
  case PetType.None:
    // person doesn't like pets at all
    break;
}

But I would want the rule to suggest the if-statement here.

2. Let's say someone adds a default statement to this switch:

switch (person.PetPreference)
{
  case PetType.None:
    // person doesn't like pets at all
    break;
  default:
    // person likes pets
    break;
}

One could argue that an if/else would still better convey the intention of the code. Check for one specific value and act accordingly, treat all other values the same.

if (person.PetPreference == PetType.None)
{
  // person doesn't like pets at all
}
else
{
  // person likes pets
}

So I think that checking for 2 actual "case" statements (not counting "default") would be the better choice.

Same can be argued for integer based values

3.

switch (person.NumberOfLotteryWins)
{
  case 0:
    // normal person
    break;
  default:
    // lucky person
    break;  
}

Would probably make more sense to be written as

if (person.NumberOfLotteryWins == 0)
{
  // normal person
}
else
{
  // lucky person
}

4. but with 2 actual case statements a switch makes more sense

switch (person.NumberOfLotteryWins)
{
  case 0:
    // normal person
    break;
  case 1:
    // lucky person
    break;  
  default:
    // unnaturally lucky person
    break;  
}

Writing this in an "if/else if" construct would look weird. Basically I guess if/else is fine, but as soon as you would have to use "if/else if" a switch is better.
That's why I suggested to check for "2 actual case statements" (not counting "default").

Regards,
Markus

To unsubscribe from this group and stop receiving emails from it, send an email to sonarqube+...@googlegroups.com.

[Michael Piefel]

Am Donnerstag, 1. September 2016 10:33:42 UTC+2 schrieb m...@soloplan.de:

[…] But in scenarios where I have at least two specific "case" checks (and the replacement would be "if/else if"), I think the switch statement is the better choice.

But having a switch with two cases and no default will then trigger a different warning. (At least for Java, it will.)

[m...@soloplan.de]

Hi Michael,

you are right, that would be rule "csharpsquid:S131": switch/Select" statements should end with a "default/Case Else" clause
http://www.sonarlint.org/visualstudio/rules/index.html#sonarLintVersion=2.6.0&ruleId=S131

But rule S1301 and S131 are separate rules with different intentions and of course can be controls separately in the SonarQube configuration. S1301 is about code style and when to use switch or if/else. S131 is about logical completeness (aka "cover every case").

I just wanted to give examples for possible combinations in switch statements. As long as I have to actual "case" checks in my switch, S1301 should accept my switch and not suggest "if/else if". Whether I should also add a "default" case is then up to rule S131.

Regards,
Markus

[Tamas Vajk]

Sorry, I'm having second thoughts on this modification.

Your original sample:

if (person.PetPreference == PetType.Cat)
{
  // do something
}
else if (person.PetPreference == PetType.Dog)
{
  // do something
}

looks bad to me too, and I would also want to change it, but the main reason why I would want to change it is because it doesn't have a closing else. If you add the closing else, or you convert it to the equivalent switch with two cases and one default, none of the rules trigger.

Tamas