Add links on best practice guidelines in coding guideline #4983
Conversation
| as it obscures the functional changes of the PR. | ||
| A separate PR should be submitted for such refactoring without any functional changes. | ||
|
|
||
| * Avoid initializing variables, fields and auto-implemented properties to zero or null because C# guarantees its are initialized to zero. |
There was a problem hiding this comment.
I feel this one is not important to be put here. For example, some developers might always make sure all local variables are initialized before use, which is fine and won't actually cause any overhead -- IL code will have the assignment but JIT will be smart enough to not do the assignment.
Instead, I think we should mention the Interlocked type here, which I saw in Managed Threading Best Practices:
Consider using the Interlocked class instead of the lock statement to atomically change simple states. The Interlocked class provides better performance for updates that must be atomic.
There was a problem hiding this comment.
Will add Interlocked.
We should have a consensus about initializations - it seems your positions is different from the @PaulHigin position.
There was a problem hiding this comment.
I think null/zero initializations for local variables doesn't matter perf-wise. Personally, I prefer to always initialize local variables before use, but that's not something should be enforced to everyone.
For type fields, I saw some are initialized to 0, but rarely see people initialize them to null, but either way, I won't pick it in code review.
For auto properties, I never saw people initialize it to null or zero (maybe there are but rare).
Overall, I think we should just remove this item.
|
|
||
| * Avoid initializing variables, fields and auto-implemented properties to zero or null because C# guarantees its are initialized to zero. | ||
|
|
||
| * Follow Microsoft best practise guidances: |
There was a problem hiding this comment.
Let's not say follow because some of those guidelines are contradicting with our coding convention. For example, the naming guideline in the first link below says "do not use "g_" or "s_" to indicate static fields", while we require static fields to start with s_ or t_.
I think it's better to say: "Here are some useful links for your reference:"
| * [Collections](https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/guidelines-for-collections) | ||
| * [Exceptions](https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/exceptions) | ||
| * [Best Practices for Developing World-Ready Applications](https://docs.microsoft.com/en-us/dotnet/standard/globalization-localization/best-practices-for-developing-world-ready-apps) - Unicode, Culture, Encoding and Localization. | ||
| * [Best Practices for System.Net Classes](https://docs.microsoft.com/en-us/dotnet/framework/network-programming/best-practices-for-system-net-classes) |
There was a problem hiding this comment.
This one seems to focus on full .NET instead of .NET Core, so it mainly talks about things related to WebRequest and WebResponse.
There was a problem hiding this comment.
I added just for WebRequest and WebResponse.
Remove?
There was a problem hiding this comment.
WebRequest and WebResponse are mostly deprecated, and HttpClient is the way to go. So I think we can remove this link.
| * [Exceptions](https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/exceptions) | ||
| * [Best Practices for Developing World-Ready Applications](https://docs.microsoft.com/en-us/dotnet/standard/globalization-localization/best-practices-for-developing-world-ready-apps) - Unicode, Culture, Encoding and Localization. | ||
| * [Best Practices for System.Net Classes](https://docs.microsoft.com/en-us/dotnet/framework/network-programming/best-practices-for-system-net-classes) | ||
| * [Best practices for exceptions](https://docs.microsoft.com/en-us/dotnet/standard/exceptions/best-practices-for-exceptions) |
There was a problem hiding this comment.
Please use capital P for practices and capital E for exceptions to be consistent with others.
| * [Best Practices for Developing World-Ready Applications](https://docs.microsoft.com/en-us/dotnet/standard/globalization-localization/best-practices-for-developing-world-ready-apps) - Unicode, Culture, Encoding and Localization. | ||
| * [Best Practices for System.Net Classes](https://docs.microsoft.com/en-us/dotnet/framework/network-programming/best-practices-for-system-net-classes) | ||
| * [Best practices for exceptions](https://docs.microsoft.com/en-us/dotnet/standard/exceptions/best-practices-for-exceptions) | ||
| * [Best Practices for Using Strings in .NET](https://docs.microsoft.com/en-us/dotnet/standard/base-types/best-practices-strings) |
There was a problem hiding this comment.
I have been very confused about when to use InvariantCulture vs. Ordinal, and I believe there are many other developers like me :) I think this one is very useful.
| * [Best practices for exceptions](https://docs.microsoft.com/en-us/dotnet/standard/exceptions/best-practices-for-exceptions) | ||
| * [Best Practices for Using Strings in .NET](https://docs.microsoft.com/en-us/dotnet/standard/base-types/best-practices-strings) | ||
| * [Best Practices for Regular Expressions in .NET](https://docs.microsoft.com/en-us/dotnet/standard/base-types/best-practices) | ||
| * [Serialization guidelines](https://docs.microsoft.com/en-us/dotnet/standard/serialization/serialization-guidelines) |
There was a problem hiding this comment.
Please use capital G for guidelines.
| as it obscures the functional changes of the PR. | ||
| A separate PR should be submitted for such refactoring without any functional changes. | ||
|
|
||
| * Avoid initializing variables, fields and auto-implemented properties to zero or null because C# guarantees its are initialized to zero. |
There was a problem hiding this comment.
I think null/zero initializations for local variables doesn't matter perf-wise. Personally, I prefer to always initialize local variables before use, but that's not something should be enforced to everyone.
For type fields, I saw some are initialized to 0, but rarely see people initialize them to null, but either way, I won't pick it in code review.
For auto properties, I never saw people initialize it to null or zero (maybe there are but rare).
Overall, I think we should just remove this item.
| * [Collections](https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/guidelines-for-collections) | ||
| * [Exceptions](https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/exceptions) | ||
| * [Best Practices for Developing World-Ready Applications](https://docs.microsoft.com/en-us/dotnet/standard/globalization-localization/best-practices-for-developing-world-ready-apps) - Unicode, Culture, Encoding and Localization. | ||
| * [Best Practices for System.Net Classes](https://docs.microsoft.com/en-us/dotnet/framework/network-programming/best-practices-for-system-net-classes) |
There was a problem hiding this comment.
WebRequest and WebResponse are mostly deprecated, and HttpClient is the way to go. So I think we can remove this link.
2 participants
Add links on Microsoft best practice guidelines.
Add a recommendation to avoid initializing by 0 or null.