Add Coding Style to Contributing.md and editorconfig

[abergs]

Took this from the ASP.NET Core repo.

I think we could apply the same rules to our code. Also a .editorconfig would be nice.

Coding guidelines

:grey_exclamation: The content of the code that we write.

Coding style guidelines – general

The most general guideline is that we use all the VS default settings in terms of code formatting, except that we put System namespaces before other namespaces (this used to be the default in VS, but it changed in a more recent version of VS).

1. Use four spaces of indentation (no tabs)
2. Use _camelCase for private fields
3. Avoid this. unless absolutely necessary
4. Always specify member visibility, even if it's the default (i.e. private string _foo; not string _foo;)
5. Open-braces ({) go on a new line
6. Use any language features available to you (expression-bodied members, throw expressions, tuples, etc.) as long as they make for readable, manageable code.
   • This is pretty bad: public (int, string) GetData(string filter) => (Data.Status, Data.GetWithFilter(filter ?? throw new ArgumentNullException(nameof(filter))));

Usage of the var keyword

The var keyword is to be used as much as the compiler will allow. For example, these are correct:

var fruit = "Lychee";
var fruits = new List<Fruit>();
var flavor = fruit.GetFlavor();
string fruit = null; // can't use "var" because the type isn't known (though you could do (string)null, don't!)
const string expectedName = "name"; // can't use "var" with const

The following are incorrect:

string fruit = "Lychee";
List<Fruit> fruits = new List<Fruit>();
FruitFlavor flavor = fruit.GetFlavor();

Use C# type keywords in favor of .NET type names

When using a type that has a C# keyword the keyword is used in favor of the .NET type name. For example, these are correct:

public string TrimString(string s) {
    return string.IsNullOrEmpty(s)
        ? null
        : s.Trim();
}

var intTypeName = nameof(Int32); // can't use C# type keywords with nameof

The following are incorrect:

public String TrimString(String s) {
    return String.IsNullOrEmpty(s)
        ? null
        : s.Trim();
}

Cross-platform coding

Our frameworks should work on CoreCLR, which supports multiple operating systems. Don't assume we only run (and develop) on Windows. Code should be sensitive to the differences between OS's. Here are some specifics to consider.

Line breaks

Windows uses \r\n, OS X and Linux uses \n. When it is important, use Environment.NewLine instead of hard-coding the line break.

Note: this may not always be possible or necessary.

Be aware that these line-endings may cause problems in code when using @"" text blocks with line breaks.

Environment Variables

OS's use different variable names to represent similar settings. Code should consider these differences.

For example, when looking for the user's home directory, on Windows the variable is USERPROFILE but on most Linux systems it is HOME.

var homeDir = Environment.GetEnvironmentVariable("USERPROFILE") 
                  ?? Environment.GetEnvironmentVariable("HOME");

File path separators

Windows uses \ and OS X and Linux use / to separate directories. Instead of hard-coding either type of slash, use Path.Combine() or Path.DirectorySeparatorChar.

If this is not possible (such as in scripting), use a forward slash. Windows is more forgiving than Linux in this regard.

When to use internals vs. public and when to use InternalsVisibleTo

As a modern set of frameworks, usage of internal types and members is allowed, but discouraged.

InternalsVisibleTo is used only to allow a unit test to test internal types and members of its runtime assembly. We do not use InternalsVisibleTo between two runtime assemblies.

If two runtime assemblies need to share common helpers then we will use a "shared source" solution with build-time only packages. Check out the some of the projects in https://github.com/aspnet/Common/ and how they are referenced from other solutions.

If two runtime assemblies need to call each other's APIs, the APIs must be public. If we need it, it is likely that our customers need it.

Async method patterns

By default all async methods must have the Async suffix. There are some exceptional circumstances where a method name from a previous framework will be grandfathered in.

Passing cancellation tokens is done with an optional parameter with a value of default(CancellationToken), which is equivalent to CancellationToken.None (one of the few places that we use optional parameters). The main exception to this is in web scenarios where there is already an HttpContext being passed around, in which case the context has its own cancellation token that can be used when needed.

Sample async method:

public Task GetDataAsync(
    QueryParams query,
    int maxData,
    CancellationToken cancellationToken = default(CancellationToken))
{
    ...
}

Extension method patterns

The general rule is: if a regular static method would suffice, avoid extension methods.

Extension methods are often useful to create chainable method calls, for example, when constructing complex objects, or creating queries.

Internal extension methods are allowed, but bear in mind the previous guideline: ask yourself if an extension method is truly the most appropriate pattern.

The namespace of the extension method class should generally be the namespace that represents the functionality of the extension method, as opposed to the namespace of the target type. One common exception to this is that the namespace for middleware extension methods is normally always the same is the namespace of IAppBuilder.

The class name of an extension method container (also known as a "sponsor type") should generally follow the pattern of <Feature>Extensions, <Target><Feature>Extensions, or <Feature><Target>Extensions. For example:

namespace Food {
    class Fruit { ... }
}

namespace Fruit.Eating {
    class FruitExtensions { public static void Eat(this Fruit fruit); }
  OR
    class FruitEatingExtensions { public static void Eat(this Fruit fruit); }
  OR
    class EatingFruitExtensions { public static void Eat(this Fruit fruit); }
}

When writing extension methods for an interface the sponsor type name must not start with an I.

Doc comments

The person writing the code will write the doc comments. Public APIs only. No need for doc comments on non-public types.

Note: Public means callable by a customer, so it includes protected APIs. However, some public APIs might still be "for internal use only" but need to be public for technical reasons. We will still have doc comments for these APIs but they will be documented as appropriate.

[CodeTherapist]

@abergs I love it! Looks pretty similar to the .NET Core coding style. There is only one thing that I would suggest to be different.

FruitFlavor flavor = fruit.GetFlavor();

Use here not var because the returning type is not obvious when you just reading the code or when you are not familiar with the API. What do you think?

[abergs]

I understand your perspective. So only using var for primitive types?

Also; I'm open to allowing the inverted if statements in the Cert/Crypto code. I actually found it to be more readable in certain scenarios.

[CodeTherapist]

Use var when the declaring type is obvious e.g.:

var fruit = "Lychee";
var fruits = new List<Fruit>();
var metadataService = new MDSMetadata();
var otherService = OtherService.Create(...); // static factory method
var myInt = 5;
var mylong = 5L;

A return value from a function is usually hard to know when you read the code - even the compiler accepts this and infer the type. There some exception to this like MyService.IsEnabled() where you would expect a bool. Does that help?

[Shtong]

Hi, is this still a thing ? I'm looking for side projects and I'd be interested in participating in this (creating the .editorconfig and fixing the code where needed).

Some thoughts I had on the matter while reading this issue and the code:

var usage

I agree with @CodeTherapist on the fact that var should not be used when the type is not explicitly apparent. When the type is obvious I like to still avoid using var but it's more of a personal preference, and using var would also be fine.

Async method patterns: Use ConfigureAwait

When using await, the awaited task should be called with ConfigureAwait(false).

The following is incorrect

crlFile = await DownloadData(cdp);

Use this instead:

crlFile = await DownloadData(cdp).ConfigureAwait(false);

This will help performance by reducing thread switching and reduce chances of deadlocks in applications that use a SynchronizationContext.

Explicit namespace usage

Specifying types with their full namespaces should be avoided, unless required (for resolving a class name conflicts for example.

The following is incorrect:

byte[] data = System.Convert.FromBase64String(conformanceRootFile);

This version is correct:

byte[] data = Convert.FromBase64String(conformanceRootFile);

[abergs]

@Shtong This is till a thing! I absolutely welcome your contributions on this.

[Shtong]

A few other things that crossed my mind while starting to format the code:

No more than one statement per line

To improve readability, there should not be more than one statement per line.

This is incorrect:

if(condition) Action();

This is correct:

if(condition)
    Action();

Do not swallow generic exceptions

When you do not plan to rethrow an exception you caught, do not write a catch block that does not specify a specific exception type. Catch a more precise exception type, and/or use when clauses to narrow down the exceptions you want to catch.

These are incorrect:

catch(Exception ex)
{
    // ... (no rethrow)
}
catch
{
    // ... (no rethrow)
}

Do not create public fields

This follows the general good practices for C# API design. Make your fields private and make them accessible through a public property instead.

---

What are your thoughts on these?

[abergs]

I'm not sure about "Do not swallow generic exceptions". In certain scenarios you really do expect a "ON ERROR CONTINUE" ;)

But I think we should prefer specific exceptions rather than generic.

I think the largest question here is to keep the inverted if's in the cert code.

if (true == someCondition)

The popular C# way is:

if (someCondition == true)

I actually do believe those complicated flows & conditions are easier to read with the inverted style and should be allowed, but we should generally prefer the popular way.

[Shtong]

I gotta say, I'm not used to inverted ifs and it is pretty weird to read :) I didn't say anything in my previous messages because it's really a matter of personal style, but it's true that this order is very unusual in the C# world, and it will probably look weird to other people used to read C#. So, I would personally vote for a non-inverted style.

About exceptions, I agree that we may find some specific instances where swallowing generic exceptions will be justified. We can allow this in a case by case basis, and ask that there is a comment to explain why it is justified, or write a log message so we can have a hint about what's going on while debugging.