dennisdoomen / CSharpGuidelines

A set of coding guidelines for C# 9.0, design principles and layout rules for improving the overall quality of your code development.
https://www.csharpcodingguidelines.com
Other
746 stars 271 forks source link

Rephrased the use of var #250

Closed dennisdoomen closed 2 years ago

dennisdoomen commented 2 years ago

Replaces #1520

bkoelman commented 2 years ago

Are you really sure you want to advise against using tooling for var management? I'm asking because this results in developers having discussions in PRs about when something is "obvious", while the primary reason to adopt (a customized version of) CSharpGuidelines is to set team-wide standards in order to avoid such stylistic discussions. Because such discussions are often quite pointless and a waste of time. In general, it's more important to pick a style, rather than which style exactly.

I'm not at all looking forward to playing police officer and adding review feedback on var usage based on any subjective ruleset, it's just a waste of precious time. In my experience, adoption of styles only works in the long run when it's automated: blame the cibuild, not your peer teammates.

Modern IDEs show the inferred type in a hint anyway on their interpretation of non-obvious, which makes the chosen style even less important (does not apply for PRs of course).

Furthermore, it prevents developers from always writing var while writing new code (productivity), then reformat the file before committing the result (or face a cibuild failure).

Of course, we all have our own personal preference on when var looks good and when it does not. That doesn't justify the cost of making it a subject of discussion every day. I'd be okay joining a team that always uses var, never uses var or something in between... as long as it is supported by tooling. I would never agree to make var usage depend on subjective context and facing var discussions for the rest of the project duration.

I agree that the industry has evolved since the introduction of var in 2011 and the advice that was given in those days isn't appropriate anymore. The reality is that modern toolchains (VS, Resharper/Rider, Javascript) manage var automatically today (based on configuration), so advising to not use such modern innovations sounds like the worst advice to me.

dennisdoomen commented 2 years ago

I was expecting you to respond ;-)

But honestly, I don't care about the tooling here. Just as I explain in my masterclasses on writing maintainable code, the point is to make sure the intend of the code is clear to anybody who needs to understand what your code is doing and why. Using var or not has nothing to do with style, and everything with capturing that intend.

bkoelman commented 2 years ago

Using var or not has nothing to do with style

The industry seems to disagree with that: implicit/explicit typing is a stylistic thing (examples here, here and here).

the point is to make sure the intend of the code is clear to anybody who needs to understand what your code is doing and why

That is achieved by choosing proper variable names. If the type is important to understand the code (depending on the context, usually it is not), the name should reflect that (for example: var openGenericInterface followed by var closedConstructedType). This results in understanding the meaning of a variable where it's being used, as opposed to the need to scroll up to where it was declared.

Popular languages that rely on static typing which were recently designed (rust, swift) don't even have explicit type syntax. Type inference in compilers has evolved now. Even modern C++ and Java now have type inference. Spelling out types is becoming less and less common, which is something we (old-school devs) probably need to get used to (this is unrelated to dynamic languages such as JavaScript). Note that I'm not recommending "use var everywhere" or "never use var", just to pick something that doesn't fight against the system.

I once met someone that wanted to forbid lambda expressions and require to use anonymous functions, because then the types are visible. Following your reasoning you'd have to agree with that, right?

bkoelman commented 2 years ago

On the first point: From a compiler perspective, there's syntax (what the user typed) and semantic meaning. In that sense, var usage is stylistic because it results in the same semantic meaning. However that distinction is irrelevant here, because our audience are humans. And for humans, style does matter.

dennisdoomen commented 2 years ago

It doesn't matter. Since the original discussion started in #203, almost two years ago, I've been trying to push myself in the direction of var. But too many times I found information was missing that made it hard to judge whether the code was doing the right thing. Maybe the problem is that people suck at naming variables the right way (and is widely known as one of the biggest challenges of programming), but in the end, I need that information. And I'm convinced other people need that information too. And all in all, it's not more work or does not affect our ability to refactor the code. So, I just created a PR out of courtesy to you, but I'm going to move forward with this PR regardless

niwrA commented 2 years ago

I agree with bkoelman obviously. And our whole team switched to var for all our new code, and we haven't looked back since. The guidelines I proved in the original PR proved to be accurate and desirable, and as I mentioned in the original PR, it is also really not just a style thing. Maintainability greatly improves when the returning type is an implementation detail. I have countless examples for this already from the last two years, where we replaced a dto with a state object, or an interface, etc.

A much nicer way to deal with this would have been to collect the examples where you felt you did need to use an explicit variable, because then the guideline could have been more specific and informative on what situations would be best suitable for what instance.

So I am going to continue to disagree here and simply state that it should be "use var unless there is no other option (like if you need to force a certain interface, then it makes sense, and then var being default is useful because it is immediately clear also that you needed to force it)."

bkoelman commented 2 years ago

I agree with bkoelman obviously.

@niwrA I suspect you misunderstand my point of view here. Because I agree with @dennisdoomen that var should only be used when the type is obvious. I pointed out that I see an industry trend where type inference is becoming more common, however, C# is an old language that evolved from even older languages and that's the ecosystem we're in today. Other languages have other conventions and perhaps things have shifted even more over 10 years.

My example on openGenericInterface/closedConstructedType tried to point out that the type system provides insufficient information because in both cases that would be System.Type, which does not help to understand the purpose of these variables and how they relate to each other. Likewise, if there are multiple representations of the same information, adding type info in the variable name can be required to distinguish. For example: char token; string tokenText; LocalizedString localizedTokenText;. But these are uncommon exceptional cases.

With "fight against the system" I meant to not have a rule that goes against the ability to automate var usage with tooling (whether that be "never var", "always var" or something in-between).

bkoelman commented 2 years ago

After discussion elsewhere, the merge of this PR was reverted and is superseded by #252.