-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
#13970
Comments
One here in System.IO.FileInfo |
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. |
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. |
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. |
There are a few horror stories about using the two lines style. Ask Apple. |
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. |
I use the following rule:
|
My personal preference is identical to what @sharwell wrote. |
I think that is a very sensible set of rules. On Saturday, January 10, 2015, Immo Landwerth notifications@github.com
Jared Parsons |
I agree. I tried to summarize it in our code guideline wiki as:
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. |
Apart the use of goto, for me, one of the causes of Apple's goto fail bug was allowing two line 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. |
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. |
cc: @CIPop +1. I agree that all |
+1 I completely agree with @robinsedlaczek Alternative to @paulomorgado broken link above: Apple's goto fail. |
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. |
I agree with @rkeithhill:
Maybe this argumentation is a bit too academic... :) |
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.
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. |
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 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. |
The simplest rule, IMHO, is to ALWAYS use braces, period. |
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();
} |
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. |
Always use braces to avoid things like this: https://www.imperialviolet.org/2014/02/22/applebug.html 😄 |
👍 to that! Simple rule, 0 risk. |
One line for simple 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:
Just our take... Its a standard with a reason, where braces are is meh, unless you are doing |
I have a proposal for a new C# syntax (dotnet/csharplang#1785) for single-line 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 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? |
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 |
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. |
Could take a lead from VB and allow multi declaration usings 😉 using (var lexer = MakeLexer(text, offset), var parser = MakeParser(lexer))
{
Something(parser);
} |
@davidsh Can you please approve dotnet/corefx#31816 |
Looking at
System.Threading.Tasks.Dataflow
(amazing library by the way), I see three different variations of formatting of single-statementif
statements. In one method.From
DataflowBlock.OutputAvailableAsync
One line:
Two lines:
With braces:
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?
The text was updated successfully, but these errors were encountered: