intellitect/codingguidelines

A repository to contain IntelliTect's tools for coding conventions

Coding Snippets Repository

Coding Snippets Extension

Coding Guidelines / Design Guidelines

A repository to contain IntelliTect's tools for coding conventions. https://intellitect.github.io/CodingGuidelines/

Guidelines Site Maintenance

There are two github actions that are used to update the CodingGuidelinesSite. One action ( Update csharp Markdown ) will run automatically when the XML file in the master branch is updated via a commit. The CodingGuidelines github page will then reflect the changes. After reviewing the "dev" site, there is another action ( Update Docs Folder on CodingGuidelinesSite ) that will move the new markdown file to production site CodingGuidelinesSite. There is also another action to manually run a xml to md conversion on any branch. There is a retired tool that was used to extract guidelines from the manuscript word documents to an XML file here: Manuscript Guidelines Extractor

codingguidelines's Issues

Create Directory.Build.props for enabling warnings as errors

See example here

INTL0002 Flags custom indexers

Customer indexers with "this" are getting flagged because they do not conform to the naming standard. We should put in an exception case to handle this.

This should not be flagged.

public int this[int index] => 0;

Set icon for the nuget package

We don't have an icon. It would look better if we had one.

Create a rule to verify that nullable is turned on at the project level.

Proposal: Favor EnumerateFile/EnumerateDirectories over GetFiles/GetDirectories

We should flag usages of GetFiles/GetDirectories and suggest using EnumerateFiles/EnumerateDirectories

Add analyzer that flags var in pattern matching

Based on internal discussion.
Pattern matching with var provides little value since it effectively matches everything and ends up copying to a local value.

Match on this

if ( bar is var foo )

Suggested code change (just to help educate people)

var foo = bar;
if ( true )

Add MS Analyzers as dependent NuGet packages

We probably want to make sure these get pulled in appropriately.

We should consider adding in the MS ones as dependencies. We need to evaluate conditional dependencies and making sure there are no conflicts (or that they are handled).

Method/Property naming rules should not apply to NativeMethods classes

Because COM and Win32 extern calls might not hew with our naming standards.

Target netstandard 2.0 with our analyzer

Can anyone think of a reason not to target netstd 2.0?

Create analyzer to detect unused local variables

An example implementation here: https://stackoverflow.com/questions/30982549/how-to-evaluate-local-variable-parameter-state-with-roslyn

The fix should support removing the variable declaration.

object foo = new object();

Fixed:

new object();

Expression Bodied method cause exception

This occurs when you have an expression bodied method.

Warning AD0001 Analyzer 'IntelliTectAnalyzer.Analyzers.UnusedLocalVariable' threw an exception of type 'System.ArgumentNullException' with message 'Value cannot be null. SecretSanta.Api C:\Dev\EWU\EWU-CSCD379-2020-Winter\SecretSanta\src\SecretSanta.Api\CSC 1 Active

INTL0302 throwing InvalidCastException

AD0001 Analyzer 'IntelliTectAnalyzer.Analyzers.FavorDirectoryEnumerationCalls' threw an exception of type 'System.InvalidCastException' with message 'Unable to cast object of type 'Microsoft.CodeAnalysis.CSharp.Syntax.MemberAccessExpressionSyntax' to type 'Microsoft.CodeAnalysis.CSharp.Syntax.IdentifierNameSyntax'.'.

Add coding standards document to project page

Right now the coding standards doc is in a private google doc. It should be moved to the project page so we can collaborate on it.

Update README for INTL0003

We have a new analyzer for forcing attributes to be on their own line. We should document this in the README alone side the other ones.

INTL0001 Allow property refactor

We should also allow for converting fields to properties as a second refactor option.

So:

public static int Foo = 42;

Convert to

public static int Foo { get; set; } = 42;

If the field is readonly convert to a readonly auto property.

public static readonly int Foo = 42;

Converts to

public static int Foo { get; } = 42;

Enable diff coverage.

https://docs.microsoft.com/en-us/azure/devops/pipelines/test/codecoverage-for-pullrequests?view=azure-devops

Add analyzer to check parameter names

Coding standard is for parameter names to be camelCase. We should add an analyzer to check it.

Document appropriate implementation of `GetHashCode`.

VS and Rider both flag warnings when you use a property in a GetHashCode implementation that is not read-only, however, I recently encountered an issue where one of the properties was virtual. This allowed the property to change in a derived class and alter the base class hash code leading to a failed dictionary lookup.

Solution: flag as warning when virtual member is used in GetHashCode

Method names should be PascalCase

We already have INTL0002 that checks property names. We should add another analyzer in the INTL00XX range that also checks method names to ensure they are PascalCase.

Remove VSIX project from repository

INTL0002 should ignore generated code

We should not processing any properties if the parent class is decorated with a GeneratedCodeAttribute

This should not get flagged

[global::System.CodeDom.Compiler.GeneratedCodeAttribute("System.Resources.Tools.StronglyTypedResourceBuilder", "16.0.0.0")]
public class Foo
{
    private int bar { get; set; }
}

The same with this:

public class Foo
{
 [global::System.CodeDom.Compiler.GeneratedCodeAttribute("System.Resources.Tools.StronglyTypedResourceBuilder", "16.0.0.0")]
    private int bar { get; set; }
}

This is effectively the same change that was made to INTL0001 in #27

INTL0001 Should not flag enum members

There is nothing wrong with the naming of this, but each of the enum members are getting flagged by the analyzer.

public enum Foo
{
    None,
    One,
    Two
}

Architecture Guidelines

Some proposals for guidelines based on things I've seen recently, and over the years:

Project documentation:

1. All projects should have a README file in their repository root that includes the following information, or contains links or instructions on where to find the following information:

   1. How to setup a local development environment for working on the project, including:
      1. Prerequisite installed software (IDEs, SDKs, Databases, other tooling, recommended IDE extensions, etc.), including version requirements.
      2. How to open the project in the recommended IDE/editor (e.g. "Open the .sln file in the repository root with Visual Studio 2019")
   2. How to build and run the project, including:
      1. How to restore any packages that aren't automatically restored. For example, Nuget packages are auto-restored by dotnet tooling, but npm packages are not.
      2. How to obtain a local copy of any required database, if necessary.
   3. How to run the project's tests
   4. How to contribute to the project's tests, including a brief summary of the different types of testing that are utilized in the application (unit testing, integration testing, UI testing, etc).
   5. How to contribute changes to the primary branch of the project, including a description of branching strategy, the project's pull request process & requirements, any review processes the project is subject to, including code reviews, architecture review, security reviews, etc.
   6. How the project is deployed, including:
      1. A description of each environment that the project may be deployed to
      2. Any URL(s) where the deployed/published project may be found
      3. A description of the process for promotion from one environment to another, including necessary approvals, manual testing/verification, and actual actions that must be taken to achieve the environment promotion (e.g. release pipelines).

2. All possible sources of application configuration that is environment-dependent MUST be clearly enumerated in a README file in the root of the project's repository, or must otherwise be linked to from such a README file. This file MUST be updated when changes are made to how an application is configured.

   1. Possible sources of configuration include, but are not limited to:
      1. Injection by an automated build or release process
      2. Stored on a cloud service, such as Azure App Service configuration (injected into the app via Environment Variables), or Azure Key Vault
      3. Stored in database table(s)
      4. Stored in files checked into the repository, including appsettings.*.json, launchSettings.json, package.json, app.config, and any other file containing configuration.
      5. Stored from predefined files/directories in environments where the application is running.
      6. Hardcoded configuration (DO NOT use hardcoded configuration, but if it truly cannot be moved elsewhere, please document it.)
   2. Possible types of configuration include, but are not limited to:
      1. Any credentials, including username/password pairs and API keys
      2. Any configuration the instructs the application how to locate an external resource, including database connection strings, URLs, IP addresses, email addresses, cloud provider subscription IDs, resource IDs, or any other external resource.

General Architecture:

1. Utilize Dependency Injection in all projects whenever possible This probably needs to be expanded upon - its maybe too open-ended and vague
2. In projects utilizing dependency injection:
   1. DO NOT store global singletons in static fields/properties. Always resolve such objects from the DI container, including services, configuration, or any other singleton.
   2. DO NOT manually instantiate classes that are registered with the DI container. This is especially applicable to Entity Framework DbContext instances, because manual instantiation implies that the DbContextOptions<> are also being manually created, are hardcoded into the DbContext constructor, or are being stored as a global singleton.
      1. Exception: Instantiating DbContext instances during setup when the instance is being configured with some an in-memory provider.
   3. AVOID using the service locator pattern, PREFER using the standard mechanisms of injection provided by your DI framework (e.g. constructor injection, ASP.NET Core parameter injection via [FromServices], etc.)

Web Application Architecture:

1. When an application needs to run long-running or recurring background jobs, you SHOULD prefer Hangfire (https://www.hangfire.io/) over homegrown out-of-process job engines, manual spawning of threads, using Task.Run, or utilizing a cloud-native solution.
   1. Exceptions may be made if:
      1. The principal purpose of the web application is to spawn, facilitate, manage, execute, or monitor such background jobs, or
      2. Running background jobs in the same process as the web application will cause significant performance, stability, or security issues.
2. DO NOT write business logic in controller classes. DO write business logic in service classes that do not have any concerns or knowledge of web requests & responses. Such classes can be directly tested with unit tests, can be easily called from other services classes to facilitate code re-use and consistency.

Add documentation for INTL0002

We should add a wiki page for INTL0002 and a short explanation for the standards.

INTL0001 Is flagging XAML variables

We should restrict it to only C# files.

Include Roslynator.Analyzers and Roslynator.CodeFixes with more strict default severity levels

Introduce https://github.com/JosefPihrt/Roslynator, which has 500+ analyzers, and also a large number of refactorings.

Lists of each:

http://pihrt.net/Roslynator/Analyzers
http://pihrt.net/Roslynator/Refactorings

Here's a very preliminary set of some recommendations for default severities. This is by no means exhaustive and is only the set that I came up with upon looking at the Info level severity issues from a single project I'm working on at the moment.

For some of these that aren't already covered, corresponding rules should be added to the C# standards.

# RCS1001: Add braces (when expression spans over multiple lines).
dotnet_diagnostic.RCS1001.severity = error

# RCS1003: Add braces to if-else (when expression spans over multiple lines).
dotnet_diagnostic.RCS1003.severity = error

# RCS1007: Add braces
dotnet_diagnostic.RCS1007.severity = error

# RCS1044: Remove original exception from throw statement
dotnet_diagnostic.RCS1044.severity = error

# RCS1123: Add parentheses according to operator precedence.
dotnet_diagnostic.RCS1123.severity = error

# RCS1037: Remove trailing white-space.
dotnet_diagnostic.RCS1037.severity = none

# RCS1036: Remove redundant empty line.
dotnet_diagnostic.RCS1036.severity = none

# RCS1170: Use read-only auto-implemented property.
dotnet_diagnostic.RCS1170.severity = warning

# RCS1213: Remove unused member declaration.
dotnet_diagnostic.RCS1213.severity = warning

Update the editor config to be explicit about our defaults

We should fill out our editor config so that it is more complete and explicit about the standards.

Docs for the defaults and some good jump off points:
https://docs.microsoft.com/en-us/visualstudio/ide/editorconfig-code-style-settings-reference?view=vs-2019
https://kent-boogaart.com/blog/editorconfig-reference-for-c-developers

INTL0001 should ignore generated code

We should not processing any fields if the parent class is decorated with a GeneratedCodeAttribute

This should not get flagged

[global::System.CodeDom.Compiler.GeneratedCodeAttribute("System.Resources.Tools.StronglyTypedResourceBuilder", "16.0.0.0")]
public class Foo
{
    private int bar;
}

The same with this:

public class Foo
{
 [global::System.CodeDom.Compiler.GeneratedCodeAttribute("System.Resources.Tools.StronglyTypedResourceBuilder", "16.0.0.0")]
    private int bar;
}

Proposal: Add Analyzer for access modifiers

I believe the coding standard should be to always specify your protection modifiers (on all members) to avoid any ambiguity.

This is apparently not something that is covered in our coding standards document.

Thoughts?

Add unit test to run analyzers on this repo

We want to ensure that the analyzers are tested on more code than just the simple unit tests. To do this we should create some functional tests that invoke the analyzers on the projects in this repo itself.

Make Microsoft.CodeAnalysis.FxCopAnalyzers a dependant NuGet package

Our analyzers are designed to augment the existing MS ones so Microsoft.CodeAnalysis.FxCopAnalyzers should be a dependant NuGet package.

Move formatting to its own number block

Currently we have overlap of the naming and formatting blocks. Move formatting to its own block

00XX Naming
01XX Formatting

So INTL0003 becomes INTL0101

"Coding Standards" or "Coding Guidelines"

Perhaps a less dogmatic term for coding practices is "coding guidelines" rather than "coding standards." Should we rename the repo accordingly or leave it as is? I ask because if we are going to change this, we should do so now.

Add analyzer to enforce attributes on their own line

From the coding standards.

DO: Place class member attribute decorations onto separate lines and encapsulated in its own square brackets.

There are two checks here.

1. Each attribute should be inside of its own square braces.
   Do: [Foo] [Bar]
   Not: [Foo, Bar]
2. Each attribute should be on its own line.
   Do:

[Foo]
[Bar]

Not:

[Foo][Bar]

The analyzer should offer fixes for both of these issues.

INTL0101 Flag Attribute if anything is on same line

public enum Foo
{
    [A]Bar
}

should be

public enum Foo
{
    [A]
    Bar
}

Also consider
[A]public void Bar() { }

Should Code Favor Single Exit or Short Circuit With Multiple Exits?

Which is preferable:

if(SomeCondition) 
{
     return 42;
}
return 77*7;

or:

string data;
if(SomeCondition)
{
   data = 42;
}
else
{
   data = 77*7;
}
return data;

Regardless, should there be a guideline or is this just a matter of opinion and context?

INTL0003 is not documented on web page

It does show up on the README.md but it is not on the web site.

The documentation should be moved to the web page

Change naming standard for fields to be _PascalCase

Current coding standard:

Prefix fields (if they are indeed needed) with underscore and then use ‘PascalCasing’ and wrap them with a property (even if the end result is private).
Exception: Unless you need to pass a member value by ref, and it cannot be a local variable. Example: Maining an internal reference count using Interlocked.
This largely removes the need for readonly fields in favor of properties.

After creating the analyzer there has been a lot of discussion on if the coding standards should be changed to have the naming for fields be underscore camel case.

Update to latest stable Nugets

Set up a .gitattributes file

...to avoid line endings kerfuffles.

Add analyzer for `async void` method signatures.

These instances are almost always going to be unintentional. They cause the calling thread to fire and forget which can lead to some subtle and difficult to track down bugs, one of which I just recently dealt with.

Solution: flag them as warnings or even errors and recommend changing to async Task

INTL0001 Should not flag constants

Currently constants are being flagged by the analyzer. Though these are fields, I believe we should relax the analyzer and not suggest the underscore prefix..

public const int Foo = 42;

Static should appear before the access modifier

I prefer static first because it stands out from the normal (which is non-static). In other words, by putting static first, it is more noticeable than when it is buried after of public`.

That said, I have seen code analysis rules suggesting to put public first - making this another issue similar to private field naming perhaps.

Flag Task.Delay(0) as being Task.CompletedTask

Task.Delay(0) is the same as Task.CompletedTask
.NET Core
.NET Framework

Test cases:

Task.Delay(0);
Task.Delay(0, token);

Suggest:
We should suggest changing to Task.CompletedTask since that is what is going to happen anyway.

Add bits to build and test in VSCode

Should run:

• dotnet build
• dotnet test

Add documentation for INTL0001

We should add a wiki page for INTL0001 and a short explanation for the standards.

Where to include message for #pragma disable?

If I am disabling a warning, what is the format of the justification message.

Here's an idea/approach but it is still ugly, better suggestions welcome:

// Justifiction: Properties initialized by Entity Framework.
#nullable disable // CS8618: Non-nullable field is uninitialized. Consider declaring as nullable.
        public ApplicationDbContext(
#nullable enable
            DbContextOptions<ApplicationDbContext>? options) : base(options) { }

// Justifiction: Properties initialized by Entity Framework.
#nullable disable // CS8618: Non-nullable field is uninitialized. Consider declaring as nullable.
        public ApplicationDbContext(
#nullable enable

or, with pragma:

// Justification: Uninitialized properties set by Entity Framework.
#pragma warning disable CS8618 // Non-nullable field is uninitialized. Consider declaring as nullable.
        public ApplicationDbContext(DbContextOptions<ApplicationDbContext> options) : base(options) { }
#pragma warning restore CS8618 // Non-nullable field is uninitialized. Consider declaring as nullable.

Create analyzer to suggest `is null` checks

Rather than checking for null with == it is better to check with is null since operators can be overloaded.

We should add an analyzer to suggest this.

if (foo == null)

After fix:

if (foo is null)

Move wiki contents to project page

After discussion with @adamskt and @MarkMichaelis we are going to deprecate the github wiki in favor of the project pages. We should move the current wiki content to the project pages and remove the wiki.

INTL0003 should flag method names with underscores

Currently the method name Create_newUser_Success is not getting flagged.

It should be flagged and the suggested name should be: Create_NewUser_Success