Runtime: Question: Single statement `if` formatting

[jacobcarpenter]

Looking at System.Threading.Tasks.Dataflow (amazing library by the way), I see three different variations of formatting of single-statement if statements. In _one_ method.

From DataflowBlock.OutputAvailableAsync

One line:

``` c#
if (source == null) throw new ArgumentNullException("source");

[**Two lines**](https://github.com/dotnet/corefx/blob/2bf5a7185fd81949da4d47a432662d8fe80989f6/src/System.Threading.Tasks.Dataflow/src/Base/DataflowBlock.cs#L1493-L1494):

``` c#
if (cancellationToken.IsCancellationRequested)
    return Common.CreateTaskFromCancellation<bool>(cancellationToken);

With braces:

c# if (target.Task.IsCompleted) { return target.Task; }

Are the guidelines for formatting these more nuanced than I'm able to see, or is this just inconsistent formatting? Is this kind of inconsistent formatting normal and acceptable? Is there a preference for how new code should be formatted?

[kiranprabhu]

One here in System.IO.FileInfo
public override String Name
{
get { return _name; } //single line
}

[weshaggard]

We don't have an explicit guideline around this and personally I'm fine with any of them depending on the situation. My general preference would be the "Two lines" version for most cases unless you are nesting in lots of scopes then I would use the "With braces" version to ensure things don't accidentally go in an unintended scope.

[jacobcarpenter]

Thanks! That sounds great.

I'm happy to have this issue closed, unless you want further discussion about extending the C# Coding Style with some guidance for this situation.

[mellinoe]

I would imagine the difference between the first vs. the second (one-line vs. two-line) is just the length of the line; we probably didn't want to have such a long single line for the second example, for readability, etc.

Also, in general (although there are still exceptions...) you may see that parameter validation checks are generally on one or two lines without braces, and parts of the main body logic are more likely to have braces. These aren't really "rules", and they definitely aren't written down anywhere.

[paulomorgado]

There are a few horror stories about using the two lines style. Ask Apple.

[terrajobst]

My gut feel tells me that we should settle on a specific convention.

/cc @jaredpar and @Priya91

[jaredpar]

I think we should go with the two line version.

The goal of the style guidelines is to have consistency within the code base. As others have pointed out the single line version isn't always realistic because of line length. Given that I'd go with the two line version.

[sharwell]

I use the following rule:

• Never use the single-line form
• Without braces
  
  
  
  • Only allowed if the body of _every_ block associated with an if/else if/.../else compound statement is placed on a single line.
  
  
• With braces
  
  
  
  • Always acceptable
  
  
  • If any block of an if/else if/.../else compound statement uses braces, then _all_ blocks in the compound statement use braces
  
  
  • If a single statement body spans multiple lines (e.g. if it is split to keep a line from being too long), then braces must be used
  
  

[terrajobst]

My personal preference is identical to what @sharwell wrote.

[jaredpar]

I think that is a very sensible set of rules.

On Saturday, January 10, 2015, Immo Landwerth [email protected]
wrote:

My personal preference is identical to what @sharwell
https://github.com/sharwell wrote.

—
Reply to this email directly or view it on GitHub
https://github.com/dotnet/corefx/issues/381#issuecomment-69475130.

Jared Parsons
http://blog.paranoidcoding.com/
http://twitter.com/jaredpar

[weshaggard]

I agree. I tried to summarize it in our code guideline wiki as:

A single line statement block can go without braces but the block must be properly indented on its own line and it must not be nested in other statement blocks that use braces (See issue 381 for examples).

Please let me know if you think the guidance needs further clarification.

Thanks @jacobcarpenter for posing the question and @sharwell for suggesting the guidance, I'm closing this issue now.

[paulomorgado]

Apart the use of goto, for me, one of the causes of Apple's goto fail bug was allowing two line ifs without the braces.

In the extremely rare occasions I use unbraced "blocks" is if they are if the whole statement (or statement segment) is on the same line.

[robinsedlaczek]

I know this issue was closed long time ago, but if you allow, I like to add a thought: we also discussed this topic with our teams when we created our internal guidelines long time ago. And we came to the point, that we always want to use braces. Also for single line statement blocks. So we must not think about and incorrect scoping can not occur accidentally.

[davidsh]

cc: @CIPop

+1. I agree that all if statements should use braces even for single line statements.

[CIPop]

+1 I completely agree with @robinsedlaczek
I've seen more than one security bug caused by this lately. The cost of these and their respective fixes outweigh the advantages of getting two lines less of code.

Alternative to @paulomorgado broken link above: Apple's goto fail.

[rkeithhill]

I agree with @paulomorgado, having debugged code like:

``` c#
if (cond)
statement1;
statement2;

I would explicitly disallow the two line (without braces) case.

Another alternative is **always** require braces but allow:

``` c#
if (source == null) { throw new ArgumentNullException(nameof(source)); }

Then let line length dictate whether the above can be put on one line or if it should span four lines. Always requiring braces would make it easy for a tool to flag when braces aren't being used.

[robinsedlaczek]

I agree with @rkeithhill:

Always requiring braces would make it easy for a tool to flag when braces aren't being used.
But one-liners are harder to read since you have to realize the condition and the expression(s) in the block. Regarding human perception, multi-line would speed it up because there is a visual structure for different syntax elements (code in the block is indented etc.).

Maybe this argumentation is a bit too academic... :)

[sharwell]

Always requiring braces would make it easy for a tool to flag when braces aren't being used.

Tools are already available to enforce the current rules. They haven't been incorporated into this repository yet, but I'm hoping they will be eventually.

...having debugged code like:

if (cond)
    statement1;
    statement2;

Between Visual Studio's formatter and the codeformatter tool, this should never exist in this repository. If code like this cannot exist, then the primary argument for requiring braces fades a bit and you end up with the more flexible rule I described previously.

[SunnyWar]

This has been studied a couple times. I recall (but could not find) a study that was done on C/C++ about 20 years ago. Then there is this study on Java: https://dzone.com/articles/omitting-braces-not-just-a-mat
The conclusions are always the same, use braces, even with a single statement after the if.

[Priya91]

The C# formatting guidelines in this repository was formulated based on VS default formatting rules. I tried the 3 code sample in the description box on VS, and on ctrl+k+d, VS doesn't alter the formatting, all 3 ways are accepted.
Having worked with roslyn and corefx, i have noticed we using the guidelines outlined by @sharwell in this comment, and i would prefer those rules.

[davidsh]

The simplest rule, IMHO, is to ALWAYS use braces, period.

[MgSam]

Not to prod the sacred cow too much, but having an always-use-braces rule could be "less expensive" in terms of wasted space if the opening brace was on the same line as condition:

C# if (condition) { foo(); }

[weshaggard]

Thanks for all the feedback but I'm still inclined to keep the existing style guideline. We have a ton of preconditions that use this no-brace style and I think that is the primary use case for it and I don't feel it is worth adding 2 lines per precondition per method only containing braces in all our libraries. Yes there is some risk but I believe there is risk in any guideline we choose and that risk should be mitigated by code reviews and testing.

[davidfowl]

Always use braces to avoid things like this:

https://www.imperialviolet.org/2014/02/22/applebug.html

:smile:

[CIPop]

Always use braces to avoid things like this [...]

:+1: to that! Simple rule, 0 risk.

[benaadams]

One line for simple if (simple test) return; or if (simple test) throw; tests; and the return and throw must be clearly visible and preform no state alteration or braces.

Otherwise should be braces, as it will fail at some point the future when someone edits or refactors.

Everything else with braces for exactly this reason:

Always use braces to avoid things like this: [...]

Just our take... Its a standard with a reason, where braces are is meh, unless you are doing js when same line as it will sometimes auto add a semicolon and change the meaning of the code - again something with a reason.

[MisinformedDNA]

I have a proposal for a new C# syntax (https://github.com/dotnet/csharplang/issues/1785) for single-line if statements. I'm not trying to gather support as much as I am gathering direction on whether any of this is worth it. I found this thread and thought you would be a perfect group to ask.

So here's the question: Is there a way to _not_ have curly braces, but _not_ run into the issues of omitting curly braces?

I think we all can agree that braces are safer than no braces (though "safer" is relative). But we like to exclude them because they are ugly and actually make the code less readable

if (condition)
{
    statement;
}

if (condition) { statement; }

I, personally, find these easier to read, but then I am adding some non-zero risk, right?

if (condition) statement;   // if short

if (condition)   // if long
    statement;

So the question rephrased, can I have the best of both worlds? Is it possible to keep the risk low and have concise and readable code?

With all the new arrow expressions, I immediately thought of something like

if (condition) => return expression;    // ISSUE: Arrows currently only support expressions
return if (condition) => expression;    // ISSUE: There are obvious problems here too
return expression if condition;    // Looking better
statement if condition;    // More generic form
statement    // <= if long
    if condition;

Would something like statement if condition; (which already exists in Perl and Ruby) help achieve that? There is only one statement allowed, so it seems to be that we prevent running into the "Apple issue", but it's been suggested that it's just as likely to be looked over.

Ignoring cost and resources to implement, do you think this provides an advantage in terms of higher readability and lower risk? Or am I just chasing a unicorn?

[gafter]

I should note that the current dotnet coding guidelines forbid code like

c# using (var lexer = MakeLexer(text, offset)) using (var parser = MakeParser(lexer)) { Something(parser); }

because the second using is not "properly" indented. That is a pattern seen widely in dotnet code.

[davidsh]

I should note that the current dotnet coding guidelines forbid code like
using (var lexer = MakeLexer(text, offset))
using (var parser = MakeParser(lexer))
{
Something(parser);
}
because the second using is not "properly" indented. That is a pattern seen widely in dotnet code.

Feel free to update those guidelines. We've found that keeping the using clauses lined up like this (instead of indented) is actually better and makes the code more readable. That is why you see lots of instances of that pattern in the CoreFx code.

[benaadams]

Could take a lead from VB and allow multi declaration usings 😉

using (var lexer = MakeLexer(text, offset), var parser = MakeParser(lexer))
{
    Something(parser);
}

[gafter]

@davidsh Can you please approve https://github.com/dotnet/corefx/pull/31816