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

Clean Code New rules / language support
1

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.

2

thanks, @Stephen!

For reference - C# static code analysis: "switch" statements should not have too many "case" clauses

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)

3

@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
4

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.)
1 Like
8

Thanks for your suggestions @Stephen. I also got some input from @JolyLoic and you both agree that replacing a switch-case with a dictionary in the code example above is not a good improvement. The resulting code is not more readable and performances are worse.

Thus I suggest an alternative change to rule S1479:

  • stop raising issues when every case in a big switch-case statement has only one statement.
  • update rule title: “switch” statements with many “case” clauses should have only one statement
  • update rule message: either reduce the number of “case” clauses to {X} or have only one statement per “case”.
  • update rule description:

===
The switch statement should be used only to clearly define some new branches in the control flow. When a switch has many case clauses and some of those closes contains multiple statements, the resulting code is hard to read and maintain.
This rule raises an issue when a switch block has many case clauses, and some of those clauses contain more than one statement.

Noncompliant Code Example

public class TooManyCase
{
    public int switchCase(char ch, int value)
    {
        switch(ch) {  // Noncompliant
            case 'a':
                return 1;
            case 'b':
                return 2;
            case 'c':
                return 3;
            // ...
            case '-':
                if (value > 10) {
                    return 42;
                } else if (value < 5 && value > 1) {
                    return 21;
                }
                return 99;
            default:
                return 1000;
        }
    }
}

Compliant Solution

public class TooManyCase
{
    public int switchCase(char ch, int value)
    {
        switch(ch) {
            case 'a':
                return 1;
            case 'b':
                return 2;
            case 'c':
                return 3;
            // ...
            case '-':
                return handleDash(value);
            default:
                return 1000;
        }
    }
    public int handleDash(int value)
    {
        if (value > 10) {
            return 42;
        } else if (value < 5 && value > 1) {
            return 21;
        }
        return 99;
    }
}

===

The only drawback I see is that S1479 and S1151 will be very close, which can be a little confusing. Yet the new title and description should make the difference clear.

Does this make sense?

1 Like
9

I would say to move forward with this suggestion. I opened #3050 to do the update

1 Like