Skip to content

Commit

Permalink
Modify S3353: Migrate To LayC (#3302)
Browse files Browse the repository at this point in the history
## Review

A dedicated reviewer checked the rule description successfully for:

- [ ] logical errors and incorrect information
- [ ] information gaps and missing content
- [ ] text style and tone
- [ ] PR summary and labels follow [the
guidelines](https://github.com/SonarSource/rspec/#to-modify-an-existing-rule)

---------

Co-authored-by: Zsolt Kolbay <[email protected]>
  • Loading branch information
1 parent cd7c6fc commit 65743cb
Show file tree
Hide file tree
Showing 7 changed files with 76 additions and 40 deletions.
1 change: 0 additions & 1 deletion rules/S3353/csharp/metadata.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
{
"title": "Unchanged local variables should be \"const\"",
"quickfix": "partial",
"tags": [
"performance"
Expand Down
50 changes: 30 additions & 20 deletions rules/S3353/csharp/rule.adoc
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
== Why is this an issue?
include::../why.adoc[]

Marking a variable that is unchanged after initialization ``++const++`` is an indication to future maintainers that "no this isn't updated, and it's not supposed to be". ``++const++`` should be used in these situations in the interests of code clarity.
include::../how.adoc[]

=== Noncompliant code example
=== Code examples

[source,csharp]
==== Noncompliant code example

[source,csharp,diff-id=1,diff-type=noncompliant]
----
public bool Seek(int[] input)
{
Expand All @@ -19,22 +21,10 @@ public bool Seek(int[] input)
return false;
}
----
or

[source,csharp]
----
public class Sample
{
public void Method()
{
var context = $"{nameof(Sample)}.{nameof(Method)}"; // Noncompliant (C# 10 and above only)
}
}
----
==== Compliant solution

=== Compliant solution

[source,csharp]
[source,csharp,diff-id=1,diff-type=compliant]
----
public bool Seek(int[] input)
{
Expand All @@ -49,9 +39,23 @@ public bool Seek(int[] input)
return false;
}
----
or

[source,csharp]
==== Noncompliant code example

[source,csharp,diff-id=2,diff-type=noncompliant]
----
public class Sample
{
public void Method()
{
var context = $"{nameof(Sample)}.{nameof(Method)}"; // Noncompliant (C# 10 and above only)
}
}
----

==== Compliant solution

[source,csharp,diff-id=2,diff-type=compliant]
----
public class Sample
{
Expand All @@ -62,6 +66,12 @@ public class Sample
}
----

== Resources

=== Documentation

* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/const[const]

ifdef::env-github,rspecator-view[]

'''
Expand Down
3 changes: 3 additions & 0 deletions rules/S3353/how.adoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
== How to fix it

Mark the given variable with the `const` modifier.
1 change: 0 additions & 1 deletion rules/S3353/javascript/metadata.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
{
"title": "Unchanged variables should be marked \"const\"",
"tags": [
"es2015"
],
Expand Down
52 changes: 35 additions & 17 deletions rules/S3353/javascript/rule.adoc
Original file line number Diff line number Diff line change
@@ -1,47 +1,65 @@
== Why is this an issue?
include::../why.adoc[]

Marking a variable that is unchanged after initialization ``++const++`` is an indication to future maintainers that "no this isn't updated, and it's not supposed to be". ``++const++`` should be used in these situations in the interests of code clarity.
include::../how.adoc[]

=== Noncompliant code example
=== Code examples

[source,javascript]
==== Noncompliant code example

[source,javascript,diff-id=1,diff-type=noncompliant]
----
function seek(input) {
let target = 32; // Noncompliant
for (let i of input) { // Noncompliant
if (i == target) {
for (const i of input) {
if (i === target) {
return true;
}
}
return false;
}
function getUrl(query) {    
let url; // Noncompliant
url = "http://example.com";
return url;
}
----

=== Compliant solution
==== Compliant solution

[source,javascript]
[source,javascript,diff-id=1,diff-type=compliant]
----
function seek(input) {
const target = 32;
for (const i of input) {
if (i == target) {
if (i === target) {
return true;
}
}
return false;
}
----

function getUrl(query) {  
const url = "http://example.com";
[source,javascript,diff-id=2,diff-type=noncompliant]
----
function getUrl(protocol, domain, path) {    
let url; // Noncompliant
url = `${protocol}/${domain}/${path}`;
return url;
}
----

==== Compliant solution

[source,javascript,diff-id=2,diff-type=compliant]
----
function getUrl(protocol, domain, path) {  
const url = `${protocol}/${domain}/${path}`;
return url;
}
----

== Resources

=== Documentation

* MDN - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/const[const]

ifdef::env-github,rspecator-view[]

'''
Expand Down
2 changes: 1 addition & 1 deletion rules/S3353/metadata.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"title": "Unchanged local variables should be \"final\"",
"title": "Unchanged variables should be marked as \"const\"",
"type": "CODE_SMELL",
"code": {
"impacts": {
Expand Down
7 changes: 7 additions & 0 deletions rules/S3353/why.adoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
== Why is this an issue?

If a variable that is not supposed to change is not marked as `const`, it could be accidentally reassigned elsewhere in the code, leading to unexpected behavior and bugs that can be hard to track down.

By declaring a variable as `const`, you ensure that its value remains constant throughout the code. It also signals to other developers that this value is intended to remain constant. This can make the code easier to understand and maintain.

In some cases, using `const` can lead to performance improvements. The compiler might be able to make optimizations knowing that the value of a `const` variable will not change.

0 comments on commit 65743cb

Please sign in to comment.