C# switch-too many case clauses - Better suggested fix

Improvement to C# rule S1479: “switch” statements should not have too many “case” clauses…

  1. The title suggests that the problem is “too many case clauses”, but the issue expressed by the rest of the documentation is that there are too many conceptual items in the list to be switched over. Perhaps the title should be “Too many “case” clauses in a “switch” statement indicates mapping against multiple sets of choices”.

  2. The documentation provides a suggestion: “A real map structure would be more readable and maintainable, and should be used instead.” But there is no such thing as a C# ‘map structure.’ Instead, suggest using an enumeration. The process of creating the enumeration is likely to make the developers think about what values belong together and which don’t.

thanks, @Stephen!

For reference - https://rules.sonarsource.com/csharp/RSPEC-1479

You are suggesting to use

“Too many “case” clauses in a “switch” statement indicates mapping against multiple sets of choices”

instead of

“switch” statements should not have too many “case” clauses.

I don’t see the benefit on this wording. indicates mapping against multiple sets of choices does not suggest it is a code smell. In general, a switch statement is used for mapping against multiple sets of choices, even if there is a low number of case clauses or a high number.

The docs refer to a dictionary, based on key-value pairs. Indeed the wording should be adapted to the .net world (cc @Nicolas_Harraudeau)

@Andrei_Epure I agree with you that the original title is more understandable and it matches better what the rule actually detects. What about modifying the description as follow:
“”"
When switch statements have large sets of case clauses, it is usually an attempt to map two sets of data. A Dictionary should be used instead to make the code more readable and maintainable.
“”"

Then we can add a code example to make it even more explicit:

Noncompliant Code Example
With a “Maximum number of case” set to 14

public class TooManyCase
{
    public int switchCase(char ch)
    {
        switch(ch) {  // Noncompliant
            case 'a':
            case 'b':
            case 'c':
            case 'd':
            case 'e':
                return 1;
            case 'f':
            case 'g':
            case 'h':
            case 'i':
            case 'j':
                return 2;
            case 'k':
            case 'l':
            case 'm':
            case 'n':
            case 'o':
                return 3;
            default:
                return 4;
        }
    }
}

Compliant Solution

using System.Collections.Generic;

public class TooManyCase
{
    Dictionary<char, int> matching = new Dictionary<char, int>()
    {
        {'a', 1}, {'b', 1}, {'c', 1}, {'d', 1}, {'e', 1},
        {'f', 2}, {'g', 2}, {'h', 2}, {'i', 2}, {'j', 2},
        {'k', 3}, {'l', 3}, {'m', 3}, {'n', 3}, {'o', 3}
    };

    public int withDictionary(char ch)
    {
        int value;
        if (this.matching.TryGetValue(ch, out value)) {
            return value;
        } else {
            return 4;
        }
    }
}

Would this make the rule more understandable @Stephen, @Andrei_Epure ?

1 Like

After re-reading the documentation, the replies above, and the rule’s source code, let me take a step back and go at this from a different direction…

The S1479 rule is simply looking at “too many ‘case’ statements”, i.e., the large number of options makes the code less readable and maintainable. It does not look at whether the large number of ‘case’ statements is due to “an attempt to switch over two sets of data”, e.g., trying to sort varieties of both apples and oranges in one switch statement.

Therefore, I suggest:

  • Change the rule’s Description to say something like “Too many ‘case’ clauses in a switch statement make the code less readable and maintainable.”
  • Remove the “map two sets of data” comment altogether, since there is no logic for that. (That would be an interesting but separate–and exceedingly difficult–analysis to perform.)
  • Have the Non-Compliant / Compliant Code examples show ways to reduce the number of ‘case’ statements. … including the ‘mapping’ possibility (which I now understand, thanks @Andrei_Epure) written above by @Nicolas_Harraudeau. (Although I am not sure that abstracting values into a Dictionary object really makes the code more readable. It just moves the complexity somewhere else.)