Suggestion: exception for SA1503: CurlyBracketsMustNotBeOmitted when the statement is a single return

Apr 20, 2011 at 8:14 PM

The curly braces in the code like follows seem very redunant:

if (!TryDoSomething(arg, param, value))
{
    return false;
}

if (!TryDoSomethingElse(arg, param, value))
{
    return false;
}

if (!TryMakeItOtherWay(arg, param, value))
{
    return false;
}

PerformRequiredActions();
CommitChanges();
return true;

Compare to more clean:

if (!TryDoSomething(arg, param, value))
    return false;

if (!TryDoSomethingElse(arg, param, value))
    return false;

if (!TryMakeItOtherWay(arg, param, value))
    return false;

PerformRequiredActions();
CommitChanges();
return true;

What do you think about making exception to the rule for single return/break/continue/throw statements?

Coordinator
Apr 20, 2011 at 8:21 PM
I would say they are essential.

A.



On 20 Apr 2011, at 20:14, "Alexey_Ru" <notifications@codeplex.com> wrote:

From: Alexey_Ru

The curly braces in the code like follows seem very redunant:

if (!TryDoSomething(arg, param, value))
{
    return false;
}

if (!TryDoSomethingElse(arg, param, value))
{
    return false;
}

if (!TryMakeItOtherWay(arg, param, value))
{
    return false;
}

PerformRequiredActions();
CommitChanges();
return true;

Compare to more clean:

if (!TryDoSomething(arg, param, value))
    return false;

if (!TryDoSomethingElse(arg, param, value))
    return false;

if (!TryMakeItOtherWay(arg, param, value))
    return false;

PerformRequiredActions();
CommitChanges();
return true;

What do you think about making exception to the rule for single return/break/continue/throw statements?

Apr 21, 2011 at 7:17 AM

I agree with Andy. Putting the "return" within braces makes for easier reading.

Apr 21, 2011 at 2:23 PM

Seems like I'm the minority who prefers a more compact form of the early-exit-on-failure pattern (in distinction from usual control flow branching).

Anyway, thanks for you answers.

Apr 24, 2011 at 12:51 PM

Alex, if you really want a compact  form, put the return in the same line as the if.

Apr 24, 2011 at 3:24 PM
Edited Apr 24, 2011 at 3:27 PM

Thanks Paulo. However I think this is not a good pattern for writing 'ifs'. I even suspect it is a bug if the StyleCop allows this (cannot check this up right now but I looked at the StyleCop sources and didn't find an exception for the case when everything is written in a single line).

Coordinator
Apr 24, 2011 at 3:27 PM
No. It's not a bug. It's correct behaviour. Single line if with curly brackets is allowed.

A.



On 24 Apr 2011, at 15:24, "Alexey_Ru" <notifications@codeplex.com> wrote:

From: Alexey_Ru

Thanks Paulo. However I think this is not a good pattern for writing 'ifs'. I even suspect this is a bug if the StyleCop allows this (cannot check this up right now but I looked at the StyleCop sources and didn't find an exception for the case when everything is written in a single line).

Apr 24, 2011 at 4:15 PM

Seems like I misunderstood Paulo. I thought he claims that curly braces could be omitted in a single line 'if' (and this would be a bug). I cannot even imagine a single line 'if' with curly brackets :) Anyway, it would look rather unpretty, and would be nice to have another rule for that.

Apr 24, 2011 at 4:21 PM

I was thinking more on the lines of:

if (!TryDoSomething(arg, param, value)) return false;

But, since I never use it, I wasn't aware that this triggers a:

SA1503: The body of the if statement must be wrapped in opening and closing curly brackets.

Which, when satisfied, triggers a:

SA1501: A statement containing curly brackets must not be placed on a single line. The opening and closing curly brackets must each be placed on their own line.

My personal opinion is that such code should be allowed, but only for simple statemetns - without the curly braces. Compound statements - with curly braces - should start on the next line.

 

Feb 8, 2013 at 1:53 PM
Edited Feb 8, 2013 at 2:09 PM
Sorry for resurrecting this but I totally agree with Alexey and here's why:

The doc says:
// When the curly brackets are omitted, it is possible to introduce an error in the code
// by inserting an additional statement beneath the if-statement. For example:

if (true)
    this.value = 2;
    return this.value;
Most of the IDEs are smart enough to fix this by decreasing the indent of the last line, though it's understandable for StyleCop to enforce a good formatting, regardless of any IDE that may or may not fix this issue.

So let's assume that it's not fixed.
if (true)
    return this.value;
    this.value += 2;
In the above example, the if block returns at its first line so it's clear that the second line will not be executed.
So I believe using the following keywords in an if block without curly braces should be completely acceptable:
  • return
  • break
  • continue
  • throw
  • goto
Or better, StyleCop could let curly braces to be omitted but enforce a blank line after the statement.
So this would be invalid:
if (count == null)
    throw new ArgumentNullException("count", "Count cannot be null.");
if (count > 5)
    throw new ArgumentOutOfRangeException("count", "Count can't be greater than 5);
But this would be perfectly valid:
if (count == null)
    throw new ArgumentNullException("count", "Count can't be null.");

if (count > 5)
    throw new ArgumentOutOfRangeException("count", "Count can't be greater than 5);
And in my opinion, that also would be much easier to read than this:
if (count == null)
{
    throw new ArgumentNullException("count", "Count cannot be null.");
}

if (count > 5)
{
    throw new ArgumentOutOfRangeException("count", "Count can't be greater than 5);
}
To sum it all up:
StyleCop should allow curly braces to be omitted and force a blank line after the statement instead.
-or-
StyleCop should make an exception to return, break, continue, throw and goto keywords.

This one is the only StyleCop rule that I disable and it hurts me.
Feb 8, 2013 at 4:58 PM
I don't agree..

As I said before,, a single line would still be readable. As soon as you change lines, it's better to always have the block intead of the single statement.
Aug 21, 2013 at 9:26 AM
I totally agree with safakgur! Very often the curly braces only takes vertical-space! (newbies will love it as they often want to produce as long code as possible...)

However I agree as well that if the curly braces are omitted, then a blank line should follow. For me personally it is even fine if the indent of the following line is correct, but I'm fine with, if somebody doesn't like that style...

Btw, I like that style as well:
if(myCondition == false) return false;
or also
string myVariable = (myCondition == false) ? "bla" : " another bla";
They make code
Aug 22, 2013 at 12:53 AM
Edited Aug 22, 2013 at 12:54 AM
I'm not sure how anybody could think that:
if (count == null)
{
    throw new ArgumentNullException("count", "Count cannot be null.");
}

if (count > 5)
{
    throw new ArgumentOutOfRangeException("count", "Count can't be greater than 5);
}
Is a more appropriate style than:
if (count == null)
    throw new ArgumentNullException("count", "Count can't be null.");

if (count > 5)
    throw new ArgumentOutOfRangeException("count", "Count can't be greater than 5);
I like the idea of enforcing a blank line if curly braces are ommited, however it should not be enforced when the following statement is an else if or else:
if (condition)
    DoSomething();
else if (otherCondition)
    DoSomethingElse();
else
    PerformDefaultAction();
Stylecop exists to "enforce a set of style and consistency rules".
We disable the SA1503 rule because we do not feel that curly braces should be forced for a single line conditional statement.
I think if you start making exceptions to the rule, you will end up confusing developers as to when the rule is/is not enforced.
I do think however, that a rule to enforce a blank line after an if statement if not followed by an else if or else would be beneficial.
Aug 22, 2013 at 1:25 AM
I would.

If you use fluent interfaces you usually break it into several lines for not ending up with a longer than you can read line.

If don't use curly braces when the statement is not on the same line of the condition, it's not immediately clear if that's a member chain or a condition plus a statement.
Aug 22, 2013 at 3:27 AM
Edited Aug 22, 2013 at 3:30 AM
If you are forced to use curly braces you cannot easily tell if the result of the conditional is going to execute one or multiple statements.

With or without curly braces, indentation makes it clear how many statements are executed. For a fluent interface, the subsequent statements will always be more indented than the first statement:
kernel.Bind<InternalEmailSender>()
     .ToSelf()
     .WithConstructorArgument("smtpServer", AppConfigAdapter.SmtpServer)
     .WithConstructorArgument("internalEmailSender", App...InternalEmailSender)
     .WithConstructorArgument("internalEmailReplyTo", App...InternalEmailReplyTo);
If the above appears in a conditional, regardless of whether the conditional uses curly braces or not it is clear that method chaining is being used.

If curly braces are ommited, I would argue there is no loss of fidelity, it is clear that if the conditional is entered into, one method chained statement will be executed.
When curly braces are used, it is no longer instantly apparent if a single or multiple statements will be executed before examining the inside of the brace block.
The developer is then required to look at the code inside the curly braces to determine the number of statements.

Because of indentation, I argue that forcing curly braces only decreases the immediately readable intent of the code, while introducing no advantage.
Aug 22, 2013 at 7:53 AM
I like strict idea of this rule. IMHO code is more readable in such case - If condition don't interferrence with Return statement.
Also I don't care that code will occupy couple extra rows. If you don't write huge methods (i.e. use the Extract Method refactoring) you won't need compaction tools.
Aug 22, 2013 at 11:00 AM
The lack of need of the curly braces for a single statement is a reminiscent of C. It's not even coherent. type, method and property declarations have mandatory curly braces.

I wonder if C# was being developed from scratch now it would be like that. Powershell isn't.
Aug 26, 2013 at 2:30 PM
Edited Aug 26, 2013 at 2:31 PM
Totally agree with Andy, Paulo and Serge: Curly braces must never be omitted.
Feb 27, 2015 at 10:14 AM
Edited Feb 27, 2015 at 10:17 AM
I would like to have an exception on this rule. When the single line is a jump statement like: break, continue, return, throw and their yield counterparts; we should be able to omit them.

Because, the extra highlighting of the statement and the subsequent error it would generate when you would mistakenly add code between the if/else and the jump statement (creating unreachable code). Would be sufficient to maintain the code quality. While having the ability to omit the braces in those scenario's, will makes our code more condense and readable.