Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[DON'T MERGE] Review additional coding style suggestions #8347

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

[DON'T MERGE] Review additional coding style suggestions #8347

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4d686fc
Update coding style
andrei-epure-sonarsource Nov 10, 2023
57e0ac2
move principles before the rest
andrei-epure-sonarsource Nov 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
78 changes: 77 additions & 1 deletion docs/coding-style.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ int CallerTwo() => Leaf() + PublicMethod();
int Leaf() => 42;
```

Do not use auto-implemented private properties. Use fields instead.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is easy to check with an internal analyzer.


### Local functions

There are no strict rules on when to use local functions. It should be decided on a case-by-case basis.
Expand Down Expand Up @@ -97,18 +99,42 @@ public void MethodB()

## Naming conventions

Generic words in class names that don't convey meaning (e.g. `Helper`) should be avoided. Overwordy and complex names should be avoided as well.
### Principles

Keep it minimal and suggestive.
- Generic words that don't convey meaning (e.g. `Helper`) should be avoided.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Generic words that don't convey meaning (e.g. `Helper`) should be avoided.
- Generic words that don't convey meaning (e.g. `Helper`, `Manager`, `Data`) should be avoided.

- Overwordy and complex names should be avoided as well.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Overwordy and complex names should be avoided as well.
- Overwordy and complex names should be avoided as well. e.g. `SaveAllDataToDatabase()` -> `Save()` (the rest should be understandable from the context).

- Use positive naming when possible.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add an example:

  • negative: var shouldNotInclude = true;
  • positive: var shouldInclude = false;


### Casing

Protected fields should start with lowercase letter.

### Parameters and variables

Single variable lambdas should use `x` as the variable name (based on lambda calculus λx). Multi variable lambdas should use descriptive names, where `x` can be used for the main iterated item like `(x, index) => ...`. Name `c` can be used for context of Roslyn callback.

Short names can be used as parameter and variable names, namely `SyntaxTree tree`, `SemanticModel model`, `SyntaxNode node` and `CancellationToken cancel`.

### Method names

FIXME Avoid Get prefixes for method names. Save three characters when it only gets x.Foo.Bar.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to vote about this one in the 4th step of our process.


### Unit tests

Unit tests for common C# and VB.NET rules should use two aliases `using CS = SonarAnalyzer.Rules.CSharp` and `using VB = SonarAnalyzer.Rules.VisualBasic`. Test method names should have `_CS` and `_VB` suffixes.

Unit tests for single language rule should not use alias nor language method suffix.

Variable name `sut` (System Under Test) is recommended in unit tests that really tests a single unit (contrary to our usual rule integration unit tests).

FIXME - Avoid names without meaning like `foo`, `bar`, `baz`. OR KISS?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to vote about this one in the 4th step of our process.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to avoid these names then let's add some examples for what to use instead. e.g. for an analyzer that raises on methods with empty bodies:

public void Foo() // Noncompliant
{ 
} 

public void Bar() // Compliant
{
   // Some explanation
}

public void Baz()  // Compliant
{
   Console.WriteLine();
}

public override void Kiss() // Compliant
{
}

Instead use names that show how the given member is relevant to the analyzer that's being tested:

public void Empty() // Noncompliant
{ 
} 

public override void HasComment()  // Compliant
{
   // Some explanation
}

public void NotEmpty()  // Compliant
{
   Console.WriteLine();
}

public override void Overriden() // Compliant
{
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we agree not to use these names then let's add an internal analyzer that enforces it (or make it part of the Verifier).


Unit test method names:
- Underscore in UT names separates logical groups, not individual words.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add an example (after we voted on the convention).

- FIXME: what should the name pattern be? NEEDS DISCUSSION ([many patterns](https://dzone.com/articles/7-popular-unit-test-naming) and also [Microsoft convention](https://learn.microsoft.com/en-us/dotnet/core/testing/unit-testing-best-practices#naming-your-tests) - I'd go for MS convention)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to vote about this one in the 4th step of our process.



## Multi-line statements

* Operators (`&&`, `||`, `and`, `or`, `+`, `:`, `?`, `??` and others) are placed at the beginning of a line.
Expand Down Expand Up @@ -179,6 +205,14 @@ Variable name `sut` (System Under Test) is recommended in unit tests that really

## Code structure

### Principles

* When to factorize: two is a group, three is a crowd.
* Less is more.
* Rely on Roslyn Type inference to reduce used characters.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by "used characters"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add an example for:

  • the var keyword: string name = "Joe"; -> var name = "Joe";
  • target-typed expressions for fields: private CustomType type = new CustomType(); =>private CustomType type = new();
  • target-typed expressions for parameters: Method(new CustomType()); => Method(new());


### Style

* Field and property initializations are done directly in the member declaration instead of in a constructor.
* `if`/`else if` and explicit `else` is used
* when it helps to understand overall structure of the method,
Expand All @@ -193,13 +227,46 @@ Variable name `sut` (System Under Test) is recommended in unit tests that really
* Var pattern `is var o` can be used only where variable declarations would require additional nesting.
* Var pattern `o is { P: var p }` can be used only where `o` can be `null` and `p` is used at least 3 times.
* Do not use `nullable`.
* Avoid single-use variables, unless they really bring readability value.
* Use file-scoped namespaces.
* Tested variable is on the left, like `iterated == searchParameter` (where `iterated` is the tested variable)
* If a string is multiline, use raw string literals, indented one tab-in from the declaration:
```
var raw = """
hello
my friend
""";
```
* Always use multi-line initializers for collection and objects, e.g.:
```
var thingy = new Thingy
{
x = "hello",
y = 42,
}

var collection = new Dictionary<string, int>
{
{ "hey" : 1 },
{ "there": 42 },
Comment on lines +250 to +251
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong sytnax.

Suggested change
{ "hey" : 1 },
{ "there": 42 },
["hey"] = 1,
["there"] = 42

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's my typo, sorry for that Andrei.
And Tim, the one I wanted to suggest is this one:

        var dict = new Dictionary<int,int>
        {
            { 1, 2 },
            { 3, 4 },
        };

}
```
* FIXME - align on how to use collection initializers int[] x = [ 1, 2, 3 ] or old style (see [slack discussion](https://sonarsource.slack.com/archives/C01H2B58DE1/p1697103918957899?thread_ts=1696951023.295859&cid=C01H2B58DE1))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to vote about this one in the 4th step of our process.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional suggestions:

  • use the spread operator instead of Concat()/Append()/Prepend()
  • use [] instead of Array.Empty<T>() or Enumerable.Empty<T>()


### Unit Tests
* VerifierBuilder.AddSnippet should not be used to assert compliant/noncomplaint test cases. Move it to a TestCases file.


## Comments

* Code should contain as few comments as necessary in favor of well-named members and variables.
* Comments should generally be on separate lines.
* Comments on the same line with code are acceptable for short lines of code and short comments.
* Documentation comments for abstract methods and their implementations should be placed only on the abstract method, to avoid duplication. _When reading the implementation, the IDE offers the tooling to peek in the base class and read the method comment._
* Avoid using comments for "Arrange, Act, Assert" in UTs, unless the test is complex.
* Use single-line comments. Exception: `Internal /* for testing */ void Something()`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naming here is confusing since this is a single line.

Suggested change
* Use single-line comments. Exception: `Internal /* for testing */ void Something()`.
* Use double-slash comments only. Exception: `Internal /* for testing */ void Something()`.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe rather "Do not use /* ... */."

* Prefer well-named members instead of documentation.
* When writing [xmldoc](https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/xmldoc/recommended-tags) for methods, avoid adding superflous tags (e.g. members that have self-explanatory names).

## FIXME and ToDo

Expand All @@ -217,6 +284,15 @@ It can still be used when and where it makes sense. For instance, when a class h
implementing generic interfaces (such as `IComparable`, `IDisposable`), it can make sense to have regions
for the implementation of these interfaces.

## Spacing

* Avoid spaces unless they bring clarity and help the reader understand logical groups. Prefer spaces over comments.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By spaces you mean newlines?
It's a bit confusing, someone might read this as using new int[]{1,2,3} instead of new int[] { 1, 2, 3 }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, empty lines, I will update


## Type definition

* If a class is a POCO data-container, use a record.
* Do not use primary constructors on normal classes and structs, use standard constructor + field/properties.

## ValueTuples

Do not use `ValueTuples` in production code. The usage in test projects is fine. `ValueTuples` are not supported in MsBuild 14 and while MsBuild 14 is not officially supported anymore, we still don't want to break it, if we can avoid it.
Expand Down