Improve 1520, use of var

[niwrA]

• changed wording and argumentation from 'use only when' to 'use always unless'
• updated the link

[dennisdoomen]

Hmm, discount is not clear enough for me. Is it a percentage? Is it an absolute value? And if so, what unit?
Same for discountedAmount. What's the unit here? 2 cows? 2 euros? A percentage?

[niwrA]

This is where context comes in. In general you would expect an entity here. If it was a percentage or an amount, that should be added to the method and the variable name.

Same for dfiscountedAmount and since amount is passed it should be clear to be the same kind of unit.

I could get into much more detail but I'm not sure this lemma is the right place or if I should write a separate blog and refer to it?

[dennisdoomen]

In general you would expect an entity here.

That would have never crossed my mind.

If it was a percentage or an amount, that should be added to the method and the variable name.

Yes, but any amount has a unit. That's why we have types. And that's why an association between classes in UML have a name and a role.

[niwrA]

My reply would be: every variable has a context. A semantic one. I'm not sure what your comment about UML refers to. What kind of association do you mean? Parent-child? Entity-to-entity? Are we still caring about ubiquitous language here? I'm expecting a domain driven design approach here, but I'm clearly missing something.

Clearly if the discount wouldn't have been returned as an entity, it would have specified a unit. But now being returned as an entity, it could offer the discount in the form of an apply function and you wouldn't need to know the unit in which the discount was stored, percentage, multiplication ... and that's actually a very important relevant detail to hide - I have seen exactly these kinds of conflicts between percentages, stored in different ways (99 or 0,99 for instance).

[dennisdoomen]

The mere fact that you have to explain all of that, reinforces my opinion that in most cases, the type is important.

My point about relationships in UML, is that in a class diagram, the end of a relationship has two important attributes: the type of the class it points to and a name that decribes the role of that type in a relationship. A variable, class field or return value are examples of such relationship and thus have a type and a role. In your example, the name of the role would be the same as the type, but I would still show this explicitly, so the reviewer doesn't have to wonder what happens there. And for the record, I like the approach you take by encapsulating discounts as an entity. I'm a big fan (and quite experienced) in DDD.

[niwrA]

Looking back at this after two years, I can easily stand by what was said previously. The functional intent is all that matters, and the type of question you have for us is exclusively a matter of naming convention.

Explicit variables being necessary is just really the exception to the rule.

And in our case, a very rare exception indeed. Rare cases where using a Task.FromResult in a test for mocking an async return value wants a specific interface for a list. In this case you could also use something else, but precisely because we use var everywhere, not using it in that location tells everyone immediately that there the type did matter.

So we'll have to agree to disagree then. But I'm willing to bet the worldwide trend will see var becoming the norm (as I think is already happening, if you look at the defaults from various rulesets). There are just too many maintainability and readability advantages.

[dennisdoomen]

See my conclusion here.

[dennisdoomen]

Here I often want to understand whether an interface is returned or a concrete class. This is not always important, but if I know there's an abstraction, I want to verify in a PR review that it's returning the abstraction.

[niwrA]

Sure, but that also depends on public or private applications and so on. A general rule could be that public methods should return objects as an interface, but at the same time when it is a Dto, (typically ending up serialized) then an interface is not necessary. In the end I would enforce interfaces where applicable at a different level, because even the interface could change without refactoring being necessary.

[dennisdoomen]

How would you enforce that?

[niwrA]

Do you mean that only coding guidelines that can be enforced are appropriate? I'm not sure I understand the question. Are you implying that specifying an interface instead of using var is a way of enforcing this?

The problem you are compensating for is that apparently you review code that uses a method, but you don't review the method itself. But isn't that the real place where this would be enforced? Any external dependency should be defined as an interface, and any object returning from that interface should itself be either an interface or a dto, as defined in the interface for the service that returns the object. That's the place to enforce something like that, just from reviewing the interface.

[dennisdoomen]

Do you mean that only coding guidelines that can be enforced are appropriate? I'm not sure I understand the question.

No, but it seems your persistence in using var blames the need for clarity on something else.

Any external dependency should be defined as an interface, and any object returning from that interface should itself be either an interface or a dto, as defined in the interface for the service that returns the object.

That is an opinion. A know a lot of folks that never use interfaces, unless they want to abstract a service (e.g. using role-based interfaces).

The problem you are compensating for is that apparently you review code that uses a method, but you don't review the method itself.

I prefer to be able to review a PR from inside Github, and then you may not be able to see the method involved. I do sometimes pull down the entire PR to my local machine, but not always because it will hold up the review. So, if something is not immediately clear from the PR, then the code is not good enough for me.

[niwrA]

Reading this back, I guess I have the advantage that the team is and tries to be consistent. But if for you a PR is always completely clear from just the changes made, that is amazing to me. I often say that if you truly want to understand a PR, you have to look at the work item specifications, and then at the tests that were written for the PR, and then take it from there. Any other method quickly boils down to very superficial reviewing.

[dennisdoomen]

It isn't as black and white as both you and me (from two years ago) said. Sometimes the PR is clear enough and contains everything I need and sometimes I need to pull down the PR and browse through the changes to see if it all makes sense.

[bkoelman]

This PR advocates to prefer var over explicit typing, which is not only a breaking change but also something I disagree with. So many times IDE support is unavailable, for example in PRs. The lack of types makes it harder to understand the code. And even when inside VS, it makes navigation harder. For example, when a method returns a list of customers, going to the underlying type for 'var' takes me to List<T> while I actually wanted to jump to the customer class (which becomes impossible).

[bkoelman]

What works well for me is to use var while writing code (which is faster), but then I reformat the file before commit. My formatting settings change var to explicit types unless the type is obvious. Resharper is very smart about this, understanding to use var on methods like CreateCustomer etc. This complies with the current rule and allows my reviewers to see types.

[niwrA]

This PR advocates to prefer var over explicit typing, which is not only a breaking change but also something I disagree with. So many times IDE support is unavailable, for example in PRs. The lack of types makes it harder to understand the code. And even when inside VS, it makes navigation harder. For example, when a method returns a list of customers, going to the underlying type for 'var' takes me to List<T> while I actually wanted to jump to the customer class (which becomes impossible).

If the lack of types makes it harder to understand the code, the semantic principle has been broken. If you want to argue that this principle is of lesser importance, then I know where our coding principles diverge to a point where we will likely never agree. But if otherwise, I would love to see you provide an example where it mattered and the semantic principle, the same one that Eric Lippens refers to, wasn't broken or significantly enhanced by explicit typing, and then of course not just the exception to the rule, but why it should be the guiding principle.

In your navigation example, navigate to the method definition and then jump to your type from there. It is not something I would sacrifice the semantic principle for.

Please, if you can, argue with code examples, and I will try to do the same.

[dennisdoomen]

I actually like the semantic aspect that @niwrA is proposing here and how this is such a big thing in DDD. I've been practicing the Ubiquitous Language for more than 10 years and think is crucial in capturing the business concepts in your code. So I propose the following compromise:

1. We rephrase AV1520 to something like "Only use var when the type is obvious or irrelevant to understand the context"
2. Introduce a new guideline to capture the semantic principle.

@bkoelman @niwrA what do you think?

[bkoelman]

I believe the guidance in this repo roughly falls into two categories: style (casing, spacing, line length, indents etc) and principles (single responsibility, dependencies, consistency etc). The first category is merely a matter of taste, where choosing something (and sticking with it) precedes over what is chosen and best automated using tools. The second category is more of a gray area, usually depends on context and where discussions result in new insights and deeper conceptual understandings.

'var' usage seems to fall in the first category, looking at Visual Studio formatting settings:

And Resharper formatting settings:

In my team we have configured style compliance as a PR validation step, making the build fail. I'm glad that we no longer have discussions on 'var', casing, line breaks or line length. At the same time, we use a reduced ruleset inside VS to avoid being distracted while writing code. A single key-press or command-line runner applies the configured styling. I'd hate to return to having discussion on 'var' again because we consider it contextual!

From time to time, proposals are done in this repo to change a recommended style, because there's always someone that is used to something else or just does not like the looks of it. The best advice to those would be to fork the repo and adapt to however they like it.

Let's not forget that C# is a general purpose language, where types are often essential to understanding the code. It is not a scripting language for a business rules engine that uses dynamic typing. For example, when I'm looking at code optimized for a hot path, I want to explicitly see usage of ReadOnlySpan<char> instead of string. When dealing with concurrency, I'd like to see usage of ConcurrentDictionary<,> or ImmutableDictionary<,> instead of IDictionary<,>. Types are often helpful, but it depends on what kind of code and the angle of the reader.

Using var everywhere makes people re-adopt practices from hungarian style. Compare:

Manager assignedTo = ...

vs:

var assignedToManager = ....

So if we were to reword this rule, I'd like it to better match existing tooling. Looks like we should use the word 'evident', whose exact meaning remains undefined and evolves over time as tooling gets smarter.

So I propose to change the title from:

Only use `var` when the type is very obvious (AV1520)

to:

Only use `var` when the type is evident (AV1520)

Aside from var usage, the discussion in the PR raises some other concerns:

• Prefer an interface over a class (covered by AV1004/AV1005)
• Emphasize purpose over mechanics (covered by AV1707/AV1712/AV1720)
• Use descriptive variable names (AV1706/AV1708)
• Prefer DSL entities over simple types (AV1140)
• Including units in variable names (would be a nice addition to AV1707)
• Did I miss any?

[niwrA]

@bkoelman I think that code analysis also defaults to suggesting using var. This is sometimes pretty annoying vs that Visual Studio defaults to explicit, but it seems that the trend is that both ReSharper and CodeAnalysis default to var.

My main issue with the whole implicit vs explicit thing, is that the wording of the rule here strongly implies that explicit is the default that can be deviated from only in very specific cases. This is not my experience - code that adheres to all the other principles will generally use var in 98% of all cases, is my experience. And in most PRs I see, explicit actively detracts from the readability of the code with superfluous long definitions, bad variable naming resulting from it, and hinders the refactorability.

Also, do you really believe that ImmutableDictionary<,> in the declaration of the variable that holds the result of the method is the place to verify that the code that returns the ImmutableDictionary<,> is the correct place to detect such errors in designing that method? I don’t think so.

@dennisdoomen I also want to point out that this PR review argument and being able to push PRs through is something that I consider a disease I want to stamp out. The result is that people review PRs and only look at superficial things like was string.Empty used instead of “”, explicit vs implicit, and not is the code readable and maintainable. Heck, most reviews should probably start with has this code been written with the proper tests and covered properly. We are checking for grammar, not understanding the sentence, and neglecting the purpose of the PR altogether.

[bkoelman]

@bkoelman I think that code analysis also defaults to suggesting using var. This is sometimes pretty annoying vs that Visual Studio defaults to explicit, but it seems that the trend is that both ReSharper and CodeAnalysis default to var.

Default configurations exist to not break existing users, not to advocate what's generally the best choice. So those defaults mean nothing to me. The fact they are configurable just indicates there's a general need for choice.

My main issue with the whole implicit vs explicit thing, is that the wording of the rule here strongly implies that explicit is the default that can be deviated from only in very specific cases. This is not my experience - code that adheres to all the other principles will generally use var in 98% of all cases, is my experience. And in most PRs I see, explicit actively detracts from the readability of the code with superfluous long definitions, bad variable naming resulting from it, and hinders the refactorability.

You've already made clear you'd like the wording reversed, which is what we disagree upon. If this needs to change in this repo, you'll need to convince @dennisdoomen on that, because he owns this repo. Pointing out unrelated problems like bad naming does make me switch to your side. Long names can be considered helpful or be considered annoying, depending on whether you value conciseness over being explicit. I value long descriptive names over abbreviations and meaningful names over including type info in the name. I've been using explicit types without hindering refactorability for years. Resharper existed long before 'var' was invented and has always provided refactoring support.

Also, do you really believe that ImmutableDictionary<,> in the declaration of the variable that holds the result of the method is the place to verify that the code that returns the ImmutableDictionary<,> is the correct place to detect such errors in designing that method? I don’t think so.

My point is that sometimes type info makes the difference. Sure there are ways to find the info I need by stepping into methods, loading up the IDE etc. But because they require additional effort to understand the code, they decrease readability.

But I think you're missing my point that var usage is a matter of style, similar to line breaks, spacing etc. So depending on what kind of code you write (simple/complex, business/technical, applications/services/libraries, functional/imperative/OO/procedural) and where you're coming from, you like one over the other, and that's okay.

[niwrA]

Visual studio’s editor is the odd one’s out and probably because of compatibility, but Resharper has changed this in the past, so it’s not just that, and Code Analysis is relatively new. And yes Resharper can do a lot of things for you, but it will still create a very cluttered pull request, if PR’s are so important ;). But that could be ok. It’s just moving problems around and saying yeah no refactoring is not a problem because some of us have tools for that.

A variable is repeated, throughout lines of code, whereas a declaration happens only once. So a good name is always more important to me, and more so than the declaration, because I should normally not have to go back to the declaration of a variable to see what type it is to understand what happens in the code.

You say it’s a matter of taste and I say it’s a matter of opinion of what constitutes good code. I don’t feel that is a matter of taste at all, but there are certainly different priorities in different contexts. However, I will still maintain that in by far the majority of contexts in C#, var is the better option.

I’m still looking forward to seeing specific examples, and especially would like to know if you guys think that considering the guidelines provided by Eric Lippens or even your own practices, var is in the minority.

I am genuinely curious - I want to always use the best possible approach and want to be able to explain why it is the best possible approach, and have data to back it up. There is an interesting research paper out there that did research across a large number of well known repositories, and could hardly detect a pattern or a preference in using explicit vs implicit. It’s chaos out there, and I’d like to see some order restored.

And I’m genuinely interested in knowing if my style of coding is so different and hard to understand versus what other people do, and if I’m missing an opportunity to further improve the readability of my code.

[dennisdoomen]

Use var when you have to; when you are using anonymous types.
Use var when the type of the declaration is obvious from the initializer, especially if it is an object creation. This eliminates redundancy.
Consider using var if the code emphasizes the semantic "business purpose" of the variable and downplays the "mechanical" details of its storage.
Use explicit types if doing so is necessary for the code to be correctly understood and maintained.
Use descriptive variable names regardless of whether you use "var". Variable names should represent the semantics of the variable, not details of its storage; "decimalRate" is bad; "interestRate" is good.

Given the discussions and many pros and cons, I'm going to run an experiment for a couple of weeks. I'm going to try to follow Eric's guidelines and see if it's causing any of loss of data.

[julianbartel]

I've been following this discussion and would like to put my 50 cents in.

Personally, I always prefer explicit typing over using var for the following reasons:
Declaring a variable with var omits valuable information: a reading developer has to infer the type, thus, it puts an additional load on the developers mind while reading the code. While that's not a big deal for the experienced developer, it reduces productivity especially for less experienced developers or when you are outside of an IDE.

On the other hand, I don't see the benefit of using var. There are cases, where var is a must (anonymous types), and there are some very few cases, where var really improves readability (an example for those cases may be complex LINQ results or multiple nested generics in general). Are there other points beside readability? Maybe refactoring: Changing a return type of a method would not impact a declaration where that return value is assigned. But such a refactoring is likely to have a semantic impact which requires some changes anyway.

All in all: I feel that the pros of preferring var are not worth the trade-off in loosing readability.

Edit: As @dennisdoomen and @niwrA suggest, I will also now develop with this discussion in mind and see if there are situations where var fits better than explicit typing and vice versa.

[bkoelman]

Eric Lipperts guidance on var was useful at the time because we had nothing, but its manual aspect has become outdated since tooling took over and automated this. Making this a case-by-case decision, still today, would be fighting against common IDEs and their editorconfig support. To me this is more important than the actual config setting chosen. Anyone agrees or disagrees on that?

[niwrA]

@julianbartel wrote:

Declaring a variable with var omits valuable information: a reading developer has to infer the type, thus, it puts an additional load on the developers mind while reading the code.

This is the core of the discussion we are having though. I maintain, and Eric Lippens suggests as much, that the omitted information isn't valuable, and instead detracts from the information that is valuable:

Eric Lipperts wrote:

Consider using var if the code emphasizes the semantic "business purpose" of the variable and downplays the "mechanical" details of its storage.

I look forward to @dennisdoomen 's findings, and could recommend anyone to do that same exercise and share their results.

However, if you really think explicit should be the default, then you might as well remove the linked article, because I think it argues pretty strongly against it!

[bkoelman]

My team did not automate because it's cool, but because it was the only way to get adoption without negative atmosphere. I've played both roles below in the past :)

...And when that happens, the inevitable bickering starts, and you start to want to bang your head against the wall that I mentioned earlier. Arguments ensue. One team member appoints himself “standards cop” and starts hassling the others from a position of self-righteousness. Another team member adopts the passive-aggressive, “too cool to care” position. It can be a mess and a headache.

Unless, of course, you make such things impossible. There’s a great piece of advice that I’ve heard before, and it applies well to people management and new technique adoption both at the same time. Build a system, and, when things go wrong, blame the system.

https://blog.ndepend.com/developers-adopt-coding-standards/

[dennisdoomen]

No need to keep fighting. Again, I think the truth will be somewhere in the middle. As I don't believe in dogmatism, I'll try myself and see where I'll end up on the scale between implicit and explicit.

And if somebody really disagrees, fork the repo and build your own internal guidelines.

[niwrA]

No need to keep fighting. Again, I think the truth will be somewhere in the middle. As I don't believe in dogmatism, I'll try myself and see where I'll end up on the scale between implicit and explicit.

And? Any preliminary results?

And if somebody really disagrees, fork the repo and build your own internal guidelines.

Yes, of course, I had already done so. But this was basically my only disagreement with the excellent existing set, so that's why I wanted to discuss it, also to learn if maybe I missed something important.

[dennisdoomen]

And? Any preliminary results?

Didn't do enough coding yet.

[dennisdoomen]

After two years of experimenting, I've settled on the fact that there's no sensible default. Sometimes var is fine and sometimes you loose information. But if we want to rely on R# or Rider to assist on this, @bkoelman has shown me that sticking with the "Explicit type unless the type is evident" guideline is the best course of action.