Update coding guidelines for control statement formatting #6904
Conversation
donnie-msft
left a comment
donnie-msft
left a comment
There was a problem hiding this comment.
While I overall don't see that a problem exists today with spacing with if-statements, have we looked into Roslyn analyzers first before taking an AI approach? As others pointed out, there's a lot of cases where we would not want extra spacing.
donnie-msft
left a comment
donnie-msft
left a comment
There was a problem hiding this comment.
We discussed offline - I'm neutral on this so if it's important to others to have a rigid guideline here, I'll trust you'll work out the right strategy.
| Don't | ||
| ```csharp | ||
| // Blank spaces do not help readability | ||
| for (int i = 0; i < 5; i++) | ||
| { | ||
|
|
||
| if (i % 2 == 0) | ||
| { | ||
|
|
||
| Console.WriteLine($"{i} is even."); | ||
|
|
||
| } |
There was a problem hiding this comment.
It looks like this is the only "Don't" example: a time when blank lines would not be appropriate. However, this example introduces blank lines in the middle of the code block which is not part of the proposed guideline so it's showing an example that isn't an exception to the proposal. The proposed guideline is to put spaces before a control block separating any statements above it.
| if (...) | ||
| ``` | ||
|
|
||
| Blank lines make it easier for readers to see where one group of statements ends and another begins. |
There was a problem hiding this comment.
In some cases, this is true. In others, I disagree as I mentioned in our meeting yesterday. In some cases, a statement and a control block serve as a group of code that is best read when together, without excessive blank spaces, such as in this contrived code example:
public int CalculateScore(int points, int modifier)
{
List<Player> activePlayers = GetActivePlayers();
if (activePlayers.Count == 0)
{
return -1;
}
int score = points * modifier;
if (score > 1000) {
score = 1000; // score capped at 1000.
}
return score;
}
There was a problem hiding this comment.
I do agree with having a blank space between control blocks. This can be handled with a coding guideline (and analyzer) to ensure that there is a blank line after every control block unless that next line is a curly brace closing another block.
| } | ||
| if (isNewRecord){ | ||
| Console.WriteLine("Congratulations!"); | ||
| }else{ |
There was a problem hiding this comment.
This example introduces a different issue than what the proposal is talking about. The issue of brace placement and spacing between braces should not be part of this example and should be demonstrated separately in a different coding guideline.
dotnet-policy-service
bot
added
the
Status:No recent activity
|
This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch. |
Bug
Fixes: #6804 (comment)
Description
Update coding guidelines as discussed in #6804 (comment)
PR Checklist