Skip to content

Commit

Permalink
SONARJAVA-4661 Update Rules Metadata (#4496)
Browse files Browse the repository at this point in the history
A batch of updates from the LaYC update
  • Loading branch information
Wohops authored Oct 20, 2023
1 parent f106053 commit b4ff76a
Show file tree
Hide file tree
Showing 58 changed files with 734 additions and 444 deletions.
Original file line number Diff line number Diff line change
@@ -1,30 +1,35 @@
<h2>Why is this an issue?</h2>
<p>When logging a message, the following requirements should 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 can easily record logged data and retrieve the logs. </li>
<li> The format of all logged messages is uniform and provides context about the message creator. This improves the readability of the logs. </li>
<li> Sensitive data is 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>Simply printing messages to <code>System.out</code> or <code>System.err</code> does not fulfill these needs. A dedicated logger should be used
instead.</p>
<h3>Noncompliant code example</h3>
<p>Those requirements are not met if a program directly writes to the standard outputs (e.g., System.out, System.err). That is why defining and using
a dedicated logger is highly recommended.</p>
<h3>Code examples</h3>
<p>The following noncompliant code:</p>
<pre data-diff-id="1" data-diff-type="noncompliant">
class PrintMessage {
public void print() {
class MyClass {
public void doSomething() {
System.out.println("My Message"); // Noncompliant, output directly to System.out without a logger
}
}
</pre>
<h3>Compliant solution</h3>
<p>Could be replaced by:</p>
<pre data-diff-id="1" data-diff-type="compliant">
import java.util.logging.Logger;

class PrintMessage {
class MyClass {

Logger logger = Logger.getLogger(getClass().getName());

public void print() {
public void doSomething() {
// ...
logger.info("My Message"); // Compliant, output via logger
// ...
}
}
</pre>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,16 +1,36 @@
<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) { // Noncompliant
/* ... */
}
}
</pre>
<p>Will be more readable as</p>
<pre>
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()) {
if (file.isFile() || file.isDirectory()) { // Noncompliant
/* ... */
}
}
</pre>
<h3>Compliant solution</h3>
<h4>Compliant solution</h4>
<pre>
if (file != null &amp;&amp; isFileOrDirectory(file)) {
if (file != null &amp;&amp; isFileOrDirectory(file)) { // Compliant
/* ... */
}

Expand Down
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
Original file line number Diff line number Diff line change
@@ -1,12 +1,19 @@
<h2>Why is this an issue?</h2>
<p>Hard coding 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 hard coded. 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 hard-coded.</p>
<p>This rule raises an issue when URI’s or path delimiters are hard coded.</p>
<h3>Noncompliant code example</h3>
<pre>
<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>
<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 Collection&lt;User&gt; listUsers() {
File userList = new File("/home/mylogin/Dev/users.txt"); // Noncompliant
Expand All @@ -15,8 +22,8 @@ <h3>Noncompliant code example</h3>
}
}
</pre>
<h3>Compliant solution</h3>
<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;
Expand All @@ -33,8 +40,4 @@ <h3>Compliant solution</h3>
}
}
</pre>
<h2>Resources</h2>
<ul>
<li> <a href="https://wiki.sei.cmu.edu/confluence/x/OjdGBQ">CERT, MSC03-J.</a> - Never hard code sensitive information </li>
</ul>

Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
readable and maintainable.</p>
<h2>Why is this an issue?</h2>
<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 at the moment you’re 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>
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 ignores <code>hashCode</code> methods.</p>
<h2>How to fix it</h2>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,24 +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>
<h3>Noncompliant code example</h3>
<p>Parentheses can disambiguate the order of operations in complex expressions and make the code easier to understand.</p>
<pre>
int x = (y / 2 + 1); //Compliant even if the parenthesis are ignored by the compiler
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">
int x = ((y / 2 + 1)); // Noncompliant

if (a &amp;&amp; ((x+y &gt; 0))) { // Noncompliant
//...
if (a &amp;&amp; ((x + y &gt; 0))) { // Noncompliant
return ((x + 1)); // Noncompliant
}

return ((x + 1)); // Noncompliant
</pre>
<h3>Compliant solution</h3>
<pre>
<pre data-diff-id="1" data-diff-type="compliant">
int x = (y / 2 + 1);

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

return (x + 1);
</pre>

Original file line number Diff line number Diff line change
@@ -1,32 +1,32 @@
<h2>Why is this an issue?</h2>
<p>Empty statements, i.e. <code>;</code>, are usually introduced by mistake, for example because:</p>
<ul>
<li> It was meant to be replaced by an actual statement, but this was forgotten. </li>
<li> There was a typo which lead the semicolon to be doubled, i.e. <code>;;</code>. </li>
</ul>
<h3>Noncompliant code example</h3>
<pre>
<p>Empty statements represented by a semicolon <code>;</code> are statements that do not perform any operation. They are often the result of a typo or
a misunderstanding of the language syntax. It is a good practice to remove empty statements since they don’t add value and lead to confusion and
errors.</p>
<h3>Code examples</h3>
<h4>Noncompliant code example</h4>
<pre data-diff-id="1" data-diff-type="noncompliant">
void doSomething() {
; // Noncompliant - was used as a kind of TODO marker
; // Noncompliant - was used as a kind of TODO marker
}

void doSomethingElse() {
System.out.println("Hello, world!");; // Noncompliant - double ;
...
System.out.println("Hello, world!");; // Noncompliant - double ;
// ...
}
</pre>
<h3>Compliant solution</h3>
<pre>
<h4>Compliant solution</h4>
<pre data-diff-id="1" data-diff-type="compliant">
void doSomething() {}

void doSomethingElse() {
System.out.println("Hello, world!");
...
for (int i = 0; i &lt; 3; i++) ; // compliant if unique statement of a loop
...
// ...
for (int i = 0; i &lt; 3; i++) ; // Compliant if unique statement of a loop
// ...
}
</pre>
<h2>Resources</h2>
<h3>Documentation</h3>
<ul>
<li> <a href="https://wiki.sei.cmu.edu/confluence/x/5dUxBQ">CERT, MSC12-C.</a> - Detect and remove code that has no effect or is never executed
</li>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,22 +1,36 @@
<h2>Why is this an issue?</h2>
<p>Overriding or shadowing a variable declared in an outer scope can strongly impact the readability, and therefore the maintainability, of a piece of
code. Further, it could lead maintainers to introduce bugs because they think they’re using one variable but are really using another.</p>
<p>Shadowing occurs when a local variable has the same name as a variable or a field in an outer scope.</p>
<p>This can lead to three main problems:</p>
<ul>
<li> Confusion: The same name can refer to different variables in different parts of the scope, making the code hard to read and understand. </li>
<li> Unintended Behavior: You might accidentally use the wrong variable, leading to hard-to-detect bugs. </li>
<li> Maintenance Issues: If the inner variable is removed or renamed, the code’s behavior might change unexpectedly because the outer variable is
now being used. </li>
</ul>
<p>To avoid these problems, rename the shadowing, shadowed, or both identifiers to accurately represent their purpose with unique and meaningful
names.</p>
<p>This rule focuses on variables in methods that shadow a field.</p>
<h3>Noncompliant code example</h3>
<pre>
class Foo {
public int myField;

public void doSomething() {
int myField = 0; // Noncompliant
...
// ...
}
}
</pre>
<h2>Resources</h2>
<h3>Documentation</h3>
<ul>
<li> <a href="https://wiki.sei.cmu.edu/confluence/display/c/DCL01-C.+Do+not+reuse+variable+names+in+subscopes">CERT, DCL01-C.</a> - Do not reuse
variable names in subscopes </li>
<li> <a href="https://wiki.sei.cmu.edu/confluence/display/java/DCL51-J.+Do+not+shadow+or+obscure+identifiers+in+subscopes">CERT, DCL51-J.</a> - Do
not shadow or obscure identifiers in subscopes </li>
<li> CERT - <a href="https://wiki.sei.cmu.edu/confluence/display/java/DCL51-J.+Do+not+shadow+or+obscure+identifiers+in+subscopes">DCL51-J. Do not
shadow or obscure identifiers in subscopes</a> </li>
</ul>
<h3>Related rules</h3>
<ul>
<li> {rule:java:S2176} - Class names should not shadow interfaces or superclasses </li>
<li> {rule:java:S2387} - Child class fields should not shadow parent class fields </li>
<li> {rule:java:S4977} - Type parameters should not shadow other type parameters </li>
</ul>

Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
{
"title": "Local variables should not shadow class fields",
"type": "CODE_SMELL",
"code": {
"impacts": {
Expand All @@ -22,10 +21,10 @@
"sqKey": "S1117",
"scope": "All",
"quickfix": "unknown",
"title": "Local variables should not shadow class fields",
"securityStandards": {
"CERT": [
"DCL51-J.",
"DCL01-C."
"DCL51-J."
]
}
}
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
<h2>Why is this an issue?</h2>
<p>Utility classes, which are collections of <code>static</code> members, are not meant to be instantiated. Even abstract utility classes, which can
be extended, should not have public constructors.</p>
<p>Java adds an implicit public constructor to every class which does not define at least one explicitly. Hence, at least one non-public constructor
should be defined.</p>
<h3>Noncompliant code example</h3>
<p>Whenever there are portions of code that are duplicated and do not depend on the state of their container class, they can be centralized inside a
"utility class". A utility class is a class that only has static members, hence it should not be instantiated.</p>
<h3>Exceptions</h3>
<p>When a class contains <code>public static void main(String[] args)</code> method it is not considered as a utility class and will be ignored by
this rule.</p>
<h2>How to fix it</h2>
<p>To prevent the class from being instantiated, you should define a non-public constructor. This will prevent the compiler from implicitly generating
a public parameterless constructor.</p>
<h3>Code examples</h3>
<h4>Noncompliant code example</h4>
<pre data-diff-id="1" data-diff-type="noncompliant">
class StringUtils { // Noncompliant

Expand All @@ -13,7 +18,7 @@ <h3>Noncompliant code example</h3>

}
</pre>
<h3>Compliant solution</h3>
<h4>Compliant solution</h4>
<pre data-diff-id="1" data-diff-type="compliant">
class StringUtils { // Compliant

Expand All @@ -27,7 +32,4 @@ <h3>Compliant solution</h3>

}
</pre>
<h3>Exceptions</h3>
<p>When class contains <code>public static void main(String[] args)</code> method it is not considered as utility class and will be ignored by this
rule.</p>

Loading

0 comments on commit b4ff76a

Please sign in to comment.