Add links on best practice guidelines in coding guideline by iSazonov · Pull Request #4983 · PowerShell/PowerShell

iSazonov · 2017-10-03T09:43:53Z

Add links on Microsoft best practice guidelines.
Add a recommendation to avoid initializing by 0 or null.

daxian-dbw · 2017-10-04T05:16:16Z

docs/dev-process/coding-guidelines.md

  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.


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.

Will add Interlocked.

We should have a consensus about initializations - it seems your positions is different from the @PaulHigin position.

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.

daxian-dbw · 2017-10-04T05:23:00Z

docs/dev-process/coding-guidelines.md


+* 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:


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:"

daxian-dbw · 2017-10-04T05:26:42Z

docs/dev-process/coding-guidelines.md

+    * [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)


This one seems to focus on full .NET instead of .NET Core, so it mainly talks about things related to WebRequest and WebResponse.

I added just for WebRequest and WebResponse.
Remove?

WebRequest and WebResponse are mostly deprecated, and HttpClient is the way to go. So I think we can remove this link.

daxian-dbw · 2017-10-04T05:27:55Z

docs/dev-process/coding-guidelines.md

+    * [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)


Please use capital P for practices and capital E for exceptions to be consistent with others.

daxian-dbw · 2017-10-04T05:30:24Z

docs/dev-process/coding-guidelines.md

+  * [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)


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.

daxian-dbw · 2017-10-04T05:32:13Z

docs/dev-process/coding-guidelines.md

+  * [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)


Please use capital G for guidelines.

daxian-dbw · 2017-10-04T16:24:57Z

docs/dev-process/coding-guidelines.md

  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.


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.

daxian-dbw · 2017-10-04T16:25:08Z

docs/dev-process/coding-guidelines.md

+    * [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)


WebRequest and WebResponse are mostly deprecated, and HttpClient is the way to go. So I think we can remove this link.

Add links on best practice guidelines in coding guideline

31882c6

iSazonov assigned daxian-dbw Oct 3, 2017

iSazonov requested a review from daxian-dbw October 3, 2017 09:43

Fix spelling

71a2c7f

daxian-dbw requested changes Oct 4, 2017

View reviewed changes

Address feedback

b900581

daxian-dbw requested changes Oct 4, 2017

View reviewed changes

iSazonov and others added 2 commits October 5, 2017 08:28

Remove unapproved additions

7365525

Remove 'System.NET' as it's been removed from doc

e5e7a99

daxian-dbw approved these changes Oct 5, 2017

View reviewed changes

daxian-dbw merged commit ec1106a into PowerShell:master Oct 5, 2017

iSazonov deleted the coding-guidence branch October 6, 2017 03:45

iSazonov mentioned this pull request May 3, 2018

Quote Multipart/Form-Data Field Names #6782

Merged

11 tasks


		* 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:

Conversation

iSazonov commented Oct 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

daxian-dbw Oct 4, 2017 •

edited

Loading

daxian-dbw Oct 4, 2017 •

edited

Loading

daxian-dbw Oct 4, 2017 •

edited

Loading

daxian-dbw Oct 4, 2017 •

edited

Loading