Remove 'case' from rule 1535

[julianbartel]

Suggestion is to remove requirement for braces around case blocks as defined in rule 1535.

• Personal background: trying to strictly code against the guideline, case-blocks were looking weird with braces. After searching through both company and GitHub repositories, I have found practically no single C# example of case blocks being surrounded with braces. It seems to be absolutely uncommon.

• Technical background: While braces have a direct influence on control flow of if, for, foreach etc., the control flow in a switch/case is defined by control flow keywords (break, return, throw etc.). This is enforced by the compiler - you can not fall through multiple case labels accidently.

[dennisdoomen]

Hmm, I see some value in that. @bkoelman what do you think?

[bkoelman]

The primary reason we require blocks inside case clauses, is to workaround an unfortunate design choice in the C# language that surprises developers:

Q: Why are variables that are declared in one case statement in scope for other cases? A: https://softwareengineering.stackexchange.com/questions/195032/why-doesnt-c-have-local-scope-in-case-blocks#answer-358671

Aside from the lack of symmetry (one block declares variables for all others too), it gets problematic when types are different in various case clauses:

switch (token.Type)
{
    case TokenType.Number:
        int value = ParseNumber(source);
        break;
    case TokenType.Operator:
        Operator value = ParseOperator(source); // error: 'value' is already declared as type 'int'
        break;
}

Lastly, I usually see people that don't use block scope adding extra line breaks between/around case clauses to improve readability. Using block scope removes the need for that.

In reply to the questions:

• If indents look weird when using block scope, correct your formatting settings.
• This is not related to control flow, but to variable scope. Because case fall-through is not possible in C#, developers no longer consider switch a set of goto statements, but they see the case clauses like the branches of an if-else, which have separate scopes.

[dennisdoomen]

Why are variables that are declared in one case statement in scope for other cases?

Wow. I did not even know that.

[bkoelman]

Wow. I did not even know that.

Yeah, switch statements are a bit weird. Did you know this is valid C# code?

switch (token.Type)
{
    case TokenType.Number:
    {
        if (source.Length > 0)
        {
            goto case TokenType.Operator;
        }

        return;
    }
    case TokenType.Operator:
    {
        if (source != null)
        {
            goto case default;
        }

        throw null;
    }
    default:
    {
        goto case TokenType.Number;
    }
}

[dennisdoomen]

What is seen cannot be unseen....

[julianbartel]

@bkoelman Thanks for the detailed explanation of your intentions here.

If indents look weird when using block scope, correct your formatting settings.

Actually, it's not looking weird because of the formatting, but because of the braces itself :-) But as being said, style is more a personal thing, so ignore this aspect now.

This is not related to control flow, but to variable scope. [...]

Ok, that's an aspect I did not have in mind. I can hardly remember a situation where this really was an issue, though. Referring to your example code, the compiler already catches the situation.

I feel all arguments have been shared, let's try to come to a conclusion: I still would suggest this change since the current rule differs from the community standard and problems with scoping are more or less rare and (depending on the concrete situation) handled by the compiler.

If you decide to keep the rule as it is, I suggest to add a note about the scoping aspect at least to make the intention clear.

[dennisdoomen]

I'm in favor of removing the requirement for braces and make it optional. If you need braces to deal with variable problems, it's time to refactor the implementation and move some of that logic to a method.

[bkoelman]

Your response puzzles me @dennisdoomen. So there's a language feature that does not behave as most people expect. We cannot change that, but isn't that why we write guidance like this document? To help the masses that don't know or case about all nuances, but just want a way that keeps them away from these pitfalls? Similar to the threading and performance guidance.

The scope block is not to deal with variable problems, but to make switch work as people intuitively expect it would work. Nor is it about large code blocks that need refactoring. It affects usually only one variable in a method that only contains the single switch statement. And what are the options?

1. Declare the shared variable outside the switch statement (violates AV1521)
2. Rename each occurrence to something else, like var error, var otherError, var customError etc. Confusing to a reader, wondering: why all the different names for the same thing?
3. Declare the variable in the first block. But wait, now you cannot safely reorder them anymore.
4. Convert to if-else-if to get rid of the mess.

It gets worse when type hierarchies are involved. The shared variable is first declared as Manager, but that breaks the other case. It is then changed to User, but now the original code breaks. Then they start adding casts inside the cases. And so on and so on...

The compiler error is clear, but shows no way to solve it elegantly. This rule does. I've seen developers doing the above 'fixes' over and over again, which is why I introduced the rule. Removing it again feels like a step back and taking away helpful guidance.

I can only hope that the use of switch statements will be replaced by switch expressions in time, who do not have these unexpected behaviors.

[bkoelman]

I'd be okay with adding an explanation, although most rules do not try to explain the pros and cons that were considered. And I think this document is intended to contain best practices that come from experience over time, more than to echo what most developers are already doing. The standards here are quite high, most code bases do not reach that level of quality. Finally, I know that the compiler/VS special-case the switch curly handling. ReSharper has formatting settings specific to deal with curlies in switches, so maybe it is not so uncommon after all.

Making it optional puts us back to discussion during code reviews whether curlies should be used on a case-by-case basis and cannot be automated via formatting settings. It defeats the purpose of choosing a standard and get it over with.

[dennisdoomen]

OK. Sorry @julianbartel, I'm with @bkoelman here. We like to keep the braces in the guidelines. Although I think we should mention some of the rationale for those as Bart explained.

I hope you respect our decision. We definitely respect you for trying to convince us and being so passionate about this topic.

[julianbartel]

@dennisdoomen @bkoelman Fair enough. I think all pros and cons are valid; in the end it depends on how we weight them up. Anyway, thanks for the constructive discussion 👍

[dennisdoomen]

thanks for the constructive discussion

Always!