SA1503: CurlyBracketsMustNotBeOmitted (Part 1)

Parenthetical Remarks

StyleCop rule SA1503 is designed to complain whenever

The opening and closing curly brackets for a C# statement have been omitted.

What does this mean, and is it a good rule?

Syntactic Structures

In C#, the bodies (or consequents) of certain flow control and looping constructs, such as if, while, and for statements, can syntactically be any single statement. For example, the underlined statements below are all single-statement bodies:

if (x == 1)
  y = 2;
else
  z = 3;

while (x < 10)
  x++;

for (var i = 0; i < 10; i++)
  x *= i;

Obviously there are times when you want the body of your construct to contain, rather than a single atomic statement, a sequence of two or more such statements. This is accommodated by the use a special type of single statement called a block, comprising just the required sequence of statements enclosed in curly braces, as shown below. Note that the entire contents of the curly braces, including the braces themselves, still comprise a single statement - which just happens to be of the block type:

if (x == 1)
{
  y = 2;
  z = 3;
}

These flow control and looping constructs can be nested within one another. A simple example of such nesting is this:

if (A())
  if (B())
    C();
  else
    D();

Since the rule in C# is that an else clause gets associated with the nearest available if statement, the else in this example has been indented to line up correctly with the second if statement, to which it properly belongs.

Problem?

Some people point out that the indentation can be applied incorrectly, and that the resultant statement

if (A())
  if (B())
    C();
else
  D();

can be ambiguous. Of course it's not ambiguous; after all, the compiler has absolutely no problem in applying the correct rules of precedence in a case like this, and coming up with the one correct result. However, it is true to say the above example is confusing, since it looks as though the else clause refers back to the first if statement rather than matching the second. The obvious remedy is to correct the indentation - and in fact to enforce consistent and correct indentation in future, so that such confusion cannot occur.

A Bad Fix

Sadly, those same people who insist on allowing badly formed indentation to pollute their code, often compound their error by suggesting a rather odd way of fixing it. Simply disallow any statement type except a block in these contexts! The result is a proliferation of brackets:

if (A())
{
  if (B())
  {
    C();
  }
  else
  {
    D();
  }
}

Although much harder to read, this version does at least have the virtue that its intention is quite clear. However, that clarity has not come about from the addition of braces. No, the alignment of the braces merely served to highlight the indentation error which was causing the confusion, and which incidentally has also been corrected in this latest version. Correct the indentation and we remove the confusion, braces or none. If anything, the braces are making it more difficult to follow the structure of nested statements like these, by making them twice as tall as they need be, and physically separating the components that you're trying to line up visually.

Another Example

In the above referenced documentation of rule SA1503, the following example is given:

if (true)
  this.value = 2;      
  return this.value;

Glancing at this code, it appears as if both the assignment statement and the return statement are children of the if-statement. In fact, this is not true. Only the assignment statement is a child of the if-statement, and the return statement will always execute regardless of the outcome of the if-statement.

Okay, so far I'm agreeing with you. But sadly we soon descend into nonsense:

StyleCop always requires the opening and closing curly brackets to be present, to prevent these kinds of errors:

if (true)
{
  this.value = 2;      
  return this.value;
}

Yeah, no. Now this so-called example lacks any credibility. Look, if that first fragment compiles, the one without the braces, then it must be sitting at the very end of its enclosing method - because it ends with an unconditional return, anything after which is unreachable code. Which of course you're already detecting, and in fact treating as an error, right? Right, so it's the last thing in the method. But this means that the second version can't compile at all, since there needs to be another unconditional return statement after it; the compiler will be justly livid about its absence (not all paths return a value, nag nag nag...).

Preliminary Conclusion

When I started looking into this issue, I was inclined to accept switching on rule SA1503. Anecdotal evidence seemed to support the idea that it could prevent bugs. And then, for me at least, there was this clincher: even Eric Lippert appeared to be suggesting that compulsory braces represented a solution to the classic dangling else problem, and one which he wished had been applied to C# from day one!

Then I realised that SA1503 wanted to do much more than just enforce braces in an if statement where an else clause was present. It wanted braces everywhere! That was when I began to pay closer attention to the arguments being offered, both for and against the measure.

The faulty examples above are typical of much I've encountered in researching what now appears to me a particularly misguided and ineffectual StyleCop rule. In seeking to mitigate against bad code layout by making wholesale changes to its syntactic structure, we not only miss our target entirely, but find ourselves instead lowering the code quality and readability. SA1503 therefore seems a bad rule from this perspective.

Next time: quick edits & legacy code.