New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Question: Single statement `if` formatting #381

Closed
jacobcarpenter opened this Issue Jan 9, 2015 · 31 comments

Comments

Projects
None yet
@jacobcarpenter
Contributor

jacobcarpenter commented Jan 9, 2015

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:

if (source == null) throw new ArgumentNullException("source");

Two lines:

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

With braces:

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

This comment has been minimized.

Show comment
Hide comment
@kiranprabhu

kiranprabhu Jan 9, 2015

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

kiranprabhu commented Jan 9, 2015

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

@weshaggard

This comment has been minimized.

Show comment
Hide comment
@weshaggard

weshaggard Jan 9, 2015

Member

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.

Member

weshaggard commented Jan 9, 2015

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.

@terrajobst terrajobst added the area-Meta label Jan 9, 2015

@jacobcarpenter

This comment has been minimized.

Show comment
Hide comment
@jacobcarpenter

jacobcarpenter Jan 9, 2015

Contributor

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.

Contributor

jacobcarpenter commented Jan 9, 2015

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

This comment has been minimized.

Show comment
Hide comment
@mellinoe

mellinoe Jan 9, 2015

Contributor

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.

Contributor

mellinoe commented Jan 9, 2015

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

This comment has been minimized.

Show comment
Hide comment
@paulomorgado

paulomorgado Jan 10, 2015

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

paulomorgado commented Jan 10, 2015

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

@terrajobst

This comment has been minimized.

Show comment
Hide comment
@terrajobst

terrajobst Jan 10, 2015

Member

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

/cc @jaredpar and @Priya91

Member

terrajobst commented Jan 10, 2015

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

/cc @jaredpar and @Priya91

@jaredpar

This comment has been minimized.

Show comment
Hide comment
@jaredpar

jaredpar Jan 10, 2015

Member

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.

Member

jaredpar commented Jan 10, 2015

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

This comment has been minimized.

Show comment
Hide comment
@sharwell

sharwell Jan 10, 2015

Member

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
Member

sharwell commented Jan 10, 2015

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

This comment has been minimized.

Show comment
Hide comment
@terrajobst

terrajobst Jan 10, 2015

Member

My personal preference is identical to what @sharwell wrote.

Member

terrajobst commented Jan 10, 2015

My personal preference is identical to what @sharwell wrote.

@jaredpar

This comment has been minimized.

Show comment
Hide comment
@jaredpar

jaredpar Jan 10, 2015

Member

I think that is a very sensible set of rules.

On Saturday, January 10, 2015, Immo Landwerth notifications@github.com
wrote:

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


Reply to this email directly or view it on GitHub
#381 (comment).

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

Member

jaredpar commented Jan 10, 2015

I think that is a very sensible set of rules.

On Saturday, January 10, 2015, Immo Landwerth notifications@github.com
wrote:

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


Reply to this email directly or view it on GitHub
#381 (comment).

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

@weshaggard

This comment has been minimized.

Show comment
Hide comment
@weshaggard

weshaggard Jan 11, 2015

Member

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.

Member

weshaggard commented Jan 11, 2015

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.

@weshaggard weshaggard closed this Jan 11, 2015

@paulomorgado

This comment has been minimized.

Show comment
Hide comment
@paulomorgado

paulomorgado Mar 4, 2015

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.

paulomorgado commented Mar 4, 2015

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

This comment has been minimized.

Show comment
Hide comment
@robinsedlaczek

robinsedlaczek Sep 17, 2015

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.

robinsedlaczek commented Sep 17, 2015

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

This comment has been minimized.

Show comment
Hide comment
@davidsh

davidsh Sep 17, 2015

Member

cc: @CIPop

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

Member

davidsh commented Sep 17, 2015

cc: @CIPop

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

@CIPop

This comment has been minimized.

Show comment
Hide comment
@CIPop

CIPop Sep 17, 2015

Member

+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.

Member

CIPop commented Sep 17, 2015

+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.

@CIPop CIPop reopened this Sep 17, 2015

@rkeithhill

This comment has been minimized.

Show comment
Hide comment
@rkeithhill

rkeithhill Oct 1, 2015

I agree with @paulomorgado, having debugged code like:

if (cond)
    statement1;
    statement2;

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

Another alternative is always require braces but allow:

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.

rkeithhill commented Oct 1, 2015

I agree with @paulomorgado, having debugged code like:

if (cond)
    statement1;
    statement2;

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

Another alternative is always require braces but allow:

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

This comment has been minimized.

Show comment
Hide comment
@robinsedlaczek

robinsedlaczek Oct 1, 2015

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... :)

robinsedlaczek commented Oct 1, 2015

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

This comment has been minimized.

Show comment
Hide comment
@sharwell

sharwell Oct 1, 2015

Member

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.

Member

sharwell commented Oct 1, 2015

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 comment has been minimized.

Show comment
Hide comment
@SunnyWar

SunnyWar Oct 1, 2015

Contributor

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.

Contributor

SunnyWar commented Oct 1, 2015

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

This comment has been minimized.

Show comment
Hide comment
@Priya91

Priya91 Oct 1, 2015

Member

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.

Member

Priya91 commented Oct 1, 2015

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

This comment has been minimized.

Show comment
Hide comment
@davidsh

davidsh Oct 1, 2015

Member

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

Member

davidsh commented Oct 1, 2015

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

@MgSam

This comment has been minimized.

Show comment
Hide comment
@MgSam

MgSam Oct 2, 2015

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:

if (condition) {
    foo();
}

MgSam commented Oct 2, 2015

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:

if (condition) {
    foo();
}
@weshaggard

This comment has been minimized.

Show comment
Hide comment
@weshaggard

weshaggard Oct 2, 2015

Member

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.

Member

weshaggard commented Oct 2, 2015

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.

@weshaggard weshaggard closed this Oct 2, 2015

@davidfowl

This comment has been minimized.

Show comment
Hide comment
@davidfowl

davidfowl Oct 2, 2015

Contributor

Always use braces to avoid things like this:

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

image

😄

Contributor

davidfowl commented Oct 2, 2015

Always use braces to avoid things like this:

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

image

😄

@CIPop

This comment has been minimized.

Show comment
Hide comment
@CIPop

CIPop Oct 2, 2015

Member

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

👍 to that! Simple rule, 0 risk.

Member

CIPop commented Oct 2, 2015

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

👍 to that! Simple rule, 0 risk.

@benaadams

This comment has been minimized.

Show comment
Hide comment
@benaadams

benaadams Oct 3, 2015

Collaborator

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.

Collaborator

benaadams commented Oct 3, 2015

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

This comment has been minimized.

Show comment
Hide comment
@MisinformedDNA

MisinformedDNA Aug 14, 2018

Collaborator

I have a proposal for a new C# syntax (dotnet/csharplang#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?

Collaborator

MisinformedDNA commented Aug 14, 2018

I have a proposal for a new C# syntax (dotnet/csharplang#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

This comment has been minimized.

Show comment
Hide comment
@gafter

gafter Aug 16, 2018

Member

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.

Member

gafter commented Aug 16, 2018

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.

@davidsh

This comment has been minimized.

Show comment
Hide comment
@davidsh

davidsh Aug 16, 2018

Member

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.

Member

davidsh commented Aug 16, 2018

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

This comment has been minimized.

Show comment
Hide comment
@benaadams

benaadams Aug 16, 2018

Collaborator

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

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

benaadams commented Aug 16, 2018

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

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

This comment has been minimized.

Show comment
Hide comment
@gafter

gafter Aug 16, 2018

Member

@davidsh Can you please approve #31816

Member

gafter commented Aug 16, 2018

@davidsh Can you please approve #31816

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment