Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Explanation of use of var and variable names is generally a bad practice. #32633

Closed
DanVioletSagmiller opened this issue Nov 17, 2022 · 2 comments · Fixed by #36428
Closed

Explanation of use of var and variable names is generally a bad practice. #32633

DanVioletSagmiller opened this issue Nov 17, 2022 · 2 comments · Fixed by #36428
Assignees
Labels
doc-enhancement Improve the current content [org][type][category] dotnet-csharp/svc fundamentals/subsvc Pri1 High priority, do before Pri2 and Pri3 📌 seQUESTered Identifies that an issue has been imported into Quest.

Comments

@DanVioletSagmiller
Copy link

DanVioletSagmiller commented Nov 17, 2022

Your section on "Implicitly typed local variables" seems like a bad practice to recommend. You specifically call it out as only the right side of the assignment needs to be obvious. Then use variable names like var1 and var2. I cannot stress enough how terrible it would be to find lines of code in a function that says something like

SetupUser(var1, var2);
// instead of --
SetupUser(name, age);

While calling out that right side is important, well named variables seem far more so.

I.e. var name = "Bob Denver"; var age = 27; is very clear in its later use for what type it is and what it's purpose is.

Regarding the use of "inputInt" in the following case, I appears to resemble Hungarian Notation, literally calling out the type. I would completely agree about that being a bad practice. It establishes a practice of writing code where your variable names don't "need" to make as much sense. However, I thought this practice died out over 20 years ago. Is this something you see commonly in the field that it needs specific call out?

BUT calling out this: "Don't rely on the variable name to specify the type of the variable." seems off. I feel the name should make clear the nature of what is being held (assuming domain knowledge of the product.) If you can't tell the nature of what the variable holds by its name, why bother naming it at all? Even during my time at multiple teams in Microsoft, this was a common practice, and brought up in code reviews. I think just calling out not to use Hungarian notation or similar practices would be better than the example and text used to explain this part. Or better yet, just call out the variable names should clearly express the nature of the data it holds. I.e. var1 would be a terrible name.


Document Details

Do not edit this section. It is required for learn.microsoft.com ➟ GitHub issue linking.


Associated WorkItem - 123093

@issues-automation issues-automation bot added fundamentals/subsvc dotnet-csharp/svc Pri1 High priority, do before Pri2 and Pri3 labels Nov 17, 2022
@dotnet-bot dotnet-bot added the ⌚ Not Triaged Not triaged label Nov 17, 2022
@DanVioletSagmiller
Copy link
Author

This also relates to the use of var in
var text = "abcdef"; foreach(var c in text)

Again, if the nature of how the data is being used is not clearly expressed by the name, why name it? In any form of visual studio, you can literally just mouse over a variable to see what type VS or VSC thinks it is. But naming is far more important. Pulling c from text as a collection is obviously a character. Its use is usually where more of the complication comes from, than the initial use of var.

@BillWagner BillWagner added the doc-enhancement Improve the current content [org][type][category] label Nov 17, 2022
@dotnet-bot dotnet-bot removed the ⌚ Not Triaged Not triaged label Nov 17, 2022
@BillWagner
Copy link
Member

Those are good points @DanVioletSagmiller

I've added them to address when we next update this article.

@BillWagner BillWagner self-assigned this Jun 30, 2023
@BillWagner BillWagner added the 🗺️ reQUEST Triggers an issue to be imported into Quest. label Jun 30, 2023
@github-actions github-actions bot added 📌 seQUESTered Identifies that an issue has been imported into Quest. and removed 🗺️ reQUEST Triggers an issue to be imported into Quest. labels Jul 1, 2023
BillWagner added a commit to BillWagner/docs that referenced this issue Jul 27, 2023
Fixes dotnet#26787: clarify "obvious"
Fixes dotnet#32633: add explanation, update variable names.
Fixes dotnet#34940: Explain that `var` is preferred in LINQ queries, despite other rules.
@ghost ghost added the in-pr This issue will be closed (fixed) by an active pull request. label Jul 27, 2023
IEvangelist added a commit that referenced this issue Jul 31, 2023
* rearrange content

Make the new content organization follow better ordering.

* rewrite and update

Rewrite to match the docs team's accepted style.

* Edits to fix `var` issues

Fixes #26787: clarify "obvious"
Fixes #32633: add explanation, update variable names.
Fixes #34940: Explain that `var` is preferred in LINQ queries, despite other rules.

* Fix naming conventions

Fixes #30626: Clarify (again) that these are our guidelines, not yours. Point out that it's not the VS default, but a configuration option.
Fixes #30642: Again, our style.
Fixes #30799: Change constant style from ALL_CAPS to PascalCase to match runtime repo.
Fixes #33959: Update variable names so delegate types are PascalCased and instances of a delegate are camelCase. Add clarifying text for the same.

* Fix exception example

Fixes #31951 : Rewrite the exception example so that it's still obvious what can fail, but couldn't be easily anticipated before making the computation.

* Fixes #30897

Move the Generic type parameter naming conventions to the general naming conventions.

* Fix build warnings

* Reference runtime convention

The use of `_` and `s_` follow the runtime conventions. I'd missed that in the previous commit.

* Apply suggestions from code review

Co-authored-by: David Pine <[email protected]>

---------

Co-authored-by: David Pine <[email protected]>
@ghost ghost removed the in-pr This issue will be closed (fixed) by an active pull request. label Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-enhancement Improve the current content [org][type][category] dotnet-csharp/svc fundamentals/subsvc Pri1 High priority, do before Pri2 and Pri3 📌 seQUESTered Identifies that an issue has been imported into Quest.
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants