Update coding guidelines (#4754)

* Update coding guidelines to make it more concrete and useful in a review process.

* Add a section about security

* Address comments

* Update .spelling

* Address more comments

* Address comment about the security section

* Fix 'i.e.' and 'e.g.'

* Update .spelling

* One more tweak

* Add motivation for using 'nameof'

.github/CODEOWNERS

@@ -10,7 +10,7 @@
 # @BrucePay @JamesWTruher
 
 # Area: Security
-# @TravisEz13 @PaulHigin @chunqingchen
+# @TravisEz13 @PaulHigin
 
 # Area: Documentation
 # @joeyaiello @TravisEz13

.github/CONTRIBUTING.md

@@ -111,15 +111,23 @@ Additional references:
 
 ![Github-PR-dev.png](Images/Github-PR-dev.png)
 
+* It's recommended to avoid a PR with too many changes.
+  A large PR not only stretches the review time, but also makes it much harder to spot issues.
+  In such case, it's better to split the PR to multiple smaller ones.
+  For large features, try to approach it in an incremental way, so that each PR won't be too big.
 * If you're contributing in a way that changes the user or developer experience, you are expected to document those changes.
   See [Contributing to documentation related to PowerShell](#contributing-to-documentation-related-to-powershell).
 * Add a meaningful title of the PR describing what change you want to check in.
-  Don't simply put: "Fixes issue #5".
-  A better example is: "Add Ensure parameter to New-Item cmdlet", with "Fixes #5" in the PR's body.
+  Don't simply put: "Fix issue #5".
+  Also don't directly use the issue title as the PR title.
+  An issue title is to briefly describe what is wrong, while a PR title is to briefly describe what is changed.
+  A better example is: "Add Ensure parameter to New-Item cmdlet", with "Fix #5" in the PR's body.
 * When you create a pull request,
-  including a summary of what's included in your changes and
-  if the changes are related to an existing GitHub issue,
-  please reference the issue in pull request description (e.g. ```Closes #11```).
+  including a summary about your changes in the PR description.
+  The description is used to create change logs,
+  so try to have the first sentence explain the benefit to end users.
+  If the changes are related to an existing GitHub issue,
+  please reference the issue in PR description (e.g. ```Fix #11```).
   See [this][closing-via-message] for more details.
 * If the change warrants a note in the [changelog](../CHANGELOG.MD)
   either update the changelog in your pull request or

.spelling

@@ -958,3 +958,9 @@ microsoft.com
  - ./tools/install-powershell.readme.md
 includeide
 sed
+ - docs/dev-process/coding-guidelines.md
+interop
+PaulHigin
+SMEs
+TravisEz13
+uppercase

docs/dev-process/coding-guidelines.md

@@ -1,92 +1,204 @@
-
-# C# Coding Style
+# C# Coding Guidelines
 
 ## Coding Conventions
 
 As a general rule, our coding convention is to follow the style of the surrounding code.
-Avoid reformatting any code when submitting a PR as it obscures the functional changes of your change.
-We run the [.NET code formatter tool](https://github.com/dotnet/codeformatter) regularly help keep consistent formatting.
+So if a file happens to differ in style from conventions defined here
+(e.g. private members are named `m_member` rather than `_member`),
+the existing style in that file takes precedence.
+
+When making changes, you may find some existing code goes against the conventions defined here.
+In such cases, please avoid reformatting any existing code when submitting a PR as it obscures the functional changes of the PR.
+A separate PR should be submitted for style-only changes.
+We also run the [.NET code formatter tool](https://github.com/dotnet/codeformatter) regularly to keep consistent formatting.
+
+### Naming Conventions
+
+* Use meaningful, descriptive words for names.
+  For method names, it's encouraged to use `VerbObject` pair such as **`LoadModule`**.
+
+* Use `_camelCase` to name internal and private fields and use `readonly` where possible.
+  Prefix instance fields with `_`, static fields with `s_` and thread static fields with `t_`.
+  When used on static fields, `readonly` should come after `static` (i.e. `static readonly` not `readonly static`).
+
+* Use `camelCase` to name non-constant local variables.
 
-A basic rule of formatting is to use "Visual Studio defaults".
-Here are some general guidelines
+* Use `PascalCase` to name constant local variables and fields.
+  The only exception is for interop code where the constant should exactly match the name and value of the code you are calling via interop (e.g. `const int ERROR_SUCCESS = 0`).
 
-* No tabs, indent 4 spaces.
-* Braces usually go on their own line,
+* Use `PascalCase` to name types and all other type members.
+
+### Layout Conventions
+
+* Use four spaces of indentation (no tabs).
+
+* Avoid more than one blank empty line at any time.
+
+* Avoid trailing spaces at the end of a line.
+
+* Braces usually go on their own lines,
   with the exception of single line statements that are properly indented.
-* Use `_camelCase` for instance fields,
-  use `readonly` where possible.
+
+* Namespace imports should be specified at the top of the file,
+  outside of `namespace` declarations.
+
+* Fields should be specified at the top within type declarations.
+  For those that serve as backing fields for properties,
+  they should be specified next to the corresponding properties.
+
+* Preprocessor directives like `#if` and `#endif` should be placed at the beginning of a line,
+  without any leading spaces.
+
+* File encoding should be `ASCII`.
+  All `BOM` encodings should be avoided.
+  Tests that need a `BOM` encoding file should generate the file on the fly.
+
+### Member Conventions
+
 * Use of `this` is neither encouraged nor discouraged.
-* Avoid more than one blank empty line.
-* Public members should use [doc comments](https://msdn.microsoft.com/en-us/library/b2s063f7.aspx),
-  internal members may use doc comments but it is not encouraged.
+
+* Use `nameof(<member-name>)` instead of `"<member-name>"` whenever possible and relevant.
+  The motivation is to easily and more accurately find references.
+
+* Always specify the visibility, even if it's the default (i.e. `private string _foo` not `string _foo`).
+  Visibility should be the first modifier (i.e. `public abstract` not `abstract public`).
+
+* Make members private where possible.
+  Avoid declaring public members unless it's absolutely necessary.
+
 * Public members in a namespace that ends with `Internal`,
   for example `System.Management.Automation.Internal` are not considered a supported public API.
   Such members are necessarily public as implementation details in code shared between C# and PowerShell script,
   or must be available publicly by generated code.
-* File encoding should be ASCII (preferred)
-  or UTF8 (with BOM) if absolutely necessary.
 
-## Preprocessor defines
+### Commenting Conventions
 
-There are 3 primary preprocessor macros we define during builds:
+* Place the comment on a separate line, not at the end of a line of code.
 
-* DEBUG - guard code that should not be included in release builds
-* CORECLR - guard code that differs between Full CLR and CoreCLR
-* UNIX - guard code that is specific to Unix (Linux and macOS)
+* Begin comment text with an uppercase letter.
+  It's recommended to end comment text with a period but not required.
 
-Any other preprocessor defines found in the source are used for one-off custom builds,
-typically to help debug specific scenarios.
+* Add comments where the code is not trivial or could be confusing.
 
-### Runtimes
+* Add comments where a reviewer needs help to understand the code.
 
-The PowerShell repo is used to build PowerShell targeting CoreCLR as well as CLR 4.5.
+* Update/remove existing comments when you are changing the corresponding code.
 
-Code under !CORECLR must build against CLR 4.5.
-We will not accept changes that require a later version of the full CLR.
-In extremely rare cases, we may use reflection to use an API in a later version of the CLR,
-but the feature must robustly handle running with CLR 4.5.
+* Make sure the added/updated comments are meaningful, accurate and easy to understand.
 
-We may reject code under !CORECLR without explanation because
-we do not support installation or testing of such code in this repo.
-All new features should support CoreCLR.
+* Public members must use [doc comments](https://msdn.microsoft.com/en-us/library/b2s063f7.aspx).
+  Internal and private members may use doc comments but it is not required.
 
-## Performance considerations
+## Performance Considerations
 
 PowerShell has a lot of performance sensitive code as well as a lot of inefficient code.
-We have some guidelines that we typically apply widely even in less important code
-because code and patterns are copied we want certain inefficient code to stay out of the performance critical code.
+We have some guidelines that we typically apply widely even in less important code because code and patterns are copied,
+and we want certain inefficient code to stay out of the performance critical code.
 
 Some general guidelines:
 
-* Avoid LINQ - it can create lots of avoidable garbage
-* Prefer `for` and `foreach`,
-  with a slight preference towards `for` when you're uncertain if `foreach` allocates an iterator.
+* Avoid LINQ - it can create lots of avoidable garbage.
+  Instead, iterate through a collection directly using `for` or `foreach` loop.
+
+* Between `for` and `foreach`,
+  `for` is slightly preferred when you're uncertain if `foreach` allocates an iterator.
+
 * Avoid `params` arrays, prefer adding overloads with 1, 2, 3, and maybe more parameters.
+
 * Be aware of APIs such as `String.Split(params char[])` that do not provide overloads to avoid array allocation.
-  When calling such APIs, reuse a static array when possible.
+  When calling such APIs, reuse a static array when possible (e.g. `Utils.Separators.Colon`).
+
+* Avoid creating empty arrays.
+  Instead, reuse the static ones via `Utils.EmptyArray<T>`.
+
 * Avoid unnecessary memory allocation in a loop.
   Move the memory allocation outside the loop if possible.
 
-## Portable code
+* Avoid gratuitous exceptions as much as possible.
+  Exception handling can be expensive due to cache misses and page faults when accessing the handling code and data.
+  Finding and designing away exception-heavy code can result in a decent performance win.
+  For example, you should stay away from things like using exceptions for control flow.
+
+* Avoid `if (obj is Example) { example = (Example)obj }` when casting an object to a type.
+  Instead, use `var example = obj as Example` or the C# 7 syntax `if (obj is Example example) {...}` as appropriate.
+  In this way you can avoid converting to the type twice.
+
+* Use `dict.TryGetValue` instead of `dict.Contains` and `dict[..]` when retrieving value from a `Dictionary`.
+  In this way you can avoid hashing the key twice.
+
+* It's OK to use the `+` operator to concatenate one-off short strings.
+  But when dealing with strings in loops or large amounts of text,
+  use a `StringBuilder` object.
+
+## Security Considerations
+
+Security is an important aspect of PowerShell and we need to be very careful about changes that may introduce security risks,
+such as code injection caused by the lack of input validation,
+privilege escalation due to the misuse of impersonation,
+or data privacy breach with a plain text password.
+
+Reviewers of a PR should be sensitive to changes that may affect security.
+Some security related keywords may serve as good indicators,
+such as `password`, `crypto`, `encryption`, `decryption`, `certificate`, `authenticate`, `ssl/tls` and `protected data`.
+
+When facing a PR with such changes,
+the reviewers should request a designated security Subject Matter Expert (SME) to review the PR.
+Currently, @PaulHigin and @TravisEz13 are our security SMEs.
+See [CODEOWNERS](../../.github/CODEOWNERS) for more information about the area experts.
+
+## Best Practices
 
-The PowerShell code base started on Windows and depends on many Win32 APIs through P/Invoke.
-Going forward, we try to depend on CoreCLR to handle platform differences,
-so avoid adding new P/Invoke calls where a suitable alternative exists in .NET.
+* Avoid hard-coding anything unless it's absolutely necessary.
+
+* Avoid a method that is too long and complex.
+  In such case, separate it to multiple methods or even a nested class as you see fit.
+
+* Use `using` statement instead of `try/finally` if the only code in the `finally` block is to call the `Dispose` method.
+
+* Use of object initializers (e.g. `new Example { Name = "Name", ID = 1 }`) is encouraged for better readability,
+  but not required.
+
+* Stick to the `DRY` principle -- Don't Repeat Yourself.
+    * Wrap the commonly used code in methods,
+      or even put it in a utility class if that makes sense,
+      so that the same code can be reused (e.g. `StringToBase64Converter.Base64ToString(string)`).
+    * Check if the code for the same purpose already exists in the code base before inventing your own wheel.
+    * Avoid repeating literal strings in code. Instead, use `const` variable to hold the string.
+    * Resource strings used for errors or UI should be put in resource files (`.resx`) so that they can be localized later.
+
+* Use of new C# language syntax is encouraged.
+  But avoid refactoring any existing code using new language syntax when submitting a PR
+  as it obscures the functional changes of the PR.
+  A separate PR should be submitted for such refactoring without any functional changes.
+
+## Portable Code
+
+There are 3 primary preprocessor macros we use during builds:
+
+* `DEBUG` - guard code that should not be included in release builds
+* `CORECLR` - guard code that differs between Full CLR and CoreCLR
+* `UNIX` - guard code that is specific to Unix (Linux and macOS)
+
+Any other preprocessor defines found in the source are used for one-off custom builds,
+typically to help debug specific scenarios.
 
-Try to minimize the use of `#if UNIX` and `#if CORECLR`.
-When absolutely necessary, avoid duplicating more code than necessary,
-and instead prefer introducing helper functions to minimize the platform differences.
+Here are some general guidelines for writing portable code:
 
-When adding platform dependent code, prefer preprocessor directives
-over runtime checks.
+* We are in the process of cleaning up Full CLR specific code (code enclosed in `!CORECLR`),
+  so do not use `CORECLR` or `!CORECLR` in new code.
+  PowerShell Core targets .NET Core only and all new changes should support .NET Core only.
 
-We produce a single binary for all UNIX variants,
-so runtime checks are currently necessary for some platform differences, e.g. macOS and Linux.
+* The PowerShell code base started on Windows and depends on many Win32 APIs through P/Invoke.
+  Going forward, we try to depend on .NET Core to handle platform differences,
+  so avoid adding new P/Invoke calls where a suitable alternative exists in .NET Core.
 
-## Code comments
+* Try to minimize the use of `#if UNIX`.
+  When absolutely necessary, avoid duplicating more code than necessary,
+  and instead prefer introducing helper functions to minimize the platform differences.
 
-It's strongly encouraged to add comments when you are making changes to the code and tests,
-especially when the changes are not trivial or may raise confusion.
-Make sure the added comments are accurate and easy to understand.
-Good code comments would greatly improve readability of the code, and make it much more maintainable.
+* When adding platform dependent code (`Windows` vs. `UNIX`), prefer preprocessor directives over runtime checks.
+  However, runtime checks are acceptable if it would greatly improve readability
+  without causing performance concerns in performance-sensitive code.
 
+* We produce a single binary for all UNIX variants,
+  so runtime checks are currently necessary for some of them (e.g. macOS vs. Linux).