11
\$\begingroup\$

I see a lot of C#-questions recently that make use of the var keyword. For those not familiar with C#/var (I don't know if other languages have similar features):

You declare something var and the compiler (and IDE usually already as well) guesses the type you want and watches over that, so the code remains strongly typed.

var potato = functionThatReturnsAString();
//potato is a string now
Console.WriteLine(potato); //works

var cucumber = 52 * 128;
//cucumber is an int now
cucumber++; //works

var pepper = "green";
pepper++; //won't work as pepper is considererd a string.

The "great innovation" behind that is that you can rapid-fire var-typed variables and can decide upon usage what they shall be / don't need to explicitly change the type in the declaration when you change the type. Just assign an object of the new type instead and it will work.

I don't like it, but that discussion has already been had elsewhere (Stack Overflow).

However, a drawback for Code Review in particular is that it makes it occasionally hard to understand what is going on in the code. Take the first code block from this question:

foreach(var t in blenabled) in connection with the strange naming, this gives us no idea of the type in the List. In that case it is clear from the following code that it is about bool, but consider numeric types:

You see a line in the code that might cause a division by 0. If the variable in question is an integer type => exception is thrown, if it is float (etc.) => value NAN is assigned.

From most standard operations you cannot tell at all which numeric type is being used. Or think about Int16, Int32, Int64: If you cannot tell the specific type, you cannot safely identify risky casts, overflows etc., even if 99.9% of times it will be Int32.

My suggestion:

The community should enforce that the var keyword should only (if at all) be used in questions and answers if the resulting type is obvious.

var pear = myClass.someObj.Value; //NOPE!
var apple = myClass.ToString(); //OK.

That means especially when getting values from custom functions or Collections declared outside the code posted, the type should be stated. After all, it is not that hard to just type out the type ONCE per variable.

clarification
I do not wish these questions to be closed. I would like the community to be aware of this problem and include into their reviews / comments remarks how and why the usage of var decreases understandability of code or hinders good reviews.

summary so far
https://codereview.meta.stackexchange.com/a/2356/50461

\$\endgroup\$
  • 2
    \$\begingroup\$ Great discussion topic, I would like to see more of this on Meta. \$\endgroup\$ – konijn Aug 21 '14 at 17:52
  • \$\begingroup\$ May I humbly suggest that you turn your update and the latter part of this comment into an answer? I think you've thought this out more than any of the current answerers. \$\endgroup\$ – RubberDuck Aug 22 '14 at 11:21
  • \$\begingroup\$ @RubberDuck meta.codereview.stackexchange.com/a/2356/50461 \$\endgroup\$ – Mark Aug 22 '14 at 11:33
  • \$\begingroup\$ that question doesn't show enough code to tell you what blenabled is it a boolean list or is it a bit list? it should have been commented on the question for the OP to provide all the relevant code, which would include the declaration of the boolean list and how it is set. I would have asked for more code personally. \$\endgroup\$ – Malachi Aug 22 '14 at 13:25
  • \$\begingroup\$ @Malachi using switch on the elements of the List with the options true and false actually gave it away as bool (since you cannot equate whatever you want to each other, like in C). Upfront clrification would have made that a little easier indeed. \$\endgroup\$ – Mark Aug 22 '14 at 14:55
  • 2
    \$\begingroup\$ For the record, C++11 has the same feature, only it's called auto. And in functional languages, type inference is even stronger, which can make code even more confusing. \$\endgroup\$ – svick Aug 22 '14 at 15:58
  • \$\begingroup\$ I feel the comment //potato is a string now is misleading; it seems to suggest that the call to WriteLine has something to do with potato's type. The type of potato is inferred (not guessed) from the type of the initializer expression functionThatReturnsAString(). \$\endgroup\$ – mjolka Aug 28 '14 at 3:24
  • \$\begingroup\$ @mjolka true, changing \$\endgroup\$ – Mark Aug 28 '14 at 6:49
22
\$\begingroup\$

Enforcing that the ambiguous var usages are replaced with typed declarations will be very hard to do for when questions are asked.

On the other hand, it appears to me that a decent answer to a question that abuses the var declarations would be to point out that as an issue as part of the review... something like:

Your code was hard to read because you abuse the var keyword. Although the compiler may determine the correct types from the implementation details of your code, it is very hard for a human to do the same by just reading what you have shown. In general, you should use explicit type declarations whenever there is any chance of confusion. This leads to more maintainable code too.

That will earn you many +1 votes on your answer too ;-)

\$\endgroup\$
  • 2
    \$\begingroup\$ I agree with this answer. The entire point of Code Review is to learn how to write better code. The only expectation we have of askers is that the code is working... we can't rightly expect code to work AND already meet a certain quality standard... it's slippery slope toward only allowing questions that have very little if anything at all to comment on. \$\endgroup\$ – nhgrif Aug 21 '14 at 21:36
  • 1
    \$\begingroup\$ @nhgrif & rolfl: I seems it sounds as if I wanted these questions to be closed: I don't. As your comment says nhgrif: This site is here for code to be reviewed, so demanding good code in the beginning would be against this purpose. What I'd like is reviewers not just silently skipping questions or figuring them out and posting answers, without pointing out that in this context the specific usage of var hindered / bared a good review. \$\endgroup\$ – Mark Aug 22 '14 at 5:53
  • \$\begingroup\$ You're always welcome to to post these reviews yourself. \$\endgroup\$ – nhgrif Aug 22 '14 at 11:20
17
\$\begingroup\$

No, because:

If you find the overuse of the var-keyword bad, then criticise as such in your answer. (as @rolfl outlined)

There is no need to (or use in) put(ting) arbitrary constraints on askers that have no "real" benefit to their code at hand. As a sanity check you could imagine we'd enforce it the other way round:

Wherever possible, C# code must make use of the var-keyword, anything else is subject to downvotes and/or edits.

For one, editing code of questions and answers is frowned upon here. Reasons are outlined in the corresponding meta-question: Suggested edit attempts to format code

And for the next, there is no real benefit in using one over the other for most code. "It's shorter" is not a valid concern, If anything the use of var is a matter of preference.

\$\endgroup\$
12
\$\begingroup\$

The problem with var is there's conflicting information in the official documentation.

Implicitly Typed Local Varibles states:

However, the use of var does have at least the potential to make your code more difficult to understand for other developers. For that reason, the C# documentation generally uses var only when it is required.

But C# Coding Conventions states:

  • Use implicit typing for local variables when the type of the variable is obvious from the right side of the assignment, or when the precise type is not important.

    // When the type of a variable is clear from the context, use var  
    // in the declaration. 
    var var1 = "This is clearly a string.";
    var var2 = 27;
    var var3 = Convert.ToInt32(Console.ReadLine());
    
  • Do not use var when the type is not apparent from the right side of the assignment.

        // When the type of a variable is not clear from the context, use an 
        // explicit type. 
        int var4 = ExampleClass.ResultSoFar();
    
  • Do not rely on the variable name to specify the type of the variable. It might not be correct.

        // Naming the following variable inputInt is misleading.  
        // It is a string. 
        var inputInt = Console.ReadLine();
        Console.WriteLine(inputInt);
    
  • Avoid the use of var in place of dynamic.

So, the best thing to do is determine if the use of var is breaking these guidelines or causing confusion in some way and address it in the review.

\$\endgroup\$
  • \$\begingroup\$ So, this is a NO to the asked question, right? We shouldn't expect askers to following these conventions in terms of whether or not we should edit/close/etc. their question (though we might should expect them to in terms of how we post an answer to their on-topic question)? \$\endgroup\$ – nhgrif Aug 22 '14 at 0:57
  • \$\begingroup\$ @nhgrif, I don't think the question was one of on or off topic. It was asking if/what the standard for review is. I was adding context and saying that the standard is a bit ambiguous. For the record though, I agree with the rofl. \$\endgroup\$ – RubberDuck Aug 22 '14 at 1:01
  • 1
    \$\begingroup\$ @nhgrif & RubberDuck Actually I don't see ambiguity or conflict in the quotes. var exists an as such can be used. The tricky thing on this site is, that from the askers' view the second quote applies, for the reviewers however, the first is more helpful. \$\endgroup\$ – Mark Aug 22 '14 at 6:04
  • \$\begingroup\$ @Mark the ambiguity is that in one breath Microsoft says not to use it, and in another they say to use it. That makes the use of var a bit subjective and a matter of opinion. \$\endgroup\$ – RubberDuck Aug 22 '14 at 11:18
  • 4
    \$\begingroup\$ @RubberDuck to me that reads as "In the Doc we won't use it because (admittedly) it can cause confusion"(which is something you absolutely want to void in a documentation)", but feel free to use it in YOUR code with these guidelines" \$\endgroup\$ – Mark Aug 22 '14 at 11:19
10
\$\begingroup\$

Summarizing what we heard so far and combining with my thoughts and intentions:

  • These question should not be closed
  • Reviewers should however point out to the OP that their use of var in the specific case reduces code clarity. We don't want to preach to people about their coding style in general.
  • On a site where people come to have their code reviewed, we cannot tell them "that's bad code, you can't post it"
  • If code cannot be properly reviewed at all, comments should be made, asking for adjustment. That's common practice in all situations already anyways.
\$\endgroup\$
  • \$\begingroup\$ Beautiful! Sounds right to me. \$\endgroup\$ – RubberDuck Aug 22 '14 at 11:36
  • \$\begingroup\$ you should make this answer a wiki answer \$\endgroup\$ – Malachi Aug 22 '14 at 13:20
  • \$\begingroup\$ @Malachi what difference does that actually make? \$\endgroup\$ – Mark Aug 22 '14 at 14:56
  • \$\begingroup\$ it's a wiki answer and should be marked as such. \$\endgroup\$ – Malachi Aug 22 '14 at 15:09

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .