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.

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

Create rule S3063: Add vbnet language #1524

Merged
merged 10 commits into from
Feb 15, 2023
55 changes: 44 additions & 11 deletions rules/S3063/csharp/rule.adoc
Original file line number Diff line number Diff line change
@@ -1,16 +1,49 @@
include::../rule.adoc[]
`StringBuilder` instances that are not accessed through a `ToString()` invocation needlessly clutter the code, and worse are a drag on performance. Either they should be removed, or the missing `ToString()` call should be added.

Choose a reason for hiding this comment

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

Maybe like this?

Suggested change
`StringBuilder` instances that are not accessed through a `ToString()` invocation needlessly clutter the code, and worse are a drag on performance. Either they should be removed, or the missing `ToString()` call should be added.
`StringBuilder` instances that never build a `string` clutter the code and worse are a drag on performance. Either they should be removed, or the missing `ToString()` call should be added.

Choose a reason for hiding this comment

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

Yes that's better, thanks


ifdef::env-github,rspecator-view[]
== Noncompliant Code Example

'''
== Implementation Specification
(visible only on this page)
[source,csharp]
----
public void DoSomething(List<string> strings) {
var sb = new StringBuilder(); // Noncompliant
sb.Append("Got: ");
foreach(var str in strings) {
sb.Append(str).Append(", ");
// ...
}
}
----

include::../message.adoc[]
== Compliant Solution

'''
== Comments And Links
(visible only on this page)
[source,csharp]
----
public void DoSomething(List<string> strings) {
foreach(var str in strings) {
// ...
}
}
----
or
[source,csharp]
----
public void DoSomething(List<string> strings) {
var sb = new StringBuilder();
sb.Append("Got: ");
foreach(var str in strings) {
sb.Append(str).Append(", ");
// ...
}
_logger.LogInformation(sb.toString);

Choose a reason for hiding this comment

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

I think the _ is a violation of one of our rules.

Suggested change
_logger.LogInformation(sb.toString);
logger.LogInformation(sb.ToString());

}
----

include::../comments-and-links.adoc[]
endif::env-github,rspecator-view[]
== Exceptions

No issue is reported when `StringBuilder` is:

* Accessed through a `CopyTo()` or `GetChunks()` invocation.

Choose a reason for hiding this comment

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

Should we add the Length access and the indexer access?

Choose a reason for hiding this comment

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

I don't know; I was wondering the same. I decided to skip them to simplify the exception paragraph since both are not that common, but I might be wrong.

Choose a reason for hiding this comment

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

Maybe like this?

Suggested change
* Accessed through a `CopyTo()` or `GetChunks()` invocation.
* Accessed through `sb.CopyTo()`, `sb.GetChunks()`, `sb.Length`, or `sb[index]`.

Choose a reason for hiding this comment

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

Since we get rid of the exception examples it's not that complicated anymore so maybe we can add them for clarity.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I only just saw your comment. I created a sub-list for them, but your suggestion may be better indeed, let me change that real quick.

* Passed as method argument, on the grounds that it will likely accessed through a `ToString()` invocation there.

Choose a reason for hiding this comment

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

Suggested change
* Passed as method argument, on the grounds that it will likely accessed through a `ToString()` invocation there.
* Passed as a method argument, on the grounds that it will likely be accessed through a `ToString()` invocation there.

* A parameter of the current method.

Choose a reason for hiding this comment

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

Suggested change
* A parameter of the current method.
* Passed in as a parameter to the current method, on the grounds that the callee will materialize the string.

* Retrieved by a custom function (`var sb = GetStringBuilder();`).
* Returned by the method.
2 changes: 2 additions & 0 deletions rules/S3063/vbnet/metadata.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
{
}
49 changes: 49 additions & 0 deletions rules/S3063/vbnet/rule.adoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
`StringBuilder` instances that are not accessed through a `ToString()` invocation needlessly clutter the code, and worse are a drag on performance. Either they should be removed, or the missing `ToString()` call should be added.

== Noncompliant Code Example

[source,vbnet]
----
Public Sub DoSomething(ByVal strings As List(Of String))
Dim sb As StringBuilder = New StringBuilder() ' Noncompliant
sb.Append("Got: ")

For Each str As String In strings
sb.Append(str).Append(", ")
Next
End Sub
----

== Compliant Solution

[source,vbnet]
----
Public Sub DoSomething(ByVal strings As List(Of String))
For Each str As String In strings
Next
End Sub
----
or
[source,vbnet]
----
Public Sub DoSomething(ByVal strings As List(Of String))
Dim sb As StringBuilder = New StringBuilder()
sb.Append("Got: ")

For Each str As String In strings
sb.Append(str).Append(", ")
Next

My.Application.Log.WriteEntry(sb.toString)
End Sub
----

== Exceptions

No issue is reported when `StringBuilder` is:

* Accessed through a `CopyTo()` or `GetChunks()` invocation.
* Passed as method arguments, on the grounds that it will likely accessed through a `ToString()` invocation there.
* A parameter of the current method.
* Retrieved by a custom function (`Dim sb As StringBuilder = GetStringBuilder()`).
* Returned by the method.