From be5acb2b9333d2ff102bcccb13f878213ac7ebd5 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Tue, 5 Sep 2017 12:18:53 -0700 Subject: [PATCH 01/10] Update coding guidelines --- .github/CONTRIBUTING.md | 11 +- docs/dev-process/coding-guidelines.md | 174 ++++++++++++++++++-------- 2 files changed, 129 insertions(+), 56 deletions(-) diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md index 4aaef236d39..ab751826020 100644 --- a/.github/CONTRIBUTING.md +++ b/.github/CONTRIBUTING.md @@ -111,15 +111,20 @@ Additional references: ![Github-PR-dev.png](Images/Github-PR-dev.png) +* It's recommended that the lines of changes in a PR should not be too big, + because it not only stretches the review time, but also makes it much harder to spot issues. + 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```). + please reference the issue in pull request 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 diff --git a/docs/dev-process/coding-guidelines.md b/docs/dev-process/coding-guidelines.md index 74e25cbf140..90b18b5caef 100644 --- a/docs/dev-process/coding-guidelines.md +++ b/docs/dev-process/coding-guidelines.md @@ -1,92 +1,160 @@ -# 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. -A basic rule of formatting is to use "Visual Studio defaults". -Here are some general guidelines +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. -* No tabs, indent 4 spaces. -* Braces usually go on their own line, +### Naming Conventions + +* Use meaningful, descriptive words for names. + For method names, it's encouraged to use `Verb-Object` 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. + +* 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 (i.e. `const int ERROR_SUCCESS = 0`). + +* 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 unnecessary 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 and should be sorted alphabetically. + +* Fields should be specified at the top within type declarations. + +* File encoding should be `ASCII` (preferred) or `UTF8` (with `BOM`) if absolutely necessary. + +### 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()` instead of `""` whenever possible and relevant. + +* 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 -There are 3 primary preprocessor macros we define during builds: +### Commenting Conventions -* 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) +* Add comments when changes are not trivial or could be confusing. -Any other preprocessor defines found in the source are used for one-off custom builds, -typically to help debug specific scenarios. - -### Runtimes +* Add comments when a reviewer needs help to understand your changes. -The PowerShell repo is used to build PowerShell targeting CoreCLR as well as CLR 4.5. +* Update 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 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 +* 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 `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 (i.e. `Utils.Separators.Colon`). + +* Avoid creating empty arrays. + Instead, reuse the static ones via `Utils.EmptyArray`. + * Avoid unnecessary memory allocation in a loop. Move the memory allocation outside the loop if possible. -## Portable code +* 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. + +## Best Practices + +* Avoid hard-coding anything unless it's absolutely necessary. -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 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 (i.e. `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. + * 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. + +* 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). From bf82a78ad28d3621086c8c72d25f5690ce5dc0a0 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Tue, 5 Sep 2017 12:54:36 -0700 Subject: [PATCH 02/10] Add a section about security --- .github/CODEOWNERS | 2 +- docs/dev-process/coding-guidelines.md | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 026714abb23..83b30ed587f 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -10,7 +10,7 @@ # @BrucePay @JamesWTruher # Area: Security -# @TravisEz13 @PaulHigin @chunqingchen +# @TravisEz13 @PaulHigin # Area: Documentation # @joeyaiello @TravisEz13 diff --git a/docs/dev-process/coding-guidelines.md b/docs/dev-process/coding-guidelines.md index 90b18b5caef..3f45a6765dd 100644 --- a/docs/dev-process/coding-guidelines.md +++ b/docs/dev-process/coding-guidelines.md @@ -104,6 +104,18 @@ Some general guidelines: * 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. +## Security Considerations + +Security is an important aspect of PowerShell and we need to be very careful about changes that may introduce security risks. +Reviewers of a PR should be sensitive to changes that may affect security. +Some security related keywords may serve as good indicators, +such as `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 * Avoid hard-coding anything unless it's absolutely necessary. From ed3c66f90701ac8c0414adcb034739e34818d947 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Tue, 5 Sep 2017 15:23:35 -0700 Subject: [PATCH 03/10] Address comments --- .github/CONTRIBUTING.md | 8 +++++--- docs/dev-process/coding-guidelines.md | 21 ++++++++++++++------- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md index ab751826020..6860cb1a49a 100644 --- a/.github/CONTRIBUTING.md +++ b/.github/CONTRIBUTING.md @@ -122,9 +122,11 @@ Additional references: 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. ```Fix #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 diff --git a/docs/dev-process/coding-guidelines.md b/docs/dev-process/coding-guidelines.md index 3f45a6765dd..154c41a3daf 100644 --- a/docs/dev-process/coding-guidelines.md +++ b/docs/dev-process/coding-guidelines.md @@ -1,4 +1,3 @@ - # C# Coding Guidelines ## Coding Conventions @@ -45,7 +44,9 @@ We also run the [.NET code formatter tool](https://github.com/dotnet/codeformatt * Fields should be specified at the top within type declarations. -* File encoding should be `ASCII` (preferred) or `UTF8` (with `BOM`) if absolutely necessary. +* 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 @@ -70,7 +71,7 @@ We also run the [.NET code formatter tool](https://github.com/dotnet/codeformatt * Add comments when a reviewer needs help to understand your changes. -* Update existing comments when you are changing the corresponding code. +* Update/remove existing comments when you are changing the corresponding code. * Make sure the added/updated comments are accurate and easy to understand. @@ -101,6 +102,11 @@ Some general guidelines: * Avoid unnecessary memory allocation in a loop. Move the memory allocation outside the loop if possible. +* 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. + * 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. @@ -129,10 +135,11 @@ See [CODEOWNERS](../../.github/CODEOWNERS) for more information about the area e 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. - * 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. + * 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 (i.e. `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. * Use of new C# language syntax is encouraged. But avoid refactoring any existing code using new language syntax when submitting a PR From 13f680a0f473d893dc0f22f7a1db844c8c49767d Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Tue, 5 Sep 2017 15:32:44 -0700 Subject: [PATCH 04/10] Update .spelling --- .spelling | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.spelling b/.spelling index 40d7995fdd6..48fcd05155a 100644 --- a/.spelling +++ b/.spelling @@ -955,3 +955,8 @@ microsoft.com - ./tools/install-powershell.readme.md includeide sed + - docs/dev-process/coding-guidelines.md +interop +PaulHigin +SMEs +TravisEz13 From 6353286e12bea3a11a75b83833f97495e891cf4c Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Wed, 6 Sep 2017 11:23:33 -0700 Subject: [PATCH 05/10] Address more comments --- .github/CONTRIBUTING.md | 5 ++-- docs/dev-process/coding-guidelines.md | 35 +++++++++++++++++++++------ 2 files changed, 30 insertions(+), 10 deletions(-) diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md index 6860cb1a49a..a7491ea8bd9 100644 --- a/.github/CONTRIBUTING.md +++ b/.github/CONTRIBUTING.md @@ -111,8 +111,9 @@ Additional references: ![Github-PR-dev.png](Images/Github-PR-dev.png) -* It's recommended that the lines of changes in a PR should not be too big, - because it not only stretches the review time, but also makes it much harder to spot issues. +* 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). diff --git a/docs/dev-process/coding-guidelines.md b/docs/dev-process/coding-guidelines.md index 154c41a3daf..8a3af903296 100644 --- a/docs/dev-process/coding-guidelines.md +++ b/docs/dev-process/coding-guidelines.md @@ -15,7 +15,7 @@ We also run the [.NET code formatter tool](https://github.com/dotnet/codeformatt ### Naming Conventions * Use meaningful, descriptive words for names. - For method names, it's encouraged to use `Verb-Object` pair such as **`LoadModule`**. + 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_`. @@ -34,15 +34,20 @@ We also run the [.NET code formatter tool](https://github.com/dotnet/codeformatt * Avoid more than one blank empty line at any time. -* Avoid unnecessary trailing spaces at the end of a line. +* 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. * Namespace imports should be specified at the top of the file, - outside of `namespace` declarations and should be sorted alphabetically. + outside of `namespace` declarations. * Fields should be specified at the top within type declarations. + For those that serve as backing fields for properties, + they are OK to 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. @@ -67,13 +72,18 @@ We also run the [.NET code formatter tool](https://github.com/dotnet/codeformatt ### Commenting Conventions -* Add comments when changes are not trivial or could be confusing. +* Place the comment on a separate line, not at the end of a line of code. + +* Begin comment text with an uppercase letter. + It's recommended to end comment text with a period but not required. + +* Add comments where the code is not trivial or could be confusing. -* Add comments when a reviewer needs help to understand your changes. +* Add comments where a reviewer needs help to understand the code. * Update/remove existing comments when you are changing the corresponding code. -* Make sure the added/updated comments are accurate and easy to understand. +* Make sure the added/updated comments are meaningful, accurate and easy to understand. * 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. @@ -107,9 +117,17 @@ Some general guidelines: 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. -* Use `dict.TryGetValue` instead of `dict.Contains` and `dict[]` when retrieving value from a `Dictionary`. +* 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. @@ -140,6 +158,7 @@ See [CODEOWNERS](../../.github/CODEOWNERS) for more information about the area e so that the same code can be reused (i.e. `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 @@ -157,7 +176,7 @@ There are 3 primary preprocessor macros we use during builds: Any other preprocessor defines found in the source are used for one-off custom builds, typically to help debug specific scenarios. -Here are some general guidelines for writing portable code: +Here are some general guidelines for writing portable code: * 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. From f42c55d58514b67354b5c1b2cf74a829cc0d1878 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Wed, 6 Sep 2017 12:05:21 -0700 Subject: [PATCH 06/10] Address comment about the security section --- docs/dev-process/coding-guidelines.md | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/docs/dev-process/coding-guidelines.md b/docs/dev-process/coding-guidelines.md index 8a3af903296..2e8ca03c5e3 100644 --- a/docs/dev-process/coding-guidelines.md +++ b/docs/dev-process/coding-guidelines.md @@ -97,9 +97,10 @@ and we want certain inefficient code to stay out of the performance critical cod Some general guidelines: * Avoid LINQ - it can create lots of avoidable garbage. + Instead, iterate through a collection directly using `for` or `foreach` loop. -* Prefer `for` and `foreach`, - with a slight preference towards `for` when you're uncertain if `foreach` allocates an iterator. +* 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. @@ -130,10 +131,14 @@ Some general guidelines: ## Security Considerations -Security is an important aspect of PowerShell and we need to be very careful about changes that may introduce security risks. +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 `crypto`, `encryption`, `decryption`, `certificate`, `authenticate`, `ssl/tls` and `protected data`. +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. From 276255dd4e11f3a975ab61b7c3f112f33318cddb Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Wed, 6 Sep 2017 12:15:57 -0700 Subject: [PATCH 07/10] Fix 'i.e.' and 'e.g.' --- docs/dev-process/coding-guidelines.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/dev-process/coding-guidelines.md b/docs/dev-process/coding-guidelines.md index 2e8ca03c5e3..f9eaf4c3b89 100644 --- a/docs/dev-process/coding-guidelines.md +++ b/docs/dev-process/coding-guidelines.md @@ -24,7 +24,7 @@ We also run the [.NET code formatter tool](https://github.com/dotnet/codeformatt * Use `camelCase` to name non-constant local variables. * 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 (i.e. `const int ERROR_SUCCESS = 0`). + 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`). * Use `PascalCase` to name types and all other type members. @@ -105,7 +105,7 @@ Some general guidelines: * 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 (i.e. `Utils.Separators.Colon`). + 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`. @@ -154,13 +154,13 @@ See [CODEOWNERS](../../.github/CODEOWNERS) for more information about the area e * 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 (i.e. `new Example { Name = "Name", ID = 1 }`) is encouraged for better readability, +* 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 (i.e. `StringToBase64Converter.Base64ToString(string)`). + 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. From 5479132ded2d55cdf7a7c415583de5938b7bbba0 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Wed, 6 Sep 2017 12:20:37 -0700 Subject: [PATCH 08/10] Update .spelling --- .spelling | 1 + 1 file changed, 1 insertion(+) diff --git a/.spelling b/.spelling index 48fcd05155a..11edaadb281 100644 --- a/.spelling +++ b/.spelling @@ -960,3 +960,4 @@ interop PaulHigin SMEs TravisEz13 +uppercase From f5b528f2fca5b87436d2c861f17298d3b8c5c300 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Wed, 6 Sep 2017 12:43:19 -0700 Subject: [PATCH 09/10] One more tweak --- docs/dev-process/coding-guidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/dev-process/coding-guidelines.md b/docs/dev-process/coding-guidelines.md index f9eaf4c3b89..a8d1812bbc4 100644 --- a/docs/dev-process/coding-guidelines.md +++ b/docs/dev-process/coding-guidelines.md @@ -44,7 +44,7 @@ We also run the [.NET code formatter tool](https://github.com/dotnet/codeformatt * Fields should be specified at the top within type declarations. For those that serve as backing fields for properties, - they are OK to be specified next to the corresponding 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. From a69cf4d58c9903bd18219565bf723500647d4449 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Thu, 7 Sep 2017 15:03:41 -0700 Subject: [PATCH 10/10] Add motivation for using 'nameof' --- docs/dev-process/coding-guidelines.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/dev-process/coding-guidelines.md b/docs/dev-process/coding-guidelines.md index a8d1812bbc4..a6ba927d474 100644 --- a/docs/dev-process/coding-guidelines.md +++ b/docs/dev-process/coding-guidelines.md @@ -58,6 +58,7 @@ We also run the [.NET code formatter tool](https://github.com/dotnet/codeformatt * Use of `this` is neither encouraged nor discouraged. * Use `nameof()` instead of `""` 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`).