Coding Style: use of var

[majikandy]

Rule 10 of the coding style states:-

10. We only use var when it's obvious what the variable type is (i.e. var stream = new FileStream(...) not var stream = OpenStandardInput()).

Whilst a reasonable idea to say we only use it when it is obvious, continuously on code reviews, almost all reasonable uses of var are picked up with the request of the review to "unvar this".

This is not a good idea to wipe out a very elegant feature of the C# language.

There are definitely cases where specifying the type explicitly can make the code more readable - however it is not reasonable to "throw the baby out with the bathwater" - var often declutters code significantly and increases readability by removing this noise.

Often if a developer has chosen to use var, it is not reasonable to assume this is just laziness, as developers do care about readability.

I would prefer this rule is removed completely from the coding style rules and PRs not blocked because of it. It should be a fair argument if a developer argues back that var is clearer.

Personally, I favour var in many cases since it forces the writer to choose good variable names and good method names, which in my opinion is far more important and a rule of having to put the type there and possibly accepting poor shortened naming or bad method names simply because the type is explicit at declaration.

[noescape00]

This is not a good idea to wipe out a very elegant feature of the C# language.

It's not being wiped out, just restricted. Every feature has it's own field of use, no need to use it everywhere possible just because we can.

There are definitely cases where specifying the type explicitly can make the code more readable

I think it makes it more readable in most of the cases.

Here is a random example from google:

//Use of var is encouraged when  declaration needlessly clutters code
Widget widget1a = new Widget();  //No
var widget1b = new Widget();  //Yes, I know I am getting a widget.

//Use of var is encouraged when method name defines return type 
//or return is type is known without further need of code inspection
Widget widget2a = GetWidget(); //OK (Verbose)
var widget2b = GetWidget(); //Yes, But...
//Danger, Will Robinson!  if GetWidget returns a foo...Code review time!

//Use of var is discouraged when method offers new clue to type 
//without further code inspection
Widget widget3a = Process(); //Yes, intent is clear without any further inspection.
var widget3b = Process(); //No, What does Process do?

I afraid I will be able to find many similar cases to the last case in the example above in our codebase if we replace explicit type with var.

The code is being written once. It is then reviewed and when it's merged it's read by random devs over and over. It's better to make it easier for the colleges to read it.

[ferdeen]

From Microsoft C# coding conventions

// When the type of a variable is not clear from the context, use an explicit type.
int var4 = ExampleClass.ResultSoFar();

This SO discussion - Why Resharper uses var for everything - goes over the same points we are discussing within the team.

Personally, I favour var in many cases since it forces the writer to choose good variable names and good method names, which in my opinion is far more important and a rule of having to put the type there and possibly accepting poor shortened naming or bad method names simply because the type is explicit at declaration.

Method names should always describe the correct intent. If not, then this should be challenged during the review stage - regardless of using varor not.

I think the rule should be relaxed but in place to assist with code reviews.

[majikandy]

The code is being written once. It is then reviewed and when it's merged it's read by random devs over and over. It's better to make it easier for the colleges to read it.

100% agree with this statement, but not the solution that explicit types are easy to read as in many cases they actually make it harder to read.

[noescape00]

as in many cases they actually make it harder to read.

I think they make it easier to read the code in >=51% of cases.

[mikedennis]

I have worked on other projects that have the same rule around var that we do. It was always an area of debate. I personally prefer explicit types and think it improves readability as long as you can use var for linq queries etc where the types can be kind of gross. I think it's just a right click to change it to the type from var so if one prefers to use var as they are coding it's easy to switch it over when they are ready to commit.

[Aprogiena]

Whilst a reasonable idea to say we only use it when it is obvious, continuously on code reviews, almost all reasonable uses of var are picked up with the request of the review to "unvar this".

The problem is that the policy uses the word obvious instead of precisely defining when it can be used. Since obviousness is subjective, such a policy rule will always lead to disagreements.

Therefore I would suggest to modify the rule in a way that it is only allowed to be used if the right side contains the type of the result is the type – i.e. calling a constructor or cast or similar.

If we come with a full list where is it allowed, there will be no space for subjectivity.

As for the rule itself, in vast majority of cases the readability is greatly improved with explicit type. The readability is not improved only in case the type is completely crazy, just as Mike mentioned, in which case I believe the code should be written in a way that such a type does not even exist.

Using Var without having the explicit type on the right side is just giving you extra room for error especially for the reviewers. We cannot afford having such rooms. I cannot see any benefit for using Var unless it is really obvious from the right side.

[dangershony]

I think this is getting out of hand both cases have arguments that are valid, initially I agreed to restrict var in some cases but leave room for flexibility for the developer to decide. It ended up that every mention of var is being commented (and not allowed) in the code reviews.

If the presence of var is not clear for the code reviewer then they can debate it with the developer, some devs prefer var and some don't, its down to preference.

I am getting used to the type declaration instead of var though, but some cases I feel are useless to use a type declaration for example what is more readable?

Case A:

foreach (KeyValuePair<int, ChainedHeader> header in this.blocksByHeight)
{}

Case B:

foreach (var header in this.blocksByHeight)
{}

I also agree with mikes comment, use var and then at the end let resharper convert to types.

[Aprogiena]

I think this is getting out of hand

how so? this issue should be about discussion about opinions on the topic, and i don't see anything else, so everything seems like a good fit here. how is this getting out of hand?

[Aprogiena]

foreach (var header in this.blocksByHeight)

here i don't know what type is blocksByHeight (should be headersByHeight anyway or chainedHeadersByHeight), so then i can not know what header is. it should be ChainedHeader or Header from its name, but no, it is KeyValuePair, so actually i think this is perfect example on where NOT to use var

so the question rather is, what is better here:

foreach (KeyValuePair<int, ChainedHeader> header in this.blocksByHeight)

vs

foreach (var keyValuePairBlockHeightChainedHeader in this.blocksByHeight)

i know it looks absurd, it kind of is, but the explicit type solves it elegantly

foreach (var chainedHeaderByHeight in this.chainedHeadersByHeight)

is slightly better than original var, but it is getting pretty unreadable when you have two names and they only differ in the middle with one s being or not being there, quite confusing

so we could go with hungarians - kvpChainedHeaderByHeight, but then we don't use HU anywhere else ...

anyone else wants to spend more time on figuring out how to write this with var so that it is actually better than just

foreach (KeyValuePair<int, ChainedHeader> item in this.chainedHeadersByHeight)

[majikandy]

I would like to suggest that perhaps there is an obsession on knowing the types - when you don't always actually need to know the type so explicitly as a reader of the code.

And as we know and all agree on, code gets read many many more times that it is written, so one less thing to think about when reading it, is a good thing.

Trust the compiler knows the type and as long as the code below where it is used isn't spaghetti, it will read just fine.

Looking at the real instance of this line of code in NBitcoin probably demonstrates this question better

This is the code in our copy of NBitcoin....

foreach (KeyValuePair<int, ChainedHeader> kv in this.blocksByHeight)
{
    chain.blocksByHeight.Add(kv.Key, kv.Value);
}

Here, seeing KeyValuePair<int, ChainedHeader> distracts you and starts you thinking about the types, why the int? why the ChainedHeader? I wonder what this for loop is about to do with these types?

.... and then, you see the code inside and realise.... oh, it is just building another dictionary from this one..... what a waste of time it was for my brain to have started thinking about those types..

Therefore the following completely cleans up the unnecessary:-

foreach (var block in this.blocksByHeight)
{
    chain.blocksByHeight.Add(block.Key, block.Value);
}

I didn't have to think at all, one dictionary built from another.... do I care about the types? No. Do I trust that the compiler has me covered. Yes.

It might seem like I contrived that example..... but that is straight from code in our codebase.

[Aprogiena]

@majikandy It is funny that I actually agree with you on most of your argumentation, except that those arguments for me are in favour of explicit types :) for example, exactly as you say, just have one less thing to think about, if there is explicit type, I don't need to think about what type is and whether the following code is correct. Yes you can trust the compiler, but then there are automatic/implicit casts and suddenly magic magic, the code compiles and it is wrong.

[majikandy]

If there isn’t a test that covers this magic magic then needless to say this magic magic should not be there.

Since our codebase has lots of areas where the explicit type shows a surprising type versus the method called. I would not suggest that we swap those to var, but we choose names for those methods that make it redundant.

It is completely possible that accepting that we write the explicit type is actually a strict rule that unwittingly allows you to write confusing method return types and confusing method names.... because it doesn’t matter as the reader will see the explicit type anyway.

Even though I prefer var. I would never suggest that we should be using var for everything, just that it is completely focusing on the wrong things if we are always forced to change them into the explicit type.

Perhaps better said, I think it is contextual to the part of the code when you can use var, not tied to the code construct you use like newing up and cast.

I still propose that line is dropped from the policy completely so that developers can make up their own mind which is clearer in the context it is being used.

[Aprogiena]

I still propose that line is dropped from the policy completely so that developers can make up their own mind which is clearer in the context it is being used.

Could you please summarise the list of benefits that this proposal should have? What are your selling points?

[majikandy]

Summary

• There are bigger fish to fry than whether to use var or not and it shouldn't be a blanket rule either way.
• Developers able to make their own decision between var and explicit type in order for the code to be more readable.
• Opportunity at review to challenge this either way without it blocking a merge, and without having to refer to the policy as the reason - since the policy does not have the context in which that piece of code is being used.

[dangershony]

In case of kvp, which I find annoying to read every time I would suggest the word Item.

foreach (var item in this.chainedHeadersByHeight)

vs

foreach (KeyValuePair<int, ChainedHeader> item in this.chainedHeadersByHeight)

The top one is way more readable (IMO) and its clear when you read down the code what item is. (I am also happy to go with kvp)

But in any case a user has to read the start of the loop where the dictionary iteration is present so specifying KeyValuePair is not needed.

This is one case, there may be others thats why var is a flexible policy.

[Aprogiena]

Same as here https://github.com/stratisproject/StratisBitcoinFullNode/issues/1400#issuecomment-388397660, It does not seem that the proposal has overwhelming majority, so we are probably unable to reach a consensus on a change, therefore we should not continue in the discussion during work hours.

[Mike-E-angelo]

I wanted to add my appreciation and support for the dialogue that has taken place here. I for one am "ALLTHEVAR", and am of the mindset that the less amount of characters (busyness) on the screen, the better. If I want to know the type I leverage tooling/tooltips, etc.

HOWEVER, I realize that is simply my perspective and the team is bigger than that. I could get into a system where a final format upon commit puts the code into a format that conforms to what the organization expects. It would seem that ReSharper would do this already for you. 😄

[dangershony]

@Mike-EEE would you create a project resharper file for that?

[Mike-E-angelo]

Good question @dangershony ... you mean by like a commit policy or some such? I use ReSharper to format my code before committing, of course. There is also an extension that easily formats all changed code for you as well: https://github.com/Zvirja/ReSharperHelpers

As far as performing this on commit, it does seem like there would be something that does this for you, but unfortunately, a quick search does not return anything meaningful. :(

[noescape00]

Deprecated. Reopen if needed.