When logging a message there are several important requirements which must be fulfilled:
+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:
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.
-
-private void DoSomething()
-{
- // ...
- Console.WriteLine("so far, so good..."); // Noncompliant
- // ...
-}
-
+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.
The following are ignored by this rule:
+The rule doesn’t raise an issue for:
[Conditional ("DEBUG")] #if DEBUG) The following noncompliant code:
+
+public class MyClass
+{
+ private void DoSomething()
+ {
+ // ...
+ Console.WriteLine("My Message"); // Noncompliant
+ // ...
+ }
+}
+
+Could be replaced by:
+
+public class MyClass
+{
+ private readonly ILogger _logger;
+
+ // ...
+
+ private void DoSomething()
+ {
+ // ...
+ _logger.LogInformation("My Message");
+ // ...
+ }
+}
+
Merging collapsible if statements increases the code’s readability.
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.
+Merging if statements when possible will decrease the nesting of the code and improve its readability.
Code like
if (condition1)
{
- if (condition2)
+ if (condition2) // Noncompliant
{
// ...
}
}
-Will be more readable as
-if (condition1 && condition2)
+if (condition1 && condition2) // Compliant
{
// ...
}
+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.
+
+if (file != null)
+{
+ if (file.isFile() || file.isDirectory()) // Noncompliant
+ {
+ /* ... */
+ }
+}
+
+
+bool isFileOrDirectory(File file)
+{
+ return file.isFile() || file.isDirectory();
+}
+
+/* ... */
+
+if (file != null && isFileOrDirectory(file)) // Compliant
+{
+ /* ... */
+}
+
diff --git a/analyzers/rspec/cs/S1066.json b/analyzers/rspec/cs/S1066.json
index f6857be37ab..732a6ea96fb 100644
--- a/analyzers/rspec/cs/S1066.json
+++ b/analyzers/rspec/cs/S1066.json
@@ -1,5 +1,5 @@
{
- "title": "Collapsible \"if\" statements should be merged",
+ "title": "Mergeable \"if\" statements should be combined",
"type": "CODE_SMELL",
"code": {
"impacts": {
diff --git a/analyzers/rspec/cs/S1075.html b/analyzers/rspec/cs/S1075.html
index 4932c455f95..2d29c1fae98 100644
--- a/analyzers/rspec/cs/S1075.html
+++ b/analyzers/rspec/cs/S1075.html
@@ -1,10 +1,15 @@
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.
-Further even if the elements of a URI are obtained dynamically, portability can still be limited if the path-delimiters are hardcoded.
-This rule raises an issue when URI’s or path delimiters are hardcoded.
+Hard-coding a URI makes it difficult to test a program for a variety of reasons:
+In addition, hard-coded URIs can contain sensitive information, like IP addresses, and they should not be stored in the code.
+For all those reasons, a URI should never be hard coded. Instead, it should be replaced by a customizable parameter.
+Further, even if the elements of a URI are obtained dynamically, portability can still be limited if the path delimiters are hard-coded.
+This rule raises an issue when URIs or path delimiters are hard-coded.
This rule does not raise an issue when an ASP.NET virtual path is passed as an argument to one of the following:
System.Web.VirtualPathUtility Microsoft.AspNetCore.Mvc.VirtualFileResult, Microsoft.AspNetCore.Routing.VirtualPathData
+public class Foo {
+ public List<User> ListUsers() {
+ string userListPath = "/home/mylogin/Dev/users.txt"; // Noncompliant
+ return ParseUsers(userListPath);
+ }
+}
+
+
+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<User> 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);
+ }
+}
+
diff --git a/analyzers/rspec/cs/S109.html b/analyzers/rspec/cs/S109.html
index 215257ab78b..538216ef3ce 100644
--- a/analyzers/rspec/cs/S109.html
+++ b/analyzers/rspec/cs/S109.html
@@ -1,39 +1,43 @@
+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.
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.
-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.
-That is why magic numbers must be demystified by first being assigned to clearly named variables before being used.
--1, 0 and 1 are not considered magic numbers.
--public static void DoSomething() +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.
+Exceptions
+This rule doesn’t raise an issue when the magic number is used as part of:
+
GetHashCode method 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.
+
+public void DoSomething()
{
- for(int i = 0; i < 4; i++) // Noncompliant, 4 is a magic number
+ for (int i = 0; i < 4; i++) // Noncompliant, 4 is a magic number
{
...
}
}
-+Compliant solution
+private const int NUMBER_OF_CYCLES = 4; -public static void DoSomething() +public void DoSomething() { - for(int i = 0; i < NUMBER_OF_CYCLES ; i++) //Compliant + for (int i = 0; i < NUMBER_OF_CYCLES; i++) // Compliant { ... } }-Exceptions
-This rule doesn’t raise an issue when the magic number is used as part of:
-
GetHashCode method Public fields in public classes do not respect the encapsulation principle and has three main disadvantages:
+Public fields in public classes do not respect the encapsulation principle and have three main disadvantages:
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.
+To prevent unauthorized modifications, private attributes and accessor methods (set and get) should be used.
Note that due to optimizations on simple properties, public fields provide only very little performance gain.
-+What is the potential impact?
+Public fields can be modified by any part of the code and this can lead to unexpected changes and hard-to-trace bugs.
+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.
+The code is harder to maintain.
+Exceptions
+Fields marked as
+readonlyorconstare ignored by this rule.Fields inside classes or structs annotated with the
+StructLayoutAttributeare ignored by this rule.How to fix it
+Depending on your needs:
+
readonly or const.
public class Foo
{
- public int instanceData = 32; // Noncompliant
+ public int InstanceData = 32; // Noncompliant
+ public int AnotherInstanceData = 32; // Noncompliant
+
}
-+Compliant solution
+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; + } } + }-Exceptions
-Fields marked as
-readonlyorconstare ignored by this rule.Fields inside classes or structs annotated with the
+StructLayoutAttributeare ignored by this rule.Pitfalls
+Please be aware that changing a field by a property in a software that uses serialization could lead to binary incompatibility.
Resources
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.
+Parentheses can disambiguate the order of operations in complex expressions and make the code easier to understand.
++a = (b * c) + (d * e); // Compliant: the intent is clear. ++
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.
-if (a && ((x + y > 0))) // Noncompliant
-{
- //...
-}
+int x = ((y / 2 + 1)); // Noncompliant
-return ((x + 1)); // Noncompliant
+if (a && ((x + y > 0))) { // Noncompliant
+ return ((x + 1)); // Noncompliant
+}
-if (a && (x + y > 0))
-{
- //...
-}
+int x = (y / 2 + 1);
-return x + 1;
+if (a && (x + y > 0)) {
+ return (x + 1);
+}
diff --git a/analyzers/rspec/cs/S1116.html b/analyzers/rspec/cs/S1116.html
index 3e3e66edd1a..b9cbe83dcb3 100644
--- a/analyzers/rspec/cs/S1116.html
+++ b/analyzers/rspec/cs/S1116.html
@@ -1,11 +1,10 @@
Empty statements, i.e. ;, are usually introduced by mistake, for example because:
;;. +Empty statements represented by a semicolon
+;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.Code examples
+Noncompliant code example
+void DoSomething() { ; // Noncompliant - was used as a kind of TODO marker @@ -21,8 +20,8 @@-Noncompliant code example
// ... }Compliant solution
-+Compliant solution
+void DoSomething() { } diff --git a/analyzers/rspec/cs/S1117.html b/analyzers/rspec/cs/S1117.html index 4b9f2facf1f..f565132f2cb 100644 --- a/analyzers/rspec/cs/S1117.html +++ b/analyzers/rspec/cs/S1117.html @@ -1,7 +1,16 @@Why is this an issue?
-Overriding or shadowing a field or a property declared in an outer scope can strongly impact the readability, and therefore the maintainability, of -a piece of code. Developers may mistakenly assume they are modifying or accessing the class field/property when, in fact, they are working with the -local variable.
+Shadowing occurs when a local variable has the same name as a variable, field, or property in an outer scope.
+This can lead to three main problems:
+
To avoid these problems, rename the shadowing, shadowed, or both variables/fields/properties to accurately represent their purpose with unique and +meaningful names. It improves clarity and allows reasoning locally about the code without considering other software parts.
+This rule focuses on variables shadowing fields or properties.
+
class Foo
{
@@ -18,7 +27,13 @@ Why is this an issue?
Resources
Documentation
Utility classes, which are collections of static members, are not meant to be
-instantiated.
C# adds an implicit public constructor to every class
-which does not explicitly define at least one constructor. Hence, at least one protected constructor should be defined
-if you wish to inherit this utility class. Or the static keyword should be added to the class declaration to prevent inheriting it.
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.
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.
+Alternatively, adding the static keyword as class modifier will also prevent it from being instantiated.
@@ -35,7 +32,7 @@Noncompliant code example
-public static class StringUtils
+public static class StringUtils // Compliant: the class is static
{
public static string Concatenate(string s1, string s2)
{
@@ -45,9 +42,9 @@ Compliant solution
or
-public class StringUtils
+public class StringUtils // Compliant: the constructor is not public
{
- protected StringUtils()
+ private StringUtils()
{
}
@@ -57,12 +54,4 @@ Compliant solution
}
}
-This rule raises an issue when a general or reserved exception is thrown.
Throwing such general exceptions as Exception, SystemException, ApplicationException,
-IndexOutOfRangeException, NullReferenceException, OutOfMemoryException and
-ExecutionEngineException prevents calling methods from handling true, system-generated exceptions differently than application-generated
-errors.
+Throwing general exceptions such as
+Exception,SystemExceptionandApplicationExceptionwill have a negative +impact on any code trying to catch these exceptions.From a consumer perspective, it is generally a best practice to only catch exceptions you intend to handle. Other exceptions should ideally be let +to propagate up the stack trace so that they can be dealt with appropriately. When a general exception is thrown, it forces consumers to catch +exceptions they do not intend to handle, which they then have to re-throw.
+Besides, when working with a general type of exception, the only way to distinguish between multiple exceptions is to check their message, which is +error-prone and difficult to maintain. Legitimate exceptions may be unintentionally silenced and errors may be hidden.
+For instance, if an exception such as
+StackOverflowExceptionis caught and not re-thrown, it may prevent the program from terminating +gracefully.When throwing an exception, it is therefore recommended to throw the most specific exception possible so that it can be handled intentionally by +consumers.
+Additionally, some reserved exceptions should not be thrown manually. Exceptions such as
+IndexOutOfRangeException, +NullReferenceException,OutOfMemoryExceptionorExecutionEngineExceptionwill be thrown automatically by the +runtime when the corresponding error occurs. Many of them indicate serious errors, which the application may not be able to recover from. It is +therefore recommended to avoid throwing them as well as using them as base classes.How to fix it
+To fix this issue, make sure to throw specific exceptions that are relevant to the context in which they arise. It is recommended to either:
+
Exception when one matches. For instance ArgumentException could be raised when an unexpected
+ argument is provided to a function. Exception or one of its subclasses.
public void DoSomething(object obj)
{
if (obj == null)
{
- throw new NullReferenceException("obj"); // Noncompliant
+ throw new NullReferenceException("obj"); // Noncompliant: This reserved exception should not be thrown manually
}
// ...
}
-+Compliant solution
+public void DoSomething(object obj) { if (obj == null) { - throw new ArgumentNullException("obj"); + throw new ArgumentNullException("obj"); // Compliant: this is a specific and non-reserved exception type } // ... }Resources
+Standards
Assignments within sub-expressions are hard to spot and therefore make the code less readable. Ideally, sub-expressions should not have -side-effects.
-
-if (string.IsNullOrEmpty(result = str.Substring(index, length))) // Noncompliant
-{
- //...
-}
-
-A common code smell that can hinder the clarity of source code is making assignments within sub-expressions. This practice involves assigning a +value to a variable inside a larger expression, such as within a loop or a conditional statement.
+This practice essentially gives a side-effect to a larger expression, thus making it less readable. This often leads to confusion and potential +errors.
+Assignments inside lambda and delegate expressions are allowed.
-var result = str.Substring(index, length);
-if (string.IsNullOrEmpty(result))
+var result = Foo(() =>
{
- //...
+ int x = 100; // dead store, but ignored
+ x = 200;
+ return x;
}
-Assignments inside lambda and delegate expressions are allowed.
-Furthermore, the following patterns are also accepted:
+The rule also ignores the following patterns:
+var a = b = c = 10;+
if statement or a loop
while ((val = GetNewValue()) > 0)
{
...
}
+private MyClass instance; -public MyClass Instance +public MyClass Instance => instance ?? (instance = new MyClass()); ++
Making assignments within sub-expressions can hinder the clarity of source code.
+This practice essentially gives a side-effect to a larger expression, thus making it less readable. This often leads to confusion and potential +errors.
+Extracting assignments into separate statements is encouraged to keep the code clear and straightforward.
+
+if (string.IsNullOrEmpty(result = str.Substring(index, length))) // Noncompliant
+{
+ // do something with "result"
+}
+
+
+var result = str.Substring(index, length);
+if (string.IsNullOrEmpty(result))
{
- get
- {
- return instance ?? (instance = new MyClass());
- }
+ // do something with "result"
}
Redundant Boolean literals should be removed from expressions to improve readability.
-+A boolean literal can be represented in two different ways:
+trueorfalse. They can be combined with logical operators +(!, &&, ||, ==, !=) to produce logical expressions that represent truth values. However, comparing a boolean literal to a +variable or expression that evaluates to a boolean value is unnecessary and can make the code harder to read and understand. The more complex a +boolean expression is, the harder it will be for developers to understand its meaning and expected behavior, and it will favour the introduction of +new bugs.How to tix it
+Remove redundant boolean literals from expressions to improve readability and make the code more maintainable.
+Code examples
+Noncompliant code example
+if (booleanMethod() == true) { /* ... */ } if (booleanMethod() == false) { /* ... */ } if (booleanMethod() || false) { /* ... */ } @@ -19,8 +26,8 @@-Noncompliant code example
... }Compliant solution
-+Compliant solution
+if (booleanMethod()) { /* ... */ } if (!booleanMethod()) { /* ... */ } if (booleanMethod()) { /* ... */ } diff --git a/analyzers/rspec/cs/S1128.html b/analyzers/rspec/cs/S1128.html index 58dfbeea0fa..cddb89a40f4 100644 --- a/analyzers/rspec/cs/S1128.html +++ b/analyzers/rspec/cs/S1128.html @@ -1,40 +1,72 @@Why is this an issue?
-Although unnecessary
+usingwon’t change anything to the produced application, removing them:Unnecessary
+usingdirectives refer to importing namespaces, types or creating aliases that are not used or referenced anywhere in the +code.Although they don’t affect the runtime behavior of the application after compilation, removing them will:
Starting with C# 10, it’s possible to define global usings for an entire +project. They reduce the need for repetitive namespace inclusions, but can also mask which namespaces are truly necessary for the code at hand. +Over-relying on them can lead to less transparent code dependencies, especially for newcomers to the project.
+The rule will not raise a warning for global using directives, even if none of the types of that namespace are used in the
+project:
-using System.Collections.Generic; // Noncompliant - unnecessary using +global using System.Net.Sockets; // Compliant by exception ++
Unnecessary using directives are ignored in ASP.NET Core projects in the following files:
_Imports.razor _ViewImports.cshtml While it’s not difficult to remove these unneeded lines manually, modern code editors support the removal of every unnecessary using
+directive with a single click from every file of the project.
+using System.IO;
+using System.Linq;
+using System.Collections.Generic; // Noncompliant - no types are used from this namespace
+using MyApp.Helpers; // Noncompliant - FileHelper is in the same namespace
+using MyCustomNamespace; // Noncompliant - no types are used from this namespace
-namespace Foo
+namespace MyApp.Helpers
{
- public class Bar
+ public class FileHelper
{
- public Bar(string path)
- {
- File.ReadAllLines(path);
- }
+ public static string ReadFirstLine(string filePath) =>
+ File.ReadAllLines(filePath).First();
}
}
-++Compliant solution
+using System.IO; +using System.Linq; -namespace Foo +namespace MyApp.Helpers { - public class Bar + public class FileHelper { - public Bar(string path) - { - File.ReadAllLines(path); - } + public static string ReadFirstLine(string filePath) => + File.ReadAllLines(filePath).First(); } }+Resources
+Documentation
+
Developers often use TOOO tags to mark areas in the code where additional work or improvements are needed but are not implemented
+
Developers often use TODO tags to mark areas in the code where additional work or improvements are needed but are not implemented
immediately. However, these TODO tags sometimes get overlooked or forgotten, leading to incomplete or unfinished code. This code smell
class aims to identify and address such unattended TODO tags to ensure a clean and maintainable codebase. This description will explore
why this is a problem and how it can be fixed to improve the overall code quality.
This rule raises an issue when a private/internal type or member is never referenced in the code.
private or internal types or private members that are never executed or referenced are unused code:
-unnecessary, inoperative code that should be removed.
Cleaning out the unused code decreases the codebase size, making it easier to understand and preventing bugs from being introduced.
-Redundant code is included in the compilation so it needs to be compiled as well. Due to this, removing it will reduce the compilation time and the -project maintanace. It will also simplify the onboarding time for new joiners since they will not need to understand what it does and why it’s -there.
-This rule doesn’t raise issues on:
-Main method void Foo(object, EventArgs) that are declared in partial class A type or member that is never called is dead code, and should be removed. Cleaning out dead code decreases the size of the maintained codebase, +making it easier to understand the program and preventing bugs from being introduced.
+This rule detects type or members that are never referenced from inside a translation unit, and cannot be referenced from the outside.
@@ -45,6 +30,18 @@+Compliant solution
private class UsedClass {...} }
This rule doesn’t raise issues on:
+Main method of the application void Foo(object, EventArgs) that are declared in partial class Using .Count() to test for emptiness works, but using .Any() makes the intent clearer, and the code more readable.
-However, there are some cases where special attention should be paid:
When you call Any(), it clearly communicates the code’s intention, which is to check if the collection is empty. Using Count()
+== 0 for this purpose is less direct and makes the code slightly more complex. However, there are some cases where special attention should be
+paid:
EntityFramework or other ORM query, calling .Count() will cause executing a potentially
- massive SQL query and could put a large overhead on the application database. Calling .Any() will also connect to the database, but
- will generate much more efficient SQL. .Select() statements that create objects, a large amount of memory could
- be unnecessarily allocated. Calling .Any() will be much more efficient because it will execute fewer iterations of the enumerable.
- EntityFramework or other ORM query, calling Count() will cause executing a potentially
+ massive SQL query and could put a large overhead on the application database. Calling Any() will also connect to the database, but will
+ generate much more efficient SQL. Select() statements that create objects, a large amount of memory could be
+ unnecessarily allocated. Calling Any() will be much more efficient because it will execute fewer iterations of the enumerable. +private static bool HasContent(IEnumerable<string> strings) { return strings.Count() > 0; // Noncompliant @@ -26,8 +25,8 @@-Noncompliant code example
return strings.Count() == 0; // Noncompliant }Compliant solution
-+Prefer using
+Any()to test for emptiness overCount().private static bool HasContent(IEnumerable<string> strings) { return strings.Any(); diff --git a/analyzers/rspec/cs/S1172.html b/analyzers/rspec/cs/S1172.html index ca41e99c468..06a456d1413 100644 --- a/analyzers/rspec/cs/S1172.html +++ b/analyzers/rspec/cs/S1172.html @@ -1,21 +1,39 @@Why is this an issue?
-Unused parameters are misleading. Whatever the values passed to such parameters, the behavior will be the same.
+A typical code smell known as unused function parameters refers to parameters declared in a function but not used anywhere within the function’s +body. While this might seem harmless at first glance, it can lead to confusion and potential errors in your code. Disregarding the values passed to +such parameters, the function’s behavior will be the same, but the programmer’s intention won’t be clearly expressed anymore. Therefore, removing +function parameters that are not being utilized is considered best practice.
This rule raises an issue when a
-privatemethod or constructor of a class/struct takes a parameter without using it.Noncompliant code example
--private void DoSomething(int a, int b) // "b" is unused +Exceptions
+This rule doesn’t raise any issue in the following contexts:
+
this parameter of extension methods. NotImplementedException. virtual, override methods. Having unused function parameters in your code can lead to confusion and misunderstanding of a developer’s intention. They reduce code readability +and introduce the potential for errors. To avoid these problems, developers should remove unused parameters from function declarations.
+
+private void DoSomething(int a, int b) // Noncompliant, "b" is unused
{
Compute(a);
}
-private void DoSomething2(int a) // value of "a" is unused
+private void DoSomething2(int a) // Noncompliant, the value of "a" is unused
{
a = 10;
Compute(a);
}
-+Compliant solution
+private void DoSomething(int a) { Compute(a); @@ -27,15 +45,4 @@-Compliant solution
Compute(a); }Exceptions
-This rule doesn’t raise any issue in the following contexts:
-
this parameter of extension methods. NotImplementedException. virtual, override methods. An empty method is generally considered bad practice and can lead to confusion, readability, and maintenance issues. Empty methods bring no functionality and are misleading to others as they might think the method implementation fulfills a specific and identified requirement.
-Such methods should be avoided and possibly removed.
-
-void DoSomething() { } // Noncompliant
-
-However, there are some cases where a method needs to be empty. In those cases, it is essential to minimize confusion and enhance clarity.
-Here are a few examples:
+There are several reasons for a method not to have a body:
NotSupportedException. -void DoSomething() => // Compliant - throw new NotSupportedException(); -
NotImplementedException. -void DoSomething() => // Compliant - throw new NotImplementedException(); -
-void DoSomething() // Compliant
-{
- // Do nothing because of X
-}
- The following empty methods are considered compliant:
@@ -30,4 +14,39 @@abstract method as the implementation is mandatory for child class
+public void ShouldNotBeEmpty() { // Noncompliant - method is empty
+}
+
+public void NotImplementedYet() { // Noncompliant - method is empty
+}
+
+public void WillNeverBeImplemented() { // Noncompliant - method is empty
+}
+
+public void EmptyOnPurpose() { // Noncompliant - method is empty
+}
+
+
+public void ShouldNotBeEmpty() {
+ DoSomething();
+}
+
+public void NotImplementedYet() {
+ throw new NotImplementedException();
+}
+
+public void WillNeverBeImplemented() {
+ throw new NotSupportedException();
+}
+
+public void EmptyOnPurpose() {
+ // comment explaining why the method is empty
+}
+
+Duplicated string literals make the process of refactoring error-prone, since you must be sure to update all occurrences.
-On the other hand, constants can be referenced from many places, but only need to be updated in a single place.
-+Duplicated string literals make the process of refactoring complex and error-prone, as any change would need to be propagated on all +occurrences.
+Exceptions
+The following are ignored:
+
Instead, use constants to replace the duplicated string literals. Constants can be referenced from many places, but only need to be updated in a +single place.
+
public class Foo
{
private string name = "foobar"; // Noncompliant
@@ -15,8 +26,8 @@ Noncompliant code example
}
}
-+Compliant solution
+public class Foo { private const string Foobar = "foobar"; @@ -31,11 +42,4 @@-Compliant solution
} }Exceptions
-The following are ignored:
-
Nested code blocks can be used to create a new scope and restrict the visibility of the variables defined inside it. Using this feature in a method -typically indicates that the method has too many responsibilities, and should be refactored into smaller methods.
-+Nested code blocks create new scopes where variables declared within are inaccessible from the outside, and their lifespan ends with the block.
+Although this may appear beneficial, their usage within a function often suggests that the function is overloaded. Thus, it may violate the Single +Responsibility Principle, and the function needs to be broken down into smaller functions.
+The presence of nested blocks that don’t affect the control flow might suggest possible mistakes in the code.
+Exceptions
+The usage of a code block after a
+caseis allowed.How to fix it
+The nested code blocks should be extracted into separate methods.
+Code examples
+Noncompliant code example
+public void Evaluate() { /* ... */ @@ -15,8 +22,8 @@-Noncompliant code example
/* ... */ }Compliant solution
-+Compliant solution
+public void Evaluate() { /* ... */ @@ -32,6 +39,9 @@-Compliant solution
stack.push(result); }Exceptions
-The usage of a code block after a "case" is allowed for this rule.
+Resources
+Documentation
+
Programmers should not comment out code as it bloats programs and reduces readability.
-Unused code should be deleted and can be retrieved from source control history if required.
+Commented-out code distracts the focus from the actual executed code. It creates a noise that increases maintenance code. And because it is never +executed, it quickly becomes out of date and invalid.
+Commented-out code should be deleted and can be retrieved from source control history if required.
diff --git a/analyzers/rspec/cs/S134.html b/analyzers/rspec/cs/S134.html index 29f8145a956..766896df1d2 100644 --- a/analyzers/rspec/cs/S134.html +++ b/analyzers/rspec/cs/S134.html @@ -1,22 +1,27 @@Nested if, switch, for, foreach, while, do, and try
-statements are key ingredients for making what’s known as "Spaghetti code".
Such code is hard to read, refactor and therefore maintain.
-With the default threshold of 3:
--if (condition1) // Compliant - depth = 1 +Nested control flow statements
+if,switch,for,foreach,while,do, +andtryare often key ingredients in creating what’s known as "Spaghetti code". This code smell can make your program difficult to +understand and maintain.When numerous control structures are placed inside one another, the code becomes a tangled, complex web. This significantly reduces the code’s +readability and maintainability, and it also complicates the testing process.
+How to fix it
+Code examples
+The following example demonstrates the behavior of the rule with the default threshold of 3 levels of nesting and one of the potential ways to fix +the code smell by introducing guard clauses:
+Noncompliant code example
++if (condition1) // Compliant - depth = 1 { /* ... */ - if (condition2) // Compliant - depth = 2 + if (condition2) // Compliant - depth = 2 { /* ... */ - for(int i = 0; i < 10; i++) // Compliant - depth = 3, not exceeding the limit + for (int i = 0; i < 10; i++) // Compliant - depth = 3 { /* ... */ - if (condition4) // Noncompliant - depth = 4 + if (condition4) // Noncompliant - depth = 4, which exceeds the limit { - if (condition5) // Depth = 5, exceeding the limit, but issues are only reported on depth = 4 + if (condition5) // Depth = 5, exceeding the limit, but issues are only reported on depth = 4 { /* ... */ } @@ -26,4 +31,33 @@+Noncompliant code example
} }Compliant solution
++if (!condition1) +{ + return; +} +/* ... */ +if (!condition2) +{ + return; +} +for (int i = 0; i < 10; i++) +{ + /* ... */ + if (condition4) + { + if (condition5) + { + /* ... */ + } + return; + } +} ++Resources
+
If a local variable is declared but not used, it is dead code and should be removed. Doing so will improve maintainability because developers will -not wonder what the variable is used for.
-An unused local variable is a variable that has been declared but is not used anywhere in the block of code where it is defined. It is dead code, +contributing to unnecessary complexity and leading to confusion when reading the code. Therefore, it should be removed from your code to maintain +clarity and efficiency.
+Having unused local variables in your code can lead to several issues:
+In summary, unused local variables can make your code less readable, more confusing, and harder to maintain, and they can potentially lead to bugs +or inefficient memory use. Therefore, it is best to remove them.
+Unused locally created resources in a using statement are not reported.
-public int NumberOfMinutes(int hours)
+using(var t = new TestTimer()) // t never used, but compliant.
{
- int seconds = 0; // seconds is never used
- return hours * 60;
+ //...
}
-+How to fix it
+The fix for this issue is straightforward. Once you ensure the unused variable is not part of an incomplete implementation leading to bugs, you +just need to remove it.
+Code examples
+Noncompliant code example
+public int NumberOfMinutes(int hours) { + int seconds = 0; // Noncompliant - seconds is unused return hours * 60; }-Exceptions
-Unused locally created resources in a
-usingstatement are not reported.-using(var t = new TestTimer()) // t never used, but compliant. +Compliant solution
++public int NumberOfMinutes(int hours) { - //... + return hours * 60; }diff --git a/analyzers/rspec/cs/S1854.html b/analyzers/rspec/cs/S1854.html index 78110b06975..dd6c4c1031d 100644 --- a/analyzers/rspec/cs/S1854.html +++ b/analyzers/rspec/cs/S1854.html @@ -1,28 +1,50 @@Why is this an issue?
-A dead store happens when a local variable is assigned a value that is not read by any subsequent instruction. Calculating or retrieving a value -only to then overwrite it or throw it away, could indicate a serious error in the code. Even if it’s not an error, it is at best a waste of resources. -Therefore all calculated values should be used.
-Noncompliant code example
--i = a + b; // Noncompliant; calculation result not used before value is overwritten -i = compute(); --Compliant solution
--i = a + b; -i += compute(); -+Dead stores refer to assignments made to local variables that are subsequently never used or immediately overwritten. Such assignments are +unnecessary and don’t contribute to the functionality or clarity of the code. They may even negatively impact performance. Removing them enhances code +cleanliness and readability. Even if the unnecessary operations do not do any harm in terms of the program’s correctness, they are - at best - a waste +of computing resources.
Exceptions
No issue is reported when
try blocks, try blocks -1, 0, 1, null, true, false, ""
- and string.Empty. -1, 0, 1, null, true, false,
+ "" or string.Empty Remove the unnecesarry assignment, then test the code to make sure that the right-hand side of a given assignment had no side effects (e.g. a +method that writes certain data to a file and returns the number of written bytes).
+You can also use discards (rather than a variable) +to express that result of a method call is ignored on purpose.
+
+int Foo(int y)
+{
+ int x = 100; // Noncompliant: dead store
+ x = 150; // Noncompliant: dead store
+ x = 200;
+ return x + y;
+}
+
+
+int Foo(int y)
+{
+ int x = 200; // Compliant: no unnecessary assignment
+ return x + y;
+}
+
Having two cases in the same switch statement or branches in the same if structure with the same
-implementation is at best duplicate code, and at worst a coding error. If the same logic is truly needed for both instances, then in an
-if structure they should be combined, or for a switch, one should fall through to the other.
+When the same code is duplicated in two or more separate branches of a conditional, it can make the code harder to understand, maintain, and can +potentially introduce bugs if one instance of the code is changed but others are not.
+Having two
+casesin aswitchstatement or two branches in anifchain with the same implementation is at +best duplicate code, and at worst a coding error.+if (a >= 0 && a < 10) +{ + DoFirst(); + DoTheThing(); +} +else if (a >= 10 && a < 20) +{ + DoTheOtherThing(); +} +else if (a >= 20 && a < 50) // Noncompliant; duplicates first condition +{ + DoFirst(); + DoTheThing(); +} ++switch (i) { case 1: @@ -20,8 +36,13 @@+Noncompliant code example
default: DoTheRest(); } - -if (a >= 0 && a < 10) +If the same logic is truly needed for both instances, then:
+
if chain they should be combined
+if ((a >= 0 && a < 10) || (a >= 20 && a < 50))
{
DoFirst();
DoTheThing();
@@ -30,10 +51,23 @@ Noncompliant code example
{
DoTheOtherThing();
}
-else if (a >= 20 && a < 50) // Noncompliant; duplicates first condition
+
+switch, one should fall through to the other
+switch (i)
{
- DoFirst();
- DoTheThing();
+ case 1:
+ case 3:
+ DoFirst();
+ DoSomething();
+ break;
+ case 2:
+ DoSomethingDifferent();
+ break;
+ default:
+ DoTheRest();
}
But this exception does not apply to if chains without else-s, or to switch-es without default clauses when
-all branches have the same single line of code. In case of if chains with else-s, or of switch-es with default
-clauses, rule {rule:csharpsquid:S3923} raises a bug.
if chains with else-s, or of switch-es with
+default clauses, rule {rule:csharpsquid:S3923} raises a bug.
if(a == 1)
{
@@ -66,4 +100,9 @@ Exceptions
doSomething();
}
+Unnecessary casting expressions make the code harder to read and understand.
-+Casting expressions are utilized to convert one data type to another, such as transforming an integer into a string. This is especially crucial in +strongly typed languages like C, C++, C#, Java, Python, and others.
+However, there are instances where casting expressions are not needed. These include situations like:
+
These scenarios are considered unnecessary casting expressions. They can complicate the code and make it more difficult to understand, without +offering any advantages.
+As a result, it’s generally advised to avoid unnecessary casting expressions. Instead, rely on the language’s type system to ensure type safety and +code clarity.
+Issues are not raised against the default literal.
+To fix your code remove the unnecessary casting expression.
+
public int Example(int i)
{
return (int) (i + 42); // Noncompliant
@@ -12,8 +29,8 @@ Noncompliant code example
return coll.Reverse().OfType<int>(); // Noncompliant
}
-+Compliant solution
+public int Example(int i) { return i + 42; @@ -24,9 +41,16 @@-Compliant solution
return coll.Reverse(); }Exceptions
-Issues are not raised against C# 7.1
defaultliteral.bool b = (bool)default; // Doesn't raise an issue+Resources
+Documentation
+
To fix the issue the access of the null value needs to be prevented by either:
To fix the issue, the access of the null value needs to be prevented by either:
null Gratuitous boolean expressions are conditions that do not change the evaluation of a program. This issue can indicate logical errors and affect the +correctness of an application, as well as its maintainability.
An operand of a boolean expression that never changes the result of the expression might not match the programmer’s intent and can lead to -unexpected behavior and potential bugs.
-
-var a = true;
-if (a)
-{
- DoSomething();
-}
-
-This also applies to the null
-coalescing operator when one of the operands always evaluates to null.
Control flow constructs like if-statements allow the programmer to direct the flow of a program depending on a boolean expression.
+However, if the condition is always true or always false, only one of the branches will ever be executed. In that case, the control flow construct and
+the condition no longer serve a purpose; they become gratuitous.
The presence of gratuitous conditions can indicate a logical error. For example, the programmer intended to have the program branch into +different paths but made a mistake when formulating the branching condition. In this case, this issue might result in a bug and thus affect the +reliability of the application. For instance, it might lead to the computation of incorrect results.
+Additionally, gratuitous conditions and control flow constructs introduce unnecessary complexity. The source code becomes harder to understand, and +thus, the application becomes more difficult to maintain.
+This rule looks for operands of a boolean expression never changing the result of the expression. It also applies to the null coalescing operator when one of
+the operands always evaluates to null.
string d = null; -var v1 = d ?? "value"; +var v1 = d ?? "value"; // Noncompliant
This rule will not raise an issue in either of these cases:
@@ -20,7 +23,7 @@const bool
const bool debug = false;
//...
-if (debug)
+if (debug) // Compliant
{
// Print something
}
@@ -29,11 +32,11 @@ Exceptions
In these cases, it is obvious the code is as intended.
How to fix it
-The conditions should be reviewed to decide whether:
-Gratuitous boolean expressions are suspicious and should be carefully removed from the code.
+First, the boolean expression in question should be closely inspected for logical errors. If a mistake was made, it can be corrected so the +condition is no longer gratuitous.
+If it becomes apparent that the condition is actually unnecessary, it can be removed. The associated control flow construct (e.g., the
+if-statement containing the condition) will be adapted or even removed, leaving only the necessary branches.
diff --git a/analyzers/rspec/cs/S2612.html b/analyzers/rspec/cs/S2612.html index 8f2091c5578..52b947c6046 100644 --- a/analyzers/rspec/cs/S2612.html +++ b/analyzers/rspec/cs/S2612.html @@ -66,6 +66,5 @@See
href="https://owasp.org/www-project-web-security-testing-guide/latest/4-Web_Application_Security_Testing/02-Configuration_and_Deployment_Management_Testing/09-Test_File_Permission">OWASP File Permission
readonly fields can only be assigned in a class constructor. If a class has a field that’s not marked readonly but is
only set in the constructor, it could cause confusion about the field’s intended use. To avoid confusion, such fields should be marked
readonly to make their intended use explicit, and to prevent future maintainers from inadvertently changing their use.
+Exceptions
+
Serializable attribute. partial classes. struct that are not primitive or pointer types are also ignored because of possible unwanted behavior. Mark the given field with the readonly modifier.
public class Person
{
- private int _birthYear; // Noncompliant
+ private int _birthYear; // Noncompliant
Person(int birthYear)
{
@@ -14,8 +24,8 @@ Noncompliant code example
}
}
-+Compliant solution
+public class Person { private readonly int _birthYear; @@ -26,13 +36,10 @@-Compliant solution
} }Exceptions
-
struct that are not primitive or pointer types are also ignored because of possible unwanted behavior. In the interests of readability, code that can be simplified should be simplified. To that end, there are several ways IEnumerable
-language integrated queries (LINQ) can be simplified
In the interests of readability, code that can be simplified should be simplified. To that end, there are several ways IEnumerable language integrated queries (LINQ) can be +simplified. This not only improves readabilty but can also lead to improved performance.
+Simplify the LINQ expressions:
OfType instead of using Select with as to type cast elements and then null-checking in a query
- expression to choose elements based on type. OfType instead of using Where and the is operator, followed by a cast in a Select Any instead of Where(element => [expression]).Any(). Count instead of Count() when it’s available. ToArray() or ToList() in the middle of a query chain. OfType instead of using Where and the
+ is operator, followed
+ by a cast in a Select Any instead of Where(element ⇒ [expression]).Any(). Using EntityFramework may require enforcing client evaluations. Such queries should use AsEnumerable() instead of
-ToArray() or ToList() in the middle of a query chain.
-seq1.Select(element => element as T).Any(element => element != null); // Noncompliant; use OfType -seq2.Select(element => element as T).Any(element => element != null && CheckCondition(element)); // Noncompliant; use OfType -seq3.Where(element => element is T).Select(element => element as T); // Noncompliant; use OfType -seq4.Where(element => element is T).Select(element => (T)element); // Noncompliant; use OfType -seq5.Where(element => [expression]).Any(); // Noncompliant; use Any([expression]) +diff --git a/analyzers/rspec/cs/S2971.json b/analyzers/rspec/cs/S2971.json index 8b465931a29..6d9dd0e7857 100644 --- a/analyzers/rspec/cs/S2971.json +++ b/analyzers/rspec/cs/S2971.json @@ -1,5 +1,5 @@ { - "title": "\"IEnumerable\" LINQs should be simplified", + "title": "LINQ expressions should be simplified", "type": "CODE_SMELL", "code": { "impacts": { diff --git a/analyzers/rspec/cs/S3257.html b/analyzers/rspec/cs/S3257.html index af785bdc113..0f74e78da63 100644 --- a/analyzers/rspec/cs/S3257.html +++ b/analyzers/rspec/cs/S3257.html @@ -1,6 +1,11 @@Using Entity Framework may require enforcing client evaluations. Such queries should use AsEnumerable() instead of
+ToArray()or +ToList()in the middle of a query chain.Code examples
+Noncompliant code example
++public void Foo(IEnumerable<Vehicle> seq, List<int> list) +{ + var result1 = seq.Select(x => x as Car).Any(x => x != null); // Noncompliant; use OfType + var result2 = seq.Select(x => x as Car).Any(x => x != null && x.HasOwner); // Noncompliant; use OfType before calling Any + var result3 = seq.Where(x => x is Car).Select(x => x as Car); // Noncompliant; use OfType + var result4 = seq.Where(x => x is Car).Select(x => (Car)x); // Noncompliant; use OfType + var result5 = seq.Where(x => x.HasOwner).Any(); // Noncompliant; use Any([predicate]) -var num = seq6.Count(); // Noncompliant -var arr = seq.ToList().ToArray(); //Noncompliant -var count = seq.ToList().Count(x=>[condition]); //Noncompliant + var num = list.Count(); // Noncompliant; use the Count property + var arr = seq.ToList().ToArray(); // Noncompliant; ToList is not needed + var count = seq.ToList().Count(x => x.HasOwner); // Noncompliant; ToList is not needed +}-Compliant solution
--seq1.OfType<T>().Any(); -seq2.OfType<T>().Any(element => CheckCondition(element)); -seq3.OfType<T>(); -seq4.OfType<T>(); -seq5.Any(element => [expression]) +Compliant solution
++public void Foo(IEnumerable<Vehicle> seq, List<int> list) +{ + var result1 = seq.OfType<Car>().Any(); + var result2 = seq.OfType<Car>().Any(x => x.HasOwner); + var result3 = seq.OfType<Car>(); + var result4 = seq.OfType<Car>(); + var result5 = seq.Any(x => x.HasOwner); -var num = seq6.Count; -var arr = seq.ToArray(); -var count = seq.Count(x=>[condition]); + var num = list.Count; + var arr = seq.ToArray(); + var count = seq.Count(x => x.HasOwner); +}+Resources
+Documentation
+
Unnecessarily verbose declarations and initializations make it harder to read the code, and should be simplified.
-Specifically the following should be omitted when they can be inferred:
+In C#, the type of a variable can often be inferred by the compiler. The use of the [var keyword](https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/implicitly-typed-local-variables) +allows you to avoid repeating the type name in a variable declaration and object instantiation because the declared type can often be inferred by the +compiler.
+Additionally, initializations providing the default value can also be omitted, helping to make the code more concise and readable.
+Unnecessarily verbose declarations and initializations should be simplified. Specifically, the following should be omitted when they can be +inferred:
+How to fix it
+Remove any unneeded code. C# provides many features designed to help you write more concise code.
+Code examples
+Noncompliant code example
+var l = new List<int>() {}; // Noncompliant, {} can be removed var o = new object() {}; // Noncompliant, {} can be removed @@ -33,8 +41,8 @@-Noncompliant code example
} }Compliant solution
-+Compliant solution
+var l = new List<int>(); var o = new object(); @@ -56,4 +64,10 @@+Compliant solution
} }Resources
+Documentation
+
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.
+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
+constcan lead to performance improvements. The compiler might be able to make optimizations knowing that the +value of aconstvariable will not change.How to fix it
+Mark the given variable with the
+constmodifier.Code examples
+Noncompliant code example
+public bool Seek(int[] input) { var target = 32; // Noncompliant @@ -16,18 +23,8 @@-Noncompliant code example
return false; }or
--public class Sample -{ - public void Method() - { - var context = $"{nameof(Sample)}.{nameof(Method)}"; // Noncompliant (C# 10 and above only) - } -} --Compliant solution
-+Compliant solution
+public bool Seek(int[] input) { const int target = 32; @@ -41,8 +38,18 @@-Compliant solution
return false; }or
-+Noncompliant code example
++public class Sample +{ + public void Method() + { + var context = $"{nameof(Sample)}.{nameof(Method)}"; // Noncompliant (C# 10 and above only) + } +} ++Compliant solution
+public class Sample { public void Method() @@ -51,4 +58,9 @@+Compliant solution
} }Resources
+Documentation
+
The standard assertions library methods such as AreEqual and AreSame in MSTest and
NUnit, or Equal and Same in XUnit, expect the first argument to be the expected value and
-the second argument to be the actual value. Swap them, and your test will still have the same outcome (succeed/fail when it should) but the error
-messages will be confusing.
This rule raises an issue when the second argument to an assertions library method is a hard-coded value and the first argument is not.
-+the second argument to be the actual value. +What is the potential impact?
+Having the expected value and the actual value in the wrong order will not alter the outcome of tests, (succeed/fail when it should) but the error +messages will contain misleading information.
+This rule raises an issue when the actual argument to an assertions library method is a hard-coded value and the expected argument is not.
+How to fix it
+You should provide the assertion methods with a hard-coded value as the expected value, while the actual value of the assertion should derive from +the portion of code that you want to test.
+Code examples
+Noncompliant code example
+Assert.AreEqual(runner.ExitCode, 0, "Unexpected exit code"); // Noncompliant; Yields error message like: Expected:<-1>. Actual:<0>.-Compliant solution
-+Compliant solution
+Assert.AreEqual(0, runner.ExitCode, "Unexpected exit code");diff --git a/analyzers/rspec/cs/S3457.html b/analyzers/rspec/cs/S3457.html index 15bd19a1e34..f43bf48ad3a 100644 --- a/analyzers/rspec/cs/S3457.html +++ b/analyzers/rspec/cs/S3457.html @@ -1,38 +1,78 @@Why is this an issue?
+A [composite format string](https://learn.microsoft.com/en-us/dotnet/standard/base-types/composite-formatting) +is a string that contains placeholders, represented by indices inside curly braces "{0}", "{1}", etc. These placeholders are replaced by values when +the string is printed or logged.
Because composite format strings are interpreted at runtime, rather than validated by the compiler, they can contain errors that lead to unexpected -behaviors or runtime errors. This rule statically validates the good behavior of composite formats when calling the methods of -
-String.Format,StringBuilder.AppendFormat,Console.Write,Console.WriteLine, -TextWriter.Write,TextWriter.WriteLine,Debug.WriteLine(String, Object[]), -Trace.TraceError(String, Object[]),Trace.TraceInformation(String, Object[]), -Trace.TraceWarning(String, Object[])andTraceSource.TraceInformation(String, Object[]).Noncompliant code example
--s = string.Format("{0}", arg0, arg1); // Noncompliant, arg1 is declared but not used. -s = string.Format("{0} {2}", arg0, arg1, arg2); // Noncompliant, the format item with index 1 is missing so arg1 will not be used. -s = string.Format("foo"); // Noncompliant, there is no need to use string.Format here. --Compliant solution
--s = string.Format("{0}", arg0); -s = string.Format("{0} {1}", arg0, arg2); -s = "foo"; -+behaviors or runtime errors. +This rule validates the correspondence between arguments and composite formats when calling the following methods:
+
String.Format StringBuilder.AppendFormat
+ Console.Write Console.WriteLine TextWriter.Write TextWriter.WriteLine Debug.WriteLine(String, Object[)] Trace.TraceError(String, Object[)] Trace.TraceInformation(String, Object[)] Trace.TraceWarning(String, Object[)] TraceSource.TraceInformation(String, Object[)] const.
var pattern = "{0} {1} {2}";
-var res = string.Format(pattern, 1, 2); // Compliant, not const string are not recognized
+var res = string.Format(pattern, 1, 2); // Incorrect, but the analyzer doesn't raise any warnings here
var array = new int[] {};
-var res = string.Format("{0} {1}", array); // Compliant we don't know the size of the array
+var res = string.Format("{0} {1}", array); // Compliant; we don't know the size of the array
:) is actually valid. A composite format string contains placeholders, replaced by values when the string is printed or logged. Mismatch in the format specifiers and the +arguments provided can lead to incorrect strings being created.
+To avoid issues, a developer should ensure that the provided arguments match format specifiers.
+Moreover, use string interpolation when possible.
+Instead of
+
+string str = string.Format("Hello {0} {1}!", firstName, lastName);
+
+use
+
+string str = $"Hello {firstName} {lastName}!";
+
+With string interpolation:
+
+s = string.Format("{0}", arg0, arg1); // Noncompliant, arg1 is declared but not used.
+s = string.Format("{0} {2}", arg0, arg1, arg2); // Noncompliant, the format item with index 1 is missing, so arg1 will not be used.
+s = string.Format("foo"); // Noncompliant; there is no need to use "string.Format" here.
+
+
+s = string.Format("{0}", arg0);
+s = string.Format("{0} {1}", arg0, arg2);
+s = "foo";
+
diff --git a/analyzers/rspec/cs/S3457.json b/analyzers/rspec/cs/S3457.json
index 0b65de9bdf4..f4de833e146 100644
--- a/analyzers/rspec/cs/S3457.json
+++ b/analyzers/rspec/cs/S3457.json
@@ -10,7 +10,7 @@
"status": "ready",
"remediation": {
"func": "Constant\/Issue",
- "constantCost": "10min"
+ "constantCost": "1min"
},
"tags": [
"confusing"
diff --git a/analyzers/rspec/cs/S3776.html b/analyzers/rspec/cs/S3776.html
index c1e8b5f9254..628d1ec0b6a 100644
--- a/analyzers/rspec/cs/S3776.html
+++ b/analyzers/rspec/cs/S3776.html
@@ -1,53 +1,233 @@
+This rule raises an issue when the code cognitive complexity of a function is above a certain threshold.
Cognitive Complexity Complexity is a measure of how hard the control flow of -a method is to understand.
-Methods with high Cognitive Complexity will be difficult to maintain.
+Cognitive Complexity is a measure of how hard it is to understand the control flow of a unit of code. Code with high cognitive complexity is hard +to read, understand, test, and modify.
+As a rule of thumb, high cognitive complexity is a sign that the code should be refactored into smaller, easier-to-manage pieces.
+Here are the core concepts:
+The method of computation is fully detailed in the pdf linked in the resources.
+Developers spend more time reading and understanding code than writing it. High cognitive complexity slows down changes and increases the cost of +maintenance.
+Reducing cognitive complexity can be challenging.
Here are a few suggestions:
.? or ?? operator
+ replaces multiple tests and simplifies the flow. Extraction of a complex condition in a new function.
+The code is using a complex condition and has a cognitive cost of 3.
-int Abs(int n) // Noncompliant: cognitive complexity = 5
+decimal CalculateFinalPrice(User user, Cart cart)
{
- if (n >= 0) // +1
+ decimal total = CalculateTotal(cart);
+ if (user.HasMembership() // +1 (if)
+ && user.OrdersCount > 10 // +1 (more than one condition)
+ && user.AccountActive
+ && !user.HasDiscount
+ || user.OrdersCount == 1) // +1 (change of operator in condition)
{
- return n;
+
+ total = ApplyDiscount(user, total);
}
- else // +2, due to nesting
+ return total;
+}
+
+Even if the cognitive complexity of the whole program did not change, it is easier for a reader to understand the code of the
+calculateFinalPrice function, which now only has a cognitive cost of 1.
+decimal CalculateFinalPrice(User user, Cart cart)
+{
+ decimal total = CalculateTotal(cart);
+ if (IsEligibleForDiscount(user)) // +1 (if)
+ {
+ total = applyDiscount(user, total);
+ }
+ return total;
+}
+
+bool IsEligibleForDiscount(User user)
+{
+ return user.HasMembership()
+ && user.OrdersCount > 10 // +1 (more than one condition)
+ && user.AccountActive
+ && !user.HasDiscount
+ || user.OrdersCount == 1; // +1 (change of operator in condition)
+}
+
+Break down large functions.
+For example, consider a function that calculates the total price of a shopping cart, including sales tax and shipping.
Note: The code
+is simplified here, to illustrate the purpose. Please imagine there is more happening in the foreach loops.
+decimal CalculateTotal(Cart cart)
+{
+ decimal total = 0;
+ foreach (Item item in cart.Items) // +1 (foreach)
{
- if (n == int.MinValue) // +1
+ total += item.Price;
+ }
+
+ // calculateSalesTax
+ foreach (Item item in cart.Items) // +1 (foreach)
+ {
+ total += 0.2m * item.Price;
+ }
+
+ //calculateShipping
+ total += 5m * cart.Items.Count;
+
+ return total;
+}
+
+This function could be refactored into smaller functions: The complexity is spread over multiple functions and the complex
+CalculateTotal has now a complexity score of zero.
+decimal CalculateTotal(Cart cart)
+{
+ decimal total = 0;
+ total = CalculateSubtotal(cart, total);
+ total += CalculateSalesTax(cart, total);
+ total += CalculateShipping(cart, total);
+ return total;
+}
+
+decimal CalculateSubtotal(Cart cart, decimal total)
+{
+ foreach (Item item in cart.Items) // +1 (foreach)
+ {
+ total += item.Price;
+ }
+
+ return total;
+}
+
+decimal CalculateSalesTax(Cart cart, decimal total)
+{
+ foreach (Item item in cart.Items) // +1 (foreach)
+ {
+ total += 0.2m * item.Price;
+ }
+
+ return total;
+}
+
+decimal CalculateShipping(Cart cart, decimal total)
+{
+ total += 5m * cart.Items.Count;
+ return total;
+}
+
+Avoid deep nesting by returning early.
+The below code has a cognitive complexity of 6.
+
+decimal CalculateDiscount(decimal price, User user)
+{
+ if (IsEligibleForDiscount(user)) // +1 ( if )
+ {
+ if (user.HasMembership()) // +2 ( nested if )
{
- throw new ArgumentException("The absolute value of int.MinValue is outside of int boundaries");
+ return price * 0.9m;
}
- else // +1
+ else if (user.OrdersCount == 1) // +1 ( else )
{
- return -n;
+ return price * 0.95m;
}
+ else // +1 ( else )
+ {
+ return price;
+ }
+ }
+ else // +1 ( else )
+ {
+ return price;
}
}
-They should be refactored to have lower complexity:
--int Abs(int n) // Compliant: cognitive complexity = 3 +Compliant solution
+Checking for the edge case first flattens the
+ifstatements and reduces the cognitive complexity to 3.+decimal CalculateDiscount(decimal price, User user) { - if (n == int.MinValue) // +1 + if (!IsEligibleForDiscount(user)) // +1 ( if ) + { + return price; + } + + if (user.HasMembership()) // +1 ( if ) { - throw new ArgumentException("The absolute value of int.MinValue is outside of int boundaries"); + return price * 0.9m; } - else if (n >= 0) // +1 + + if (user.OrdersCount == 1) // +1 ( else ) + { + return price * 0.95m; + } + + return price; +} ++Use the null-conditional operator to access data.
+In the below code, the cognitive complexity is increased due to the multiple checks required to access the manufacturer’s name. This can be +simplified using the optional chaining operator.
+Noncompliant code example
++string GetManufacturerName(Product product) +{ + string manufacturerName = null; + if (product != null && product.Details != null && + product.Details.Manufacturer != null) // +1 (if) +1 (multiple condition) { - return n; + manufacturerName = product.Details.Manufacturer.Name; } - else // +1 + + if (manufacturerName != null) // +1 (if) { - return -n; + return manufacturerName; } + + return "Unknown"; +} ++Compliant solution
+The optional chaining operator will return
+nullif any reference in the chain isnull, avoiding multiple checks. The +??operator allows to provide the default value to use.+string GetManufacturerName(Product product) +{ + return product?.Details?.Manufacturer?.Name ?? "Unknown"; }+Pitfalls
+As this code is complex, ensure that you have unit tests that cover the code before refactoring.
Resources
Documentation
Transparency attributes in the .NET Framework, designed to protect security-critical operations, can lead to ambiguities and vulnerabilities when +declared at different levels such as both for the class and a method.
Transparency attributes, SecurityCriticalAttribute and SecuritySafeCriticalAttribute are used to identify code that
-performs security-critical operations. The second one indicates that it is safe to call this code from transparent, while the first one does not.
-Since the transparency attributes of code elements with larger scope take precedence over transparency attributes of code elements that are contained
-in the first element a class, for instance, with a SecurityCriticalAttribute can not contain a method with a
-SecuritySafeCriticalAttribute.
This rule raises an issue when a member is marked with a System.Security security attribute that has a different transparency than the
-security attribute of a container of the member.
+Transparency attributes can be declared at several levels. If two different attributes are declared at two different levels, the attribute that +prevails is the one in the highest level. For example, you can declare that a class is
+SecuritySafeCriticaland that a method of this +class isSecurityCritical. In this case, the method will beSecuritySafeCriticaland theSecurityCritical+attribute attached to it is ignored.What is the potential impact?
+Below are some real-world scenarios that illustrate some impacts of an attacker exploiting the vulnerability.
+Elevation of Privileges
+An attacker could potentially exploit conflicting transparency attributes to perform actions with higher privileges than intended.
+Data Exposure
+If a member with conflicting attributes is involved in handling sensitive data, an attacker could exploit the vulnerability to gain unauthorized +access to this data. This could lead to breaches of confidentiality and potential data loss.
+How to fix it
+Code examples
+Noncompliant code example
+using System; using System.Security; namespace MyLibrary { - - [SecurityCritical] + [SecuritySafeCritical] public class Foo { - [SecuritySafeCritical] // Noncompliant + [SecurityCritical] // Noncompliant public void Bar() { } } }-Compliant solution
-+Compliant solution
+using System; using System.Security; namespace MyLibrary { - - [SecurityCritical] public class Foo { + [SecurityCritical] public void Bar() { } } }+How does this work?
+Never set class-level annotations
+A class should never have class-level annotations if some functions have different permission levels. Instead, make sure every function has its own +correct annotation.
+If no function needs a particularly distinct security annotation in a class, just set a class-level
[SecurityCritical].Resources
+Articles & blog posts
+
This rule is deprecated, and will eventually be removed.
Because serialization constructors allocate and initialize objects, security checks that are present on regular constructors must also be present on a serialization constructor. Failure to do so would allow callers that could not otherwise create an instance to use the serialization constructor diff --git a/analyzers/rspec/cs/S4212.json b/analyzers/rspec/cs/S4212.json index 49e92f81d1c..79bcb902d08 100644 --- a/analyzers/rspec/cs/S4212.json +++ b/analyzers/rspec/cs/S4212.json @@ -7,7 +7,7 @@ }, "attribute": "COMPLETE" }, - "status": "ready", + "status": "deprecated", "remediation": { "func": "Constant\/Issue", "constantCost": "5min" diff --git a/analyzers/rspec/cs/S4423.html b/analyzers/rspec/cs/S4423.html index 2764db7711c..1f3b3539acf 100644 --- a/analyzers/rspec/cs/S4423.html +++ b/analyzers/rspec/cs/S4423.html @@ -113,6 +113,5 @@
Merging collapsible if statements increases the code’s readability.
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.
+Merging if statements when possible will decrease the nesting of the code and improve its readability.
Code like
If condition1 Then
If condition2 Then ' Noncompliant
@@ -8,7 +10,7 @@ Noncompliant code example
End If
End If
-Will be more readable as
If condition1 AndAlso condition2 Then
' ...
diff --git a/analyzers/rspec/vbnet/S1066.json b/analyzers/rspec/vbnet/S1066.json
index f6857be37ab..732a6ea96fb 100644
--- a/analyzers/rspec/vbnet/S1066.json
+++ b/analyzers/rspec/vbnet/S1066.json
@@ -1,5 +1,5 @@
{
- "title": "Collapsible \"if\" statements should be merged",
+ "title": "Mergeable \"if\" statements should be combined",
"type": "CODE_SMELL",
"code": {
"impacts": {
diff --git a/analyzers/rspec/vbnet/S1075.html b/analyzers/rspec/vbnet/S1075.html
index 07181ead5f9..d5278fa91f2 100644
--- a/analyzers/rspec/vbnet/S1075.html
+++ b/analyzers/rspec/vbnet/S1075.html
@@ -1,8 +1,13 @@
Why is this an issue?
-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.
-Further even if the elements of a URI are obtained dynamically, portability can still be limited if the path-delimiters are hard-coded.
-This rule raises an issue when URI’s or path delimiters are hard coded.
+Hard-coding a URI makes it difficult to test a program for a variety of reasons:
+In addition, hard-coded URIs can contain sensitive information, like IP addresses, and they should not be stored in the code.
+For all those reasons, a URI should never be hard coded. Instead, it should be replaced by a customizable parameter.
+Further, even if the elements of a URI are obtained dynamically, portability can still be limited if the path delimiters are hard-coded.
+This rule raises an issue when URIs or path delimiters are hard-coded.
diff --git a/analyzers/rspec/vbnet/S112.html b/analyzers/rspec/vbnet/S112.html index f9299e9a450..4e02981eefa 100644 --- a/analyzers/rspec/vbnet/S112.html +++ b/analyzers/rspec/vbnet/S112.html @@ -1,29 +1,52 @@ +This rule raises an issue when a generic exception (such as Exception, SystemException,
+ApplicationException, IndexOutOfRangeException, NullReferenceException, OutOfMemoryException or
+ExecutionEngineException) is thrown.
Throwing such general exceptions as Exception, SystemException, ApplicationException,
-IndexOutOfRangeException, NullReferenceException, OutOfMemoryException and
-ExecutionEngineException prevents calling methods from handling true, system-generated exceptions differently than application-generated
-errors.
+Throwing general exceptions such as
+Exception,SystemExceptionandApplicationExceptionwill have a negative +impact on any code trying to catch these exceptions.From a consumer perspective, it is generally a best practice to only catch exceptions you intend to handle. Other exceptions should ideally not be +caught and let to propagate up the stack trace so that they can be dealt with appropriately. When a generic exception is thrown, it forces consumers +to catch exceptions they do not intend to handle, which they then have to re-throw.
+Besides, when working with a generic type of exception, the only way to distinguish between multiple exceptions is to check their message, which is +error-prone and difficult to maintain. Legitimate exceptions may be unintentionally silenced and errors may be hidden.
+For instance, if an exception such as
+StackOverflowExceptionis caught and not re-thrown, it may prevent the program from terminating +gracefully.When throwing an exception, it is therefore recommended to throw the most specific exception possible so that it can be handled intentionally by +consumers.
+Additionally, some reserved exceptions should not be thrown manually. Exceptions such as
+IndexOutOfRangeException, +NullReferenceException,OutOfMemoryExceptionorExecutionEngineExceptionwill be thrown automatically by the +runtime when the corresponding error occurs. Many of them indicate serious errors, which the application may not be able to recover from. It is +therefore recommended to avoid throwing them as well as using them as base classes.How to fix it
+To fix this issue, make sure to throw specific exceptions that are relevant to the context in which they arise. It is recommended to either:
+
Exception when one matches. For instance ArgumentException could be raised when an unexpected
+ argument is provided to a function. Exception or one of its subclasses.
Public Sub DoSomething(obj As Object)
If obj Is Nothing Then
' Noncompliant
- Throw New NullReferenceException("obj")
+ Throw New NullReferenceException("obj") ' Noncompliant: This reserved exception should not be thrown manually
End If
' ...
End Sub
-+Compliant solution
+Public Sub DoSomething(obj As Object) If obj Is Nothing Then - Throw New ArgumentNullException("obj") + Throw New ArgumentNullException("obj") ' Compliant: this is a specific and non-reserved exception type End If ' ... End SubResources
+Standards
Redundant Boolean literals should be removed from expressions to improve readability.
-+A boolean literal can be represented in two different ways:
+TrueorFalse. They can be combined with logical operators +(Not, And, Or, =) to produce logical expressions that represent truth values. However, comparing a boolean literal to a variable or +expression that evaluates to a boolean value is unnecessary and can make the code harder to read and understand. The more complex a boolean expression +is, the harder it will be for developers to understand its meaning and expected behavior, and it will favour the introduction of new bugs.How to tix it
+Remove redundant boolean literals from expressions to improve readability and make the code more maintainable.
+Code examples
+Noncompliant code example
+If BooleanMethod() = True Then ' Noncompliant ' ... End If @@ -20,8 +26,8 @@-Noncompliant code example
booleanVariable = If(BooleanMethod(), exp, True) ' Noncompliant booleanVariable = If(BooleanMethod(), exp, False) ' NoncompliantCompliant solution
-+Compliant solution
+If BooleanMethod() Then ' ... End If diff --git a/analyzers/rspec/vbnet/S1135.html b/analyzers/rspec/vbnet/S1135.html index 3bb6c3fa8a4..99c29968dd6 100644 --- a/analyzers/rspec/vbnet/S1135.html +++ b/analyzers/rspec/vbnet/S1135.html @@ -1,5 +1,5 @@Why is this an issue?
-Developers often use
TOOOtags to mark areas in the code where additional work or improvements are needed but are not implemented +Developers often use
diff --git a/analyzers/rspec/vbnet/S117.html b/analyzers/rspec/vbnet/S117.html index e77ad902b27..0a7d010b846 100644 --- a/analyzers/rspec/vbnet/S117.html +++ b/analyzers/rspec/vbnet/S117.html @@ -1,27 +1,77 @@ +TODOtags to mark areas in the code where additional work or improvements are needed but are not implemented immediately. However, theseTODOtags sometimes get overlooked or forgotten, leading to incomplete or unfinished code. This code smell class aims to identify and address such unattendedTODOtags to ensure a clean and maintainable codebase. This description will explore why this is a problem and how it can be fixed to improve the overall code quality.Local variables should be named consistently to communicate intent and improve maintainability. Rename your local variable to follow your project’s +naming convention to address this issue.
Why is this an issue?
-Sharing some naming conventions is a key point to make it possible for a team to efficiently collaborate.This rule checks that all local variables -follow a naming convention.
-The default configuration is:
+A naming convention in software development is a set of guidelines for naming code elements like variables, functions, and classes.
+
Local +variables hold the meaning of the written code. Their names should be meaningful and follow a consistent and easily recognizable pattern.
Adhering +to a consistent naming convention helps to make the code more readable and understandable, which makes it easier to maintain and debug. It also +ensures consistency in the code, especially when multiple developers are working on the same project.This rule checks that local variable names match a provided regular expression.
+What is the potential impact?
+Inconsistent naming of local variables can lead to several issues in your code:
With the default regular expression ^[a-z][a-z0-9]*([A-Z]{1,3}[a-z0-9]+)*([A-Z]{2})?$:
+In summary, not adhering to a naming convention for local variables can lead to confusion, errors, and inefficiencies, making the code harder to +read, understand, and maintain.
+How to fix it
+First, familiarize yourself with the particular naming convention of the project in question. Then, update the name to match the convention, as +well as all usages of the name. For many IDEs, you can use built-in renaming and refactoring features to update all usages at once.
+Code examples
+Noncompliant code example
+With the default regular expression
+^[a-z][a-z0-9]*([A-Z]{1,3}[a-z0-9]+)*([A-Z]{2})?$, bringing the following constraints:
Module Module1
Sub Main()
Dim Foo = 0 ' Noncompliant
End Sub
End Module
-+Compliant solution
+Module Module1 Sub Main() Dim foo = 0 ' Compliant End Sub End Module+Resources
+Documentation
+
Unused parameters are misleading. Whatever the values passed to such parameters, the behavior will be the same.
+A typical code smell known as unused function parameters refers to parameters declared in a function but not used anywhere within the function’s +body. While this might seem harmless at first glance, it can lead to confusion and potential errors in your code. Disregarding the values passed to +such parameters, the function’s behavior will be the same, but the programmer’s intention won’t be clearly expressed anymore. Therefore, removing +function parameters that are not being utilized is considered best practice.
This rule raises an issue when a private procedure of a Class, Module or Structure takes a
parameter without using it.
+Exceptions
+This rule doesn’t raise any issue in the following contexts:
+
NotImplementedException. virtual, override procedures. Having unused function parameters in your code can lead to confusion and misunderstanding of a developer’s intention. They reduce code readability +and introduce the potential for errors. To avoid these problems, developers should remove unused parameters from function declarations.
+
Private Sub DoSomething(ByVal a As Integer, ByVal b as Integer) ' "b" is unused
Compute(a)
End Sub
@@ -13,8 +31,8 @@ Noncompliant code example
Return b
End Function
-+Compliant solution
+Private Sub DoSomething(ByVal a As Integer) Compute(a) End Sub @@ -24,15 +42,4 @@-Compliant solution
Return b End FunctionExceptions
-This rule doesn’t raise any issue in the following contexts:
-
NotImplementedException. virtual, override procedures. An empty method is generally considered bad practice and can lead to confusion, readability, and maintenance issues. Empty methods bring no functionality and are misleading to others as they might think the method implementation fulfills a specific and identified requirement.
-Such methods should be avoided and possibly removed.
--Sub DoSomething() ' Noncompliant -End Sub --
However, there are some cases where a method needs to be empty. In those cases, it is essential to minimize confusion and enhance clarity.
-Here are a few examples:
+There are several reasons for a method not to have a body:
NotSupportedException, it makes clear the initial intention not to implement it in the future. -Sub DoSomething() ' Compliant - Throw New NotSupportedException -End Sub -
NotImplementedException, the initial intention to add an implementation in the future is clear. -Sub DoSomething() ' Compliant - Throw New NotImplementedException -End Sub -
-Sub DoSomething() ' Compliant - ' Do nothing because of X -End Sub -
The following empty methods are considered compliant:
@@ -32,4 +14,38 @@MustOverride method as the implementation is mandatory for child class +Sub ShouldNotBeEmpty() ' Noncompliant - method is empty +End Sub + +Sub NotImplementedYet() ' Noncompliant - method is empty +End Sub + +Sub WillNeverBeImplemented() ' Noncompliant - method is empty +End Sub + +Sub EmptyOnPurpose() ' Noncompliant - method is empty +End Sub ++
+Sub ShouldNotBeEmpty() + DoSomething() +End Sub + +Sub NotImplementedYet() + Throw New NotImplementedException +End Sub + +Sub WillNeverBeImplemented() + Throw New NotSupportedException +End Sub + +Sub EmptyOnPurpose() + ' Do nothing because of X +End Sub +diff --git a/analyzers/rspec/vbnet/S1192.html b/analyzers/rspec/vbnet/S1192.html index 5e6a208b75e..5ddafb1c473 100644 --- a/analyzers/rspec/vbnet/S1192.html +++ b/analyzers/rspec/vbnet/S1192.html @@ -1,8 +1,19 @@
Duplicated string literals make the process of refactoring error-prone, since you must be sure to update all occurrences.
-On the other hand, constants can be referenced from many places, but only need to be updated in a single place.
-+Duplicated string literals make the process of refactoring complex and error-prone, as any change would need to be propagated on all +occurrences.
+Exceptions
+The following are ignored:
+
Instead, use constants to replace the duplicated string literals. Constants can be referenced from many places, but only need to be updated in a +single place.
+
Public Class Foo
Private Name As String = "foobar" ' Noncompliant
@@ -17,8 +28,8 @@ Noncompliant code example
End Class
-+Compliant solution
+Public Class Foo Private Const Foobar As String = "foobar" @@ -35,11 +46,4 @@-Compliant solution
End ClassExceptions
-The following are ignored:
-
Nested If, Select, For, For Each, While, Do, and Try
-statements are key ingredients for making what’s known as "Spaghetti code".
Such code is hard to read, refactor and therefore maintain.
-With the default threshold of 3:
+Nested control flow statements If, Select, For, For Each, While, Do,
+and Try are often key ingredients in creating what’s known as "Spaghetti code". This code smell can make your program difficult to
+understand and maintain.
When numerous control structures are placed inside one another, the code becomes a tangled, complex web. This significantly reduces the code’s +readability and maintainability, and it also complicates the testing process.
+The following example demonstrates the behavior of the rule with the default threshold of 3 levels of nesting:
+If condition1 ' Compliant - depth = 1 ' ... diff --git a/analyzers/rspec/vbnet/S1481.html b/analyzers/rspec/vbnet/S1481.html index 902a0f5a049..6745290be5a 100644 --- a/analyzers/rspec/vbnet/S1481.html +++ b/analyzers/rspec/vbnet/S1481.html @@ -1,23 +1,44 @@Why is this an issue?
-If a local variable is declared but not used, it is dead code and should be removed. Doing so will improve maintainability because developers will -not wonder what the variable is used for.
-Noncompliant code example
+An unused local variable is a variable that has been declared but is not used anywhere in the block of code where it is defined. It is dead code, +contributing to unnecessary complexity and leading to confusion when reading the code. Therefore, it should be removed from your code to maintain +clarity and efficiency.
+What is the potential impact?
+Having unused local variables in your code can lead to several issues:
+
In summary, unused local variables can make your code less readable, more confusing, and harder to maintain, and they can potentially lead to bugs +or inefficient memory use. Therefore, it is best to remove them.
+Unused locally created resources in a Using statement are not reported.
+Using t = New TestTimer() +End Using ++
The fix for this issue is straightforward. Once you ensure the unused variable is not part of an incomplete implementation leading to bugs, you +just need to remove it.
+
Public Function NumberOfMinutes(ByVal hours As Integer) As Integer
- Dim seconds As Integer = 0 ' Seconds never used
+ Dim seconds As Integer = 0 ' Noncompliant - seconds is unused
Return hours * 60
End Function
-+Compliant solution
+Public Function NumberOfMinutes(ByVal hours As Integer) As Integer Return hours * 60 End Function-Exceptions
-Unused locally created resources in a
-Usingstatement are not reported.-Using t = New TestTimer() -End Using -diff --git a/analyzers/rspec/vbnet/S1871.html b/analyzers/rspec/vbnet/S1871.html index 1c19f0d72ad..247247a22ce 100644 --- a/analyzers/rspec/vbnet/S1871.html +++ b/analyzers/rspec/vbnet/S1871.html @@ -1,9 +1,22 @@Why is this an issue?
+When the same code is duplicated in two or more separate branches of a conditional, it can make the code harder to understand, maintain, and can +potentially introduce bugs if one instance of the code is changed but others are not.
Having two
-Casesin the sameSelectstatement or branches in the sameIfstructure with the same -implementation is at best duplicate code, and at worst a coding error. If the same logic is truly needed for both instances, then in an -Ifstructure they should be combined, or for aSelect, one should fall through to the other.Noncompliant code example
-+implementation is at best duplicate code, and at worst a coding error. ++If a >= 0 AndAlso a < 10 Then + DoFirst() + DoTheThing() +ElseIf a >= 10 AndAlso a < 20 Then + DoTheOtherThing() +ElseIf a >= 20 AndAlso a < 50 ' Noncompliant; duplicates first condition + DoFirst() + DoTheThing() +Else + DoTheRest(); +End If ++Select i Case 1 DoFirst() @@ -16,19 +29,35 @@+Noncompliant code example
Case Else: DoTheRest() End Select - -If a >= 0 AndAlso a < 10 Then +If the same logic is needed for both instances, then:
+
If structure they should be combined +If (a >= 0 AndAlso a < 10) OrElse (a >= 20 AndAlso a < 50) Then DoFirst() DoTheThing() ElseIf a >= 10 AndAlso a < 20 Then DoTheOtherThing() -ElseIf a >= 20 AndAlso a < 50 ' Noncompliant; duplicates first condition - DoFirst() - DoTheThing() Else DoTheRest(); End If+
Select, the values should be put in the Case expression list. +Select i + Case 1, 3 + DoFirst() + DoSomething() + Case 2 + DoSomethingDifferent() + Case Else: + DoTheRest() +End Select +
Blocks in an If chain or Case clause that contain a single line of code are ignored.
@@ -41,8 +70,8 @@Exceptions
End If
But this exception does not apply to If chains without Else-s, or to Select-s without Case Else
-clauses when all branches have the same single line of code. In case of If chains with Else-s, or of Select-es
-with Case Else clauses, rule {rule:vbnet:S3923} raises a bug.
If chains with Else-s, or of
+Select-es with Case Else clauses, rule {rule:vbnet:S3923} raises a bug.
If a >= 0 AndAlso a < 10 Then DoTheThing() @@ -50,4 +79,9 @@+Exceptions
DoTheOtherThing() ' Noncompliant, this might have been done on purpose but probably not End If
To fix the issue the access of the Nothing value needs to be prevented by either:
To fix the issue, the access of the Nothing value needs to be prevented by either:
Nothing Gratuitous boolean expressions are conditions that do not change the evaluation of a program. This issue can indicate logical errors and affect the +correctness of an application, as well as its maintainability.
An operand of a boolean expression that never changes the result of the expression might not match the programmer’s intent and can lead to -unexpected behavior and potential bugs.
--Dim a = True - -If a Then - DoSomething() -End If -- +
The presence of gratuitous conditions can indicate a logical error. For example, the programmer intended to have the program branch into +different paths but made a mistake when formulating the branching condition. In this case, this issue might result in a bug and thus affect the +reliability of the application. For instance, it might lead to the computation of incorrect results.
+Additionally, gratuitous conditions and control flow constructs introduce unnecessary complexity. The source code becomes harder to understand, and +thus, the application becomes more difficult to maintain.
+This rule looks for operands of a boolean expression never changing the result of the expression. It also applies to the null conditional operator
when one of the operands always evaluates to Nothing.
@@ -32,11 +34,11 @@Exceptions
In these cases, it is obvious the code is as intended.
How to fix it
-The conditions should be reviewed to decide whether:
-
Gratuitous boolean expressions are suspicious and should be carefully removed from the code.
+First, the boolean expression in question should be closely inspected for logical errors. If a mistake was made, it can be corrected so the +condition is no longer gratuitous.
+If it becomes apparent that the condition is actually unnecessary, it can be removed. The associated control flow construct (e.g., the
+if-statement containing the condition) will be adapted or even removed, leaving only the necessary branches.
diff --git a/analyzers/rspec/vbnet/S2612.html b/analyzers/rspec/vbnet/S2612.html index 199ea44d811..96ad5b7ae6d 100644 --- a/analyzers/rspec/vbnet/S2612.html +++ b/analyzers/rspec/vbnet/S2612.html @@ -66,6 +66,5 @@See
href="https://owasp.org/www-project-web-security-testing-guide/latest/4-Web_Application_Security_Testing/02-Configuration_and_Deployment_Management_Testing/09-Test_File_Permission">OWASP File Permission
This rule raises an issue when the code cognitive complexity of a function is above a certain threshold.
Cognitive Complexity Complexity is a measure of how hard the control flow of -a method is to understand.
-Methods with high Cognitive Complexity will be difficult to maintain.
+Cognitive Complexity is a measure of how hard it is to understand the control flow of a unit of code. Code with high cognitive complexity is hard +to read, understand, test, and modify.
+As a rule of thumb, high cognitive complexity is a sign that the code should be refactored into smaller, easier-to-manage pieces.
+Here are the core concepts:
+The method of computation is fully detailed in the pdf linked in the resources.
+Developers spend more time reading and understanding code than writing it. High cognitive complexity slows down changes and increases the cost of +maintenance.
Function Abs(ByVal n As Integer) As Integer ' Noncompliant: cognitive complexity = 5
If n >= 0 Then ' +1
@@ -30,11 +46,11 @@ Why is this an issue?
Resources
Documentation
;, are", "Empty statements, i.e. ;, are usually introduced by mistake, for example because:");
+ ValidateDescription("S1116", "by a semicolon ;", "Empty statements represented by a semicolon ; are statements that do not perform any operation.");
[TestMethod]
public void Description_HtmlIsDecoded() =>