Skip to content

Commit

Permalink
Rspec update before release (#8288)
Browse files Browse the repository at this point in the history
  • Loading branch information
@costin-zaharia-sonarsource
costin-zaharia-sonarsource authored Nov 1, 2023
1 parent 7c70640 commit 0b84a16
Show file tree
Hide file tree
Showing 93 changed files with 1,509 additions and 659 deletions.
55 changes: 38 additions & 17 deletions analyzers/rspec/cs/S106.html
View file
Original file line number Diff line number Diff line change
@@ -1,29 +1,50 @@
<h2>Why is this an issue?</h2>
<p>When logging a message there are several important requirements which must be fulfilled:</p>
<p>In software development, logs serve as a record of events within an application, providing crucial insights for debugging. When logging, it is
essential to ensure that the logs are:</p>
<ul>
<li> The user must be able to easily retrieve the logs </li>
<li> The format of all logged message must be uniform to allow the user to easily read the log </li>
<li> Logged data must actually be recorded </li>
<li> Sensitive data must only be logged securely </li>
<li> easily accessible </li>
<li> uniformly formatted for readability </li>
<li> properly recorded </li>
<li> securely logged when dealing with sensitive data </li>
</ul>
<p>If a program directly writes to the standard outputs, there is absolutely no way to comply with those requirements. That’s why defining and using a
dedicated logger is highly recommended.</p>
<h3>Noncompliant code example</h3>
<pre>
private void DoSomething()
{
// ...
Console.WriteLine("so far, so good..."); // Noncompliant
// ...
}
</pre>
<p>Those requirements are not met if a program directly writes to the standard outputs (e.g., Console). That is why defining and using a dedicated
logger is highly recommended.</p>
<h3>Exceptions</h3>
<p>The following are ignored by this rule:</p>
<p>The rule doesn’t raise an issue for:</p>
<ul>
<li> Console Applications </li>
<li> Calls in methods decorated with <code>[Conditional ("DEBUG")]</code> </li>
<li> Calls included in DEBUG preprocessor branches (<code>#if DEBUG</code>) </li>
</ul>
<h3>Code examples</h3>
<p>The following noncompliant code:</p>
<pre data-diff-id="1" data-diff-type="noncompliant">
public class MyClass
{
private void DoSomething()
{
// ...
Console.WriteLine("My Message"); // Noncompliant
// ...
}
}
</pre>
<p>Could be replaced by:</p>
<pre data-diff-id="1" data-diff-type="compliant">
public class MyClass
{
private readonly ILogger _logger;

// ...

private void DoSomething()
{
// ...
_logger.LogInformation("My Message");
// ...
}
}
</pre>
<h2>Resources</h2>
<ul>
<li> <a href="https://owasp.org/Top10/A09_2021-Security_Logging_and_Monitoring_Failures/">OWASP Top 10 2021 Category A9</a> - Security Logging and
Expand Down
40 changes: 35 additions & 5 deletions analyzers/rspec/cs/S1066.html
View file
Original file line number Diff line number Diff line change
@@ -1,20 +1,50 @@
<h2>Why is this an issue?</h2>
<p>Merging collapsible <code>if</code> statements increases the code’s readability.</p>
<h3>Noncompliant code example</h3>
<p>Nested code - blocks of code inside blocks of code - is eventually necessary, but increases complexity. This is why keeping the code as flat as
possible, by avoiding unnecessary nesting, is considered a good practice.</p>
<p>Merging <code>if</code> statements when possible will decrease the nesting of the code and improve its readability.</p>
<p>Code like</p>
<pre>
if (condition1)
{
if (condition2)
if (condition2) // Noncompliant
{
// ...
}
}
</pre>
<h3>Compliant solution</h3>
<p>Will be more readable as</p>
<pre>
if (condition1 &amp;&amp; condition2)
if (condition1 &amp;&amp; condition2) // Compliant
{
// ...
}
</pre>
<h2>How to fix it</h2>
<p>If merging the conditions seems to result in a more complex code, extracting the condition or part of it in a named function or variable is a
better approach to fix readability.</p>
<h3>Code examples</h3>
<h4>Noncompliant code example</h4>
<pre>
if (file != null)
{
if (file.isFile() || file.isDirectory()) // Noncompliant
{
/* ... */
}
}
</pre>
<h4>Compliant solution</h4>
<pre>
bool isFileOrDirectory(File file)
{
return file.isFile() || file.isDirectory();
}

/* ... */

if (file != null &amp;&amp; isFileOrDirectory(file)) // Compliant
{
/* ... */
}
</pre>

2 changes: 1 addition & 1 deletion analyzers/rspec/cs/S1066.json
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"title": "Collapsible \"if\" statements should be merged",
"title": "Mergeable \"if\" statements should be combined",
"type": "CODE_SMELL",
"code": {
"impacts": {
Expand Down
45 changes: 39 additions & 6 deletions analyzers/rspec/cs/S1075.html
View file
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
<h2>Why is this an issue?</h2>
<p>Hardcoding a URI makes it difficult to test a program: path literals are not always portable across operating systems, a given absolute path may
not exist on a specific test environment, a specified Internet URL may not be available when executing the tests, production environment filesystems
usually differ from the development environment, …​etc. For all those reasons, a URI should never be hardcoded. Instead, it should be replaced by
customizable parameter.</p>
<p>Further even if the elements of a URI are obtained dynamically, portability can still be limited if the path-delimiters are hardcoded.</p>
<p>This rule raises an issue when URI’s or path delimiters are hardcoded.</p>
<p>Hard-coding a URI makes it difficult to test a program for a variety of reasons:</p>
<ul>
<li> path literals are not always portable across operating systems </li>
<li> a given absolute path may not exist in a specific test environment </li>
<li> a specified Internet URL may not be available when executing the tests </li>
<li> production environment filesystems usually differ from the development environment </li>
</ul>
<p>In addition, hard-coded URIs can contain sensitive information, like IP addresses, and they should not be stored in the code.</p>
<p>For all those reasons, a URI should never be hard coded. Instead, it should be replaced by a customizable parameter.</p>
<p>Further, even if the elements of a URI are obtained dynamically, portability can still be limited if the path delimiters are hard-coded.</p>
<p>This rule raises an issue when URIs or path delimiters are hard-coded.</p>
<h3>Exceptions</h3>
<p>This rule does not raise an issue when an ASP.NET virtual path is passed as an argument to one of the following:</p>
<ul>
Expand All @@ -13,4 +18,32 @@ <h3>Exceptions</h3>
<li> all methods of: <code>System.Web.VirtualPathUtility</code> </li>
<li> constructors of: <code>Microsoft.AspNetCore.Mvc.VirtualFileResult</code>, <code>Microsoft.AspNetCore.Routing.VirtualPathData</code> </li>
</ul>
<h2>How to fix it</h2>
<h3>Code examples</h3>
<h4>Noncompliant code example</h4>
<pre data-diff-id="1" data-diff-type="noncompliant">
public class Foo {
public List&lt;User&gt; ListUsers() {
string userListPath = "/home/mylogin/Dev/users.txt"; // Noncompliant
return ParseUsers(userListPath);
}
}
</pre>
<h4>Compliant solution</h4>
<pre data-diff-id="1" data-diff-type="compliant">
public class Foo {
// Configuration is a class that returns customizable properties: it can be mocked to be injected during tests.
private Configuration config;
public Foo(Configuration myConfig) {
this.config = myConfig;
}
public List&lt;User&gt; ListUsers() {
// Find here the way to get the correct folder, in this case using the Configuration object
string listingFolder = config.GetProperty("myApplication.listingFolder");
// and use this parameter instead of the hard coded path
string userListPath = Path.Combine(listingFolder, "users.txt"); // Compliant
return ParseUsers(userListPath);
}
}
</pre>

50 changes: 27 additions & 23 deletions analyzers/rspec/cs/S109.html
View file
Original file line number Diff line number Diff line change
@@ -1,39 +1,43 @@
<p>A magic number is a hard-coded numerical value that may lack context or meaning. They should not be used because they can make the code less
readable and maintainable.</p>
<h2>Why is this an issue?</h2>
<p>A magic number is a number that comes out of nowhere, and is directly used in a statement. Magic numbers are often used, for instance to limit the
number of iterations of a loop, to test the value of a property, etc.</p>
<p>Using magic numbers may seem obvious and straightforward when you’re writing a piece of code, but they are much less obvious and straightforward at
debugging time.</p>
<p>That is why magic numbers must be demystified by first being assigned to clearly named variables before being used.</p>
<p>-1, 0 and 1 are not considered magic numbers.</p>
<h3>Noncompliant code example</h3>
<pre>
public static void DoSomething()
<p>Magic numbers make the code more complex to understand as it requires the reader to have knowledge about the global context to understand the
number itself. Their usage may seem obvious when writing the code, but it may not be the case for another developer or later once the context faded
away. -1, 0, and 1 are not considered magic numbers.</p>
<h3>Exceptions</h3>
<p>This rule doesn’t raise an issue when the magic number is used as part of:</p>
<ul>
<li> the <code>GetHashCode</code> method </li>
<li> a variable/field declaration </li>
<li> the single argument of an attribute </li>
<li> a named argument for a method or attribute </li>
<li> a constructor call </li>
<li> a default value for a method argument </li>
</ul>
<h2>How to fix it</h2>
<p>Replacing them with a constant allows us to provide a meaningful name associated with the value. Instead of adding complexity to the code, it
brings clarity and helps to understand the context and the global meaning.</p>
<h3>Code examples</h3>
<h4>Noncompliant code example</h4>
<pre data-diff-id="1" data-diff-type="noncompliant">
public void DoSomething()
{
for(int i = 0; i &lt; 4; i++) // Noncompliant, 4 is a magic number
for (int i = 0; i &lt; 4; i++) // Noncompliant, 4 is a magic number
{
...
}
}
</pre>
<h3>Compliant solution</h3>
<pre>
<h4>Compliant solution</h4>
<pre data-diff-id="1" data-diff-type="compliant">
private const int NUMBER_OF_CYCLES = 4;

public static void DoSomething()
public void DoSomething()
{
for(int i = 0; i &lt; NUMBER_OF_CYCLES ; i++) //Compliant
for (int i = 0; i &lt; NUMBER_OF_CYCLES; i++) // Compliant
{
...
}
}
</pre>
<h3>Exceptions</h3>
<p>This rule doesn’t raise an issue when the magic number is used as part of:</p>
<ul>
<li> the <code>GetHashCode</code> method </li>
<li> a variable/field declaration </li>
<li> the single argument of an attribute </li>
<li> a named argument for a method or attribute </li>
<li> a constructor call </li>
</ul>

61 changes: 46 additions & 15 deletions analyzers/rspec/cs/S1104.html
View file
Original file line number Diff line number Diff line change
@@ -1,36 +1,67 @@
<h2>Why is this an issue?</h2>
<p>Public fields in public classes do not respect the encapsulation principle and has three main disadvantages:</p>
<p>Public fields in public classes do not respect the encapsulation principle and have three main disadvantages:</p>
<ul>
<li> Additional behavior such as validation cannot be added. </li>
<li> The internal representation is exposed, and cannot be changed afterwards. </li>
<li> Member values are subject to change from anywhere in the code and may not meet the programmer’s assumptions. </li>
</ul>
<p>By using private fields and public properties (set and get), unauthorized modifications are prevented. Properties also benefit from additional
protection (security) features such as Link Demands.</p>
<p>To prevent unauthorized modifications, private attributes and accessor methods (set and get) should be used.</p>
<p>Note that due to optimizations on simple properties, public fields provide only very little performance gain.</p>
<h3>Noncompliant code example</h3>
<pre>
<h3>What is the potential impact?</h3>
<p>Public fields can be modified by any part of the code and this can lead to unexpected changes and hard-to-trace bugs.</p>
<p>Public fields don’t hide the implementation details. As a consequence, it is no longer possible to change how the data is stored internally without
impacting the client code of the class.</p>
<p>The code is harder to maintain.</p>
<h3>Exceptions</h3>
<p>Fields marked as <code>readonly</code> or <code>const</code> are ignored by this rule.</p>
<p>Fields inside classes or structs annotated with the <code>StructLayoutAttribute</code> are ignored by this rule.</p>
<h2>How to fix it</h2>
<p>Depending on your needs:</p>
<ul>
<li> Use auto-implemented properties:<br> For common cases, where no validation is required, auto-implemented properties are a good alternative to
fields: these allows fine grained access control and offers the flexibility to add validation or change internal storage afterwards. <em>Note:</em>
as a bonus it is now possible to monitor value changes using breakpoints. </li>
<li> Encapsulate the fields in your class. To do so:
<ol>
<li> Make the field private. </li>
<li> Use public properties (set and get) to access and modify the field. </li>
</ol> </li>
<li> Mark field as <code>readonly</code> or <code>const</code>. </li>
</ul>
<h3>Code examples</h3>
<h4>Noncompliant code example</h4>
<pre data-diff-id="1" data-diff-type="noncompliant">
public class Foo
{
public int instanceData = 32; // Noncompliant
public int InstanceData = 32; // Noncompliant
public int AnotherInstanceData = 32; // Noncompliant

}
</pre>
<h3>Compliant solution</h3>
<pre>
<h4>Compliant solution</h4>
<pre data-diff-id="1" data-diff-type="compliant">
public class Foo
{
private int instanceData = 32;
// using auto-implemented properties
public int InstanceData { get; set; } = 32;

public int InstanceData
// using field encapsulation
private int _anotherInstanceData = 32;

public int AnotherInstanceData
{
get { return instanceData; }
set { instanceData = value ; }
get { return _anotherInstanceData; }
set
{
// perform validation
_anotherInstanceData = value;
}
}

}
</pre>
<h3>Exceptions</h3>
<p>Fields marked as <code>readonly</code> or <code>const</code> are ignored by this rule.</p>
<p>Fields inside classes or structs annotated with the <code>StructLayoutAttribute</code> are ignored by this rule.</p>
<h3>Pitfalls</h3>
<p>Please be aware that changing a field by a property in a software that uses serialization could lead to binary incompatibility.</p>
<h2>Resources</h2>
<ul>
<li> <a href="https://cwe.mitre.org/data/definitions/493">MITRE, CWE-493</a> - Critical Public Variable Without Final Modifier </li>
Expand Down
26 changes: 14 additions & 12 deletions analyzers/rspec/cs/S1110.html
View file
Original file line number Diff line number Diff line change
@@ -1,22 +1,24 @@
<h2>Why is this an issue?</h2>
<p>The use of parentheses, even those not required to enforce a desired order of operations, can clarify the intent behind a piece of code. But
redundant pairs of parentheses could be misleading, and should be removed.</p>
<p>Parentheses can disambiguate the order of operations in complex expressions and make the code easier to understand.</p>
<pre>
a = (b * c) + (d * e); // Compliant: the intent is clear.
</pre>
<p>Redundant parentheses are parenthesis that do not change the behavior of the code, and do not clarify the intent. They can mislead and complexify
the code. They should be removed.</p>
<h3>Noncompliant code example</h3>
<pre data-diff-id="1" data-diff-type="noncompliant">
if (a &amp;&amp; ((x + y &gt; 0))) // Noncompliant
{
//...
}
int x = ((y / 2 + 1)); // Noncompliant

return ((x + 1)); // Noncompliant
if (a &amp;&amp; ((x + y &gt; 0))) { // Noncompliant
return ((x + 1)); // Noncompliant
}
</pre>
<h3>Compliant solution</h3>
<pre data-diff-id="1" data-diff-type="compliant">
if (a &amp;&amp; (x + y &gt; 0))
{
//...
}
int x = (y / 2 + 1);

return x + 1;
if (a &amp;&amp; (x + y &gt; 0)) {
return (x + 1);
}
</pre>

Loading

0 comments on commit 0b84a16

Please sign in to comment.