DotNetAnalyzers / StyleCopAnalyzers

An implementation of StyleCop rules using the .NET Compiler Platform
MIT License
2.66k stars 507 forks source link

Question/Rule Proposal for blank line after one line if #3068

Open Sebastriani opened 4 years ago

Sebastriani commented 4 years ago

I didn't find any rule to check for a blank line after one line if statements that don't use braces. I don't want to force the use of curly braces for one line if statements to be able to enforce a blank line separation from if to if (rule 1513)

If there is no such rule, I would like to propose it, something like "Single line if statements should be followed by a blank line"

I want to avoid this type of code:

if (somevar == somevalue)
    // A line of code
if (someothervar == someothervalue)
    // Another line of code

and favor this:

if (somevar == somevalue)
    // A line of code

if (someothervar == someothervalue)
    // Another line of code

without having to do this:

if (somevar == somevalue)
{
    // A line of code
}

if (someothervar == someothervalue)
{
    // Another line of code
}
SeidChr commented 4 years ago

To me it appears to be counter intuitive, that someone who prefers the readability limitations of single line if statements would want to add an empty line there. In my experience people ditch the curly braces when they want to write the most condensed code they can to cram as much as possible into one screen. But there may be others.

Since the last line after a closed curly braces needs to be an empty line too, the rule makes sense when it comes to readability. Also maintainability, because you can detect these blocks easier.

I just don't know if it would increase the readability on truly single line (without line break) statements. Especially many of them in a row.

Sebastriani commented 4 years ago

@SeidChr I think it's all about having options, if there where only a fixed set of rules that cannot be configured, I would probably agree with you but, at least in my case, I tend to use one line if statemens with or without braces depending on the context and how it seems easier to read in that context. I'm not one of those that goes in favor of the most condensed code, all the opposite, I think that writting well separated code introduces a much less significant cognitive load than if you squash everything without empy lines.

That being said, I'm in charge of making the rules for a project and I don't wan't to force the team to use braces for one line if statements, but I would like to force them to separate those statements with one blank line if they don't use braces.

As I said before, I think it's all about having options, you say for instance that "the las line after closed curly braces needs to be an emtpy line", but that (SA1513) is just a configurable rule as are all the others, just as I would like to add this one we are discussing about.

sharwell commented 4 years ago

I'm generally in favor of having an analyzer for this case, but there are a few exceptions that I believe would be important. The first two that come to mind are:

  1. Allow else or else if to follow without a blank line
  2. Allow consecutive if statements without a blank line if the child of each one either returns or throws (i.e. the behavior is equivalent to using else if)
Sebastriani commented 4 years ago

I hadn't think about it, now I'm thinking that allowing to not separate with a blank line for an "else"` would be ok but, if you allow to not do it when you have "else if", then the condition you write might clump with the code below and it can make it harder to visualize, more so if the condition has roughly the same length that the code below.

When I think about the format of the code I'm almost always thinking about reducing the cognitive load and make it easier for our brains to detect, isolate and process the pieces of code, mostly for those who will need to reverse engineer our code after we wrote it, and even for ourselves in the future.

I think it would be really nice if it's possible to make more than one rule regarding this to configure them at will to be able to combine them to have the desired result.

SeidChr commented 4 years ago

@SeidChr I think it's all about having options, if there where only a fixed set of rules that cannot be configured, I would probably agree with you but, at least in my case, I tend to use one line if statemens with or without braces depending on the context and how it seems easier to read in that context. I'm not one of those that goes in favor of the most condensed code, all the opposite, I think that writting well separated code introduces a much less significant cognitive load than if you squash everything without empy lines.

That being said, I'm in charge of making the rules for a project and I don't wan't to force the team to use braces for one line if statements, but I would like to force them to separate those statements with one blank line if they don't use braces.

As I said before, I think it's all about having options, you say for instance that "the las line after closed curly braces needs to be an emtpy line", but that (SA1513) is just a configurable rule as are all the others, just as I would like to add this one we are discussing about.

As strange as it sounds, i think when it comes to different codestyles there really shouldn't be much of an option. I really dislike that you even have the option to have alternative rules active. When it comes to this extended rule, it is an extension to the ruleset, which is active when you disable (not change) the required curlies rule. So it restricts the "free mind" to still write maintainable code. I can just hope no one prohibits the use of curlies on single line control flow statements.

Sebastriani commented 4 years ago

Why shouldn't be there much options of coding style? Shouldn't be actually the opposite and that each person/team could be able to choose whatever they see fit for their projects?

For instance, you have Linus Torvalds with his taste for coding style https://github.com/torvalds/linux/blob/master/Documentation/process/coding-style.rst He started the Linux Kernel, why woudn't he have the right to choose the coding style of the Kernel?

I think that tools should allow things, not prohibit them, and if the tools allows to prohibit a thing, let that be because the person in charge of using that tool decided it, not because the tool decided that for him.

It's you that have to respect the driving laws, not the car, the car is the tool, it deppends on you how to use it. IMHO.

SeidChr commented 4 years ago

Why shouldn't be there much options of coding style? Shouldn't be actually the opposite and that each person/team could be able to choose whatever they see fit for their projects?

Well, i fear to open an endless bucket of discussions now. So maybe we should not capture this ticket for it. I'll answer anyways.

Just a quick overview: Working for almost 13 years in the industry, ive seen alot of languages (mostly c#) and many many different code styles. The one thing i realized: every time you read a differntly styled piece of code, even though it has the absolute same meaning, it is harder or easier to understand when it is far or near to your own style. Next is, that every time you change the team, the project, the company, the room, the seat, you will be confronted with a different style of code which you have to wrap your head arround. Just for the sake of making your work harder. This somewhat resides in the "problem", that every fresh developer comes into a team which got its own style, which he will get used to and in the end praise it to others because he used it for so long that he just wont accept the downsides. "Flutter" for example follows a strict, globally unified coding style which is baked into the IDE and language. This would be really helpful for c# as well. StyleCop is the only set of guidelines which is well documented and has good reasoning behind i'ts rules. You have the option to ditch a single rule, if it doesnt apply to your situation (like curlies), but that wont exclude those who still write code following that style (do not punish the curlies ;) ). So whenever you get into a team, you can just write code in the StyleCop "default" style, and it will be fine. Also you will read code that is very near to your own style (StyleCop style). And for reasons you can find in the rule-documentation, the code will just be objectively better overall.

Also: differnt languages SHOULD, due to differnt language constructs, keywords and requirements, have different (but still per language unified) styles. Whoever thinks its fine to apply a java, c or vb coding style to c# needs to be punished ;)

Far too long for an "overview" and i guess somewhat off topic. Feel free to message me when you want to discuss this further :)

Sebastriani commented 4 years ago

Just a quick overview: Working for almost 13 years in the industry, ive seen alot of languages (mostly c#) and many many different code styles.

Well, I've been working for the last 20 years in the industry, also in different languages, last 8 in C# mainly, but I don't get your point, would you wrap it up for me please? Or are you actually saying that it would be better off setting a fixed set of rules so everybody has to comply to them?

Also: differnt languages SHOULD, due to differnt language constructs, keywords and requirements, have different (but still per language unified) styles. Whoever thinks its fine to apply a java, c or vb coding style to c# needs to be punished ;)

Regarding this quote, agreed but, what does it have to do with my rule proposal?

The one thing i realized: every time you read a differntly styled piece of code, even though it has the absolute same meaning, it is harder or easier to understand when it is far or near to your own style. Next is, that every time you change the team, the project, the company, the room, the seat, you will be confronted with a different style of code which you have to wrap your head arround. Just for the sake of making your work harder. This somewhat resides in the "problem", that every fresh developer comes into a team which got its own style, which he will get used to and in the end praise it to others because he used it for so long that he just wont accept the downsides.

True, but you cannot intend to force a style to the whole world, if that would be the case StyleCop shouldn't be configurable.

StyleCop is the only set of guidelines which is well documented and has good reasoning behind i'ts rules. You have the option to ditch a single rule, if it doesnt apply to your situation (like curlies), but that wont exclude those who still write code following that style (do not punish the curlies ;) ).

Curlies or no curlies is not "my situation", I'm trying to put order in a proyect that never had any convention or good practices, it has already 160.000+ LOC and around 10 to 13 developers working on it. I do not want to force them to always use curlies on one line if statments, I my self use them or not, as I said previously, depending on the context, but I don't want them to clump the code with al stacked one line if statements, you'd understand if you'd see the quality of the existing code...

So whenever you get into a team, you can just write code in the StyleCop "default" style, and it will be fine. Also you will read code that is very near to your own style (StyleCop style). And for reasons you can find in the rule-documentation, the code will just be objectively better overall.

This does not happen in real life, nor will ever happen, so in this real life scenario, I would like to have this rule I'm proposing, being that there are so many rules and that all are configurable, I still can't see the problem with this rule in particular, in my case, it will save me time by not having to ask people to add blank lines in the Pull Requests.

Far too long for an "overview" and i guess somewhat off topic.

If it has to do with adding the rule or not, it's not off topic...

SeidChr commented 4 years ago

Well, I've been working for the last 20 years in the industry, also in different languages, last 8 in C# mainly, but I don't get your point, would you wrap it up for me please? Or are you actually saying that it would be better off setting a fixed set of rules so everybody has to comply to them?

stylecop analyzers as implementet right now, are hinting you in your IDE towards the better alternative. It is not enforcing anything at any point in time (when you are not setting it up to do so). This is just great as it is for those who understand the reasoning behind its rules or are willing to learn it. In my first comment i've tried to make the point, that whoever disables the curly brace rule will probably just also disable the other one for the very same reasons which i have stated above.

Also: differnt languages SHOULD, due to differnt language constructs, keywords and requirements, have different (but still per language unified) styles. Whoever thinks its fine to apply a java, c or vb coding style to c# needs to be punished ;)

Regarding this quote, agreed but, what does it have to do with my rule proposal?

probably nothing. sorry. to be clear, i am not opposing your proposal. as long as it is an extension of the existing rules and not an alternative.

True, but you cannot intend to force a style to the whole world, if that would be the case StyleCop shouldn't be configurable.

Well, i would still use stylecop if it wouldnt be configurable, but i am not trying to enforce the rules nor should stylecop do so. For me and the most of me fellow collegues stylecop is an educational tool.

Curlies or no curlies is not "my situation", I'm trying to put order in a proyect that never had any convention or good practices, it has already 160.000+ LOC and around 10 to 13 developers working on it. I do not want to force them to always use curlies on one line if statments, I my self use them or not, as I said previously, depending on the context, but I don't want them to clump the code with al stacked one line if statements, you'd understand if you'd see the quality of the existing code...

I believe you. Having seen similar code and working with it every day. I'd still recommend to just import stylecop and go along with the pathfinder rule. One file after the other will be improved. Even though this is a long process, it is (imo) the correct way to handle it. You, not wanting them to clumb the code, is you, having the opinion that the curly braces are clumbsy. which is your good right to have but produces (or wont get rid of) code which is harder to maintain. If you and your team decides to disable the warning about that, thats fine. And you shall have the warning about the missing free line after the control statement as stated in your proposal.

This does not happen in real life, nor will ever happen, so in this real life scenario, I would like to have this rule I'm proposing, being that there are so many rules and that all are configurable, I still can't see the problem with this rule in particular, in my case, it will save me time by not having to ask people to add blank lines in the Pull Requests.

Well, i have seen it in work. Just not in C#. The goal is to get rid of the illusion that it cannot work. Because it can. It got nothing much to do with your proposal though. But it's the reason why i oppose having "alternative" rules in stylecop :)

Sebastriani commented 4 years ago

I'll try to make this as short as possible without quoting to not make it so long.

I am implementing StyleCop as a NuGet package and I set the violation level of the rules I want the team to comply, to Error. So the build fails if any violation occurs, this of course as well in the CI build pipeline. I did this to avoid having to loss time reviewing things in the Pull Requests that can be checked automatically. So it's not only hinting the IDE as Visual Studio "IDE" or "CS" rules do, it breaks the build. And for me that's perfect, it is what I am specifically looking for.

I never, ever in my comments said that curly braces are clumbsy, I never, ever said I want to disable them... I don't know why you interpreted it that way, probably I didn't express my self correctly since english is not my native language? I said that I don't want to force curly braces for one line if statements, but that if they write a one line if statement without curly braces, I want to force the use of a blank line after it.

I DON'T want to make an alternative rule, I want to ADD a rule.

And what I meant about not happening in real life is the fact that never ever you or no one is going to achieve that everyone follows the StyleCop rules just as they are, not even if they end up no being configurable, unless Microsoft Embed them in the compiler itself.

Please, re read my first comment, I think it was at least clear for @sharwell

SeidChr commented 4 years ago

Hey, you asked me why i dislike options in codestyle. The answer was about this, and not about your requested new rule.

Your answers appear to me as if i personally attack you in any way. This is not the case. Or at least i did not intend to do so.

Regarding breaking a build on violating sc rules. I can understand why people do this, but i never ever would work with that tool if I wouldn't have to. Especially with legacy code.

Regarding StyleCop in the compiler: i never got the point why Microsoft did not communicate sc as their suggested codestyle. Some blogs about this mention that people there are just as stubborn as developers everywhere else. So now we get a new codestyle every second year or so from some important team at ms, and everyone feels the urge to change his style to this. 🤷‍♂️

Closing this bucket of discussion now.

Dreamescaper commented 3 years ago

Guys, would that be fine if I implement this rule? Would that PR be accepted?

sharwell commented 3 years ago

@Dreamescaper sure, you can proceed. However, keep in mind that before it merges I would want to run the rule on a few repositories (this one, dotnet/roslyn, dotnet/runtime) and see how noisy it is. If it turns out to be very noisy in practice we might need to disable the rule by default.