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

andrei-epure-sonarsource · 2023-11-10T18:47:26Z

No description provided.

andrei-epure-sonarsource · 2023-11-10T18:49:49Z

docs/coding-style.md


 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.


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

andrei-epure-sonarsource · 2023-11-10T18:49:53Z

docs/coding-style.md

 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?


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

andrei-epure-sonarsource · 2023-11-10T18:49:57Z

docs/coding-style.md

+
+Unit test method names:
+- Underscore in UT names separates logical groups, not individual words.
+- 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)


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

andrei-epure-sonarsource · 2023-11-10T18:50:05Z

docs/coding-style.md

+	{ "there": 42 },
+}
+```
+* 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))


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

sonarcloud · 2023-11-10T19:07:01Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

sonarcloud · 2023-11-10T19:11:57Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

Tim-Pohlmann · 2023-11-13T07:46:18Z

docs/coding-style.md

+	{ "hey" : 1 },
+	{ "there": 42 },


Wrong sytnax.

Suggested change

{ "hey" : 1 },

{ "there": 42 },

["hey"] = 1,

["there"] = 42

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 }, };

Tim-Pohlmann · 2023-11-13T07:49:18Z

docs/coding-style.md


 ## 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()`.


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()`.

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

gregory-paidis-sonarsource · 2023-11-13T17:37:56Z

docs/coding-style.md

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


What do you mean by "used characters"?

gregory-paidis-sonarsource · 2023-11-13T17:43:37Z

docs/coding-style.md

@@ -218,6 +285,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.


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 }

yes, empty lines, I will update

zsolt-kolbay-sonarsource

Overall it looks good. Most of my comments are on adding examples (even for seemingly trivial cases), and we have a couple of topics we need to vote on before we can continue.

zsolt-kolbay-sonarsource · 2024-05-27T21:31:57Z

docs/coding-style.md

+Keep it minimal and suggestive.
+- Generic words that don't convey meaning (e.g. `Helper`) should be avoided.
+- Overwordy and complex names should be avoided as well.
+- Use positive naming when possible.


Add an example:

negative: var shouldNotInclude = true;

positive: var shouldInclude = false;

zsolt-kolbay-sonarsource · 2024-05-27T21:35:09Z

docs/coding-style.md

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


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.

zsolt-kolbay-sonarsource · 2024-05-27T21:40:48Z

docs/coding-style.md

+
+Keep it minimal and suggestive.
+- Generic words that don't convey meaning (e.g. `Helper`) should be avoided.
+- Overwordy and complex names should be avoided as well.


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).

zsolt-kolbay-sonarsource · 2024-05-27T22:20:46Z

docs/coding-style.md

 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?


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 { }

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).

zsolt-kolbay-sonarsource · 2024-05-27T22:21:16Z

docs/coding-style.md

+FIXME - Avoid names without meaning like `foo`, `bar`, `baz`. OR KISS?
+
+Unit test method names:
+- Underscore in UT names separates logical groups, not individual words.


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

zsolt-kolbay-sonarsource · 2024-05-27T22:28:02Z

docs/coding-style.md

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


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());

zsolt-kolbay-sonarsource · 2024-05-28T09:12:05Z

docs/coding-style.md

+	{ "there": 42 },
+}
+```
+* 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))


Additional suggestions:

use the spread operator instead of Concat()/Append()/Prepend()

use [] instead of Array.Empty<T>() or Enumerable.Empty<T>()

zsolt-kolbay-sonarsource · 2024-05-28T12:48:25Z

docs/coding-style.md

@@ -39,6 +39,8 @@ int CallerTwo() => Leaf() + PublicMethod();
 int Leaf() =>  42;
 ```

+Do not use auto-implemented private properties. Use fields instead.


This is easy to check with an internal analyzer.

sonarcloud · 2024-05-29T10:23:40Z

Quality Gate passed for 'Sonar .NET Java Plugin'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

sonarcloud · 2024-05-29T10:26:03Z

Quality Gate passed for 'SonarAnalyzer for .NET'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

pavel-mikula-sonarsource · 2024-06-07T15:38:55Z

docs/coding-style.md

@@ -39,6 +39,8 @@ int CallerTwo() => Leaf() + PublicMethod();
 int Leaf() =>  42;


I want to do a review once it's ready for it.

github-actions bot assigned andrei-epure-sonarsource Nov 10, 2023

github-actions bot added this to In progress in Best Kanban Nov 10, 2023

andrei-epure-sonarsource commented Nov 10, 2023

View reviewed changes

Tim-Pohlmann reviewed Nov 13, 2023

View reviewed changes

gregory-paidis-sonarsource reviewed Nov 13, 2023

View reviewed changes

pavel-mikula-sonarsource moved this from In progress to To do in Best Kanban Feb 20, 2024

zsolt-kolbay-sonarsource reviewed May 28, 2024

View reviewed changes

andrei-epure-sonarsource added 2 commits May 29, 2024 12:09

Update coding style

4d686fc

move principles before the rest

57e0ac2

zsolt-kolbay-sonarsource force-pushed the andrei/coding-style branch from ddbb391 to 57e0ac2 Compare May 29, 2024 10:09

zsolt-kolbay-sonarsource assigned zsolt-kolbay-sonarsource and unassigned andrei-epure-sonarsource May 29, 2024

pavel-mikula-sonarsource reviewed Jun 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

andrei-epure-sonarsource commented Nov 10, 2023

andrei-epure-sonarsource Nov 10, 2023

andrei-epure-sonarsource Nov 10, 2023

andrei-epure-sonarsource Nov 10, 2023

andrei-epure-sonarsource Nov 10, 2023

sonarcloud bot commented Nov 10, 2023

sonarcloud bot commented Nov 10, 2023

Tim-Pohlmann Nov 13, 2023

gregory-paidis-sonarsource Nov 13, 2023

Tim-Pohlmann Nov 13, 2023 •

edited

Tim-Pohlmann Nov 13, 2023

gregory-paidis-sonarsource Nov 13, 2023

gregory-paidis-sonarsource Nov 13, 2023

andrei-epure-sonarsource Nov 13, 2023

zsolt-kolbay-sonarsource left a comment

zsolt-kolbay-sonarsource May 27, 2024

zsolt-kolbay-sonarsource May 27, 2024

zsolt-kolbay-sonarsource May 27, 2024

zsolt-kolbay-sonarsource May 27, 2024

zsolt-kolbay-sonarsource May 28, 2024 •

edited

zsolt-kolbay-sonarsource May 27, 2024

zsolt-kolbay-sonarsource May 27, 2024

zsolt-kolbay-sonarsource May 28, 2024

zsolt-kolbay-sonarsource May 28, 2024

sonarcloud bot commented May 29, 2024

sonarcloud bot commented May 29, 2024

pavel-mikula-sonarsource Jun 7, 2024

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

	- 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.
	- Overwordy and complex names should be avoided as well. e.g. `SaveAllDataToDatabase()` -> `Save()` (the rest should be understandable from the context).

		@@ -39,6 +39,8 @@ int CallerTwo() => Leaf() + PublicMethod();
		int Leaf() => 42;

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

Are you sure you want to change the base?

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

Conversation

andrei-epure-sonarsource commented Nov 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Nov 10, 2023

sonarcloud bot commented Nov 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tim-Pohlmann Nov 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zsolt-kolbay-sonarsource left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zsolt-kolbay-sonarsource May 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented May 29, 2024

Quality Gate passed for 'Sonar .NET Java Plugin'

sonarcloud bot commented May 29, 2024

Quality Gate passed for 'SonarAnalyzer for .NET'

Choose a reason for hiding this comment

Tim-Pohlmann Nov 13, 2023 •

edited

zsolt-kolbay-sonarsource May 28, 2024 •

edited