Vbnet

A cast is an explicit conversion, which is a way to tell the compiler the intent to convert from one type to another.

In Visual Basic, there are two explicit conversion operators:

Functions can return values using two different syntaxes. The modern, and correct, way to do it is to use a `Return statement. The VB6 way, i.e. old way, is to assign a return value to the function’s name .

The VB6 syntax is obsolete as it was introduced to simplify migration from VB6 projects. The compiler will create a local variable which is implicitly returned when execution exits the function’s scope.

Return` statement should be used instead as they are easier to read and understand.

There’s no point in creating an array solely for the purpose of passing it to a ParamArray parameter. Simply pass the elements directly. They will be consolidated into an array automatically.

Short-circuit evaluation is an evaluation strategy for Boolean operators, that doesn’t evaluate the second argument of the operator if it is not needed to determine the result of the operation.

VB.NET provides logical operators that implement short-circuiting evaluations AndAlso and OrElse, as well as the non-short-circuiting versions And and Or. Unlike short-circuiting operators, the non-short-circuiting operators evaluate both operands and afterward perform the logical operation.

For example False AndAlso FunctionCall always results in False even when the FunctionCall invocation would raise an exception. In contrast, False And FunctionCall also evaluates FunctionCall, and results in an exception if FunctionCall raises an exception.

Similarly, True OrElse FunctionCall always results in True, no matter what the return value of FunctionCall would be.

The use of non-short-circuit logic in a boolean context is likely a mistake, one that could cause serious program errors as conditions are evaluated under the wrong circumstances.

Complex boolean expressions are hard to read and so to maintain.

In most cases, indexed properties should be named Item for consistency. Exceptions are when there exists a name which is obviously better, for example System.String.Chars(System.Int32).

A loop statement with at most one iteration is equivalent to an If statement; the following block is executed only once.

If the initial intention was to conditionally execute the block only once, an If statement should be used instead. If that was not the initial intention, the block of the loop should be fixed so the block is executed multiple times.

A loop statement with at most one iteration can happen when a statement unconditionally transfers control, such as a jump statement or a throw statement, is misplaced inside the loop block.

This rule raises when the following statements are misplaced:

Using the With statement for a series of calls to the same object makes the code more readable.

When an assembly uses Windows Forms (classes and interfaces from the `System.Windows.Forms namespace) its entry point should be marked with the STAThreadAttribute to indicate that the threading model should be “Single-Threaded Apartment” (STA) which is the only one supported by Windows Forms.

This rule raises an issue when the entry point (Shared Sub Main` method) of an assembly using Windows Forms is not marked as STA.

Shared coding conventions allow teams to collaborate efficiently. This rule checks that all non-private Const field names comply with the provided regular expression.

The default configuration is the one recommended by Microsoft:

The DebuggerDisplayAttribute is used to determine how an object is displayed in the debugger window.

The DebuggerDisplayAttribute constructor takes a single mandatory argument: the string to be displayed in the value column for instances of the type. Any text within curly braces is evaluated as the name of a field or property, or any complex expression containing method calls and operators.

Naming a non-existent member between curly braces will result in a BC30451 error in the debug window when debugging objects. Although there is no impact on the production code, providing a wrong value can lead to difficulties when debugging the application.

This rule raises an issue when text specified between curly braces refers to members that don’t exist in the current context.

Shared coding conventions allow teams to collaborate efficiently. This rule checks that all even names match a provided regular expression.

The default configuration is the one recommended by Microsoft:

Because parameter names could be changed during refactoring, they should not be spelled out literally in strings. Instead, use `NameOf(), and the string that’s output will always be correct.

This rule raises an issue when any string in the Throw` statement is an exact match for the name of one of the method parameters.

Using Thread.Sleep in a test might introduce unpredictable and inconsistent results depending on the environment. Furthermore, it will block the thread, which means the system resources are not being fully used.

Creating a new Exception without actually throwing it is useless and is probably due to a mistake.

If you’re using a Structure, it is likely because you’re interested in performance. But by failing to implement IEquatable<T> you’re loosing performance when comparisons are made because without IEquatable<T>, boxing and reflection are used to make comparisons.

Other than `Exit Select, using an Exit statement is never a good idea.

Exit Do, Exit For, Exit Try, and Exit While will all result in unstructured control flow, i.e. spaghetti code.

Exit Function, Exit Property, and Exit Sub are all poor, less-readable substitutes for a simple return, and if used with code that should return a value (Exit Function and in some cases Exit Property) they could result in a NullReferenceException.

This rule raises an issue for all uses of Exit except Exit Select and Exit Do` statements in loops without condition.

Unsigned integers have different arithmetic operators than signed ones - operators that few developers understand. Therefore, signed types should be preferred where possible.

Certain bitwise operations are not needed and should not be performed because their results are predictable.

Specifically, using And -1 with any value always results in the original value.

That is because the binary representation of -1 on a numeric data type supporting negative numbers, such as Integer or Long, is based on two’s complement and made of all 1s: &B111…​111.

Performing And between a value and &B111…​111 means applying the And operator to each bit of the value and the bit 1, resulting in a value equal to the provided one, bit by bit.

Calling ToString() on an object should always return a string. Thus, overriding the ToString method should never return Nothing because it breaks the method’s implicit contract, and as a result the consumer’s expectations.

Shared naming conventions allow teams to collaborate efficiently. This rule checks that all non-private Shared ReadOnly fields names match a provided regular expression.

The default configuration is:

Numbers can be shifted with the <[/code> and <code]> operators, but the right operand of the operation needs to be an int or a type that has an implicit conversion to int. However, when the left operand is an object, the compiler’s type checking is turned off, therfore you can pass anything to the right of a shift operator and have it compile. If the argument can’t be implicitly converted to int at runtime, a RuntimeBinderException will be raised.

Using the same value on either side of a binary operator is almost always a mistake. In the case of logical operators, it is either a copy/paste error and therefore a bug, or it is simply wasted code, and should be simplified. In the case of most binary mathematical operators, having the same value on both sides of an operator yields predictable results, and should be simplified.

This rule ignores *, +, &, <<, and >>.

Array literals are more compact than array creation expressions.

Visual Basic .NET offers a non-short-circuit conditional function, `IIf(), which returns either its second or third parameter based on the expression in the first parameter. Using it is slower than using If() because each parameter is unconditionally evaluated. Further, its use can lead to runtime exceptions because IIf always evaluates all three of its arguments.

The newer version, If()`, should be used instead because it short-circuits the evaluation of its parameters.

A chain of If/ElseIf statements is evaluated from top to bottom. At most, only one branch will be executed: the first statement with a condition that evaluates to True. Therefore, duplicating a condition leads to unreachable code inside the duplicated condition block. Usually, this is due to a copy/paste error.

The result of such duplication can lead to unreachable code or even to unexpected behavior.

Using operator pairs (=+ or =-) that look like reversed single operators (+= or -=) is confusing. They compile and run but do not produce the same result as their mirrored counterpart.

Sharing some naming conventions is a key point to make it possible for a team to efficiently collaborate.

This rule allows to check that all parameter names match a provided regular expression.

The default configuration is the one recommended by Microsoft:

The size of a collection and the length of an array are always greater than or equal to zero. Testing it doesn’t make sense, since the result is always true.

For the sake of backward compatibility, Visual Basic .NET continues to offer a set of functions that convert from Object to different primitive types: CChar, CStr, CBool, CDate, CSng, CDbl, CDec, CByte, CSByte, CShort, CUShort, CInt, CUInt, CLng, CULng. However, using these functions is misleading, because it suggests a cast. It is better to cast explicitly using CType(), or use Convert.To() when the value should be converted.

While it is technically correct to assign to parameters from within method bodies, doing so before the parameter value is read is likely a bug. Instead, initial values of parameters should be, if not treated as readonly then at least read before reassignment.

This rule applies whenever an `If statement is followed by one or more ElseIf statements; the final ElseIf should be followed by an Else statement.

The requirement for a final Else statement is defensive programming.

The Else statement should either take appropriate action or contain a suitable comment as to why no action is taken. This is consistent with the requirement to have a final Case Else clause in a Select Case` statement.

Since Visual Studio 2010 SP1, the ByVal parameter modifier is implicitly applied, and therefore not required anymore. Removing it from your source code will improve readability.

Shared coding conventions allow teams to collaborate efficiently. This rule checks that all even handler names match a provided regular expression.

The default configuration is:

Event handlers with a handles clause and two-parameter methods with EventArgs second parameter are covered by this rule.

Consistently using the `& operator for string concatenation make the developer intentions clear.

&, unlike +, will convert its operands to strings and perform an actual concatenation.

+` on the other hand can be an addition, or a concatenation, depending on the operand types.

Shared naming conventions allow teams to collaborate efficiently. This rule checks that all enum names match a provided regular expression.

The default configuration is the one recommended by Microsoft:

Creating an extension method that extends Object is not recommended because it makes the method available on every type. Extensions should be applied at the most specialized level possible, and that is very unlikely to be Object.

An enumeration can be decorated with the FlagsAttribute to indicate that it can be used as a bit field: a set of flags, that can be independently set and reset.

For example, the following definition of the day of the week:

The repetition of the Not operator is usually a typo. The second operator invalidates the first one:

StringBuilder instances that never build a string clutter the code and worse are a drag on performance. Either they should be removed, or the missing ToString() call should be added.

To improve the code readability, the explicit line continuation character, _, should not be used. Instead, it is better to break lines after an operator.

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.

Recursion is a technique used to define a problem in terms of the problem itself, usually in terms of a simpler version of the problem itself.

For example, the implementation of the generator for the n-th value of the Fibonacci sequence comes naturally from its mathematical definition, when recursion is used:

A Catch clause that only rethrows the caught exception has the same effect as omitting the Catch altogether and letting it bubble up automatically.

Shared coding conventions allow teams to collaborate efficiently. This rule checks that all `Private Shared ReadOnly field names comply with the provided regular expression.

The default configuration is:

Nullable value types can hold either a value or Nothing.

The value stored in the nullable type can be accessed with the Value property or by casting it to the underlying type. Still, both operations throw an InvalidOperationException when the value is Nothing. A nullable type should always be tested before accessing the value to avoid raising exceptions.

Throwing general exceptions such as `Exception, SystemException and ApplicationException will 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 StackOverflowException is 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, OutOfMemoryException or ExecutionEngineException` will 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.

Shared coding conventions allow teams to collaborate efficiently. This rule checks that property names match a provided regular expression.

The default configuration is the one recommended by Microsoft:

Properties provide a way to enforce encapsulation by providing property procedures that give controlled access to Private fields. However, in classes with multiple fields, it is not unusual that copy-and-paste is used to quickly create the needed properties, which can result in the wrong field being accessed by the property procedures.

End statements exit the control flow of the program in an unstructured way. This statement stops code execution immediately without executing Dispose or Finalize methods, or executing Finally blocks. Therefore, it should be avoided.

Shared coding conventions allow teams to collaborate efficiently. This rule checks that all non-private fields names match a provided regular expression.

Note that this rule does not apply to non-private Shared ReadOnly fields, for which there is another rule.

The default configuration is:

Most checks against an IndexOf value compare it with -1 because 0 is a valid index.

When Select Case statements have large sets of case clauses, it is usually an attempt to map two sets of data. A Dictionary should be used instead to make the code more readable and maintainable.

GoTo is an unstructured control flow statement. It makes code less readable and maintainable. Structured control flow statements such as If, For, While, or Exit should be used instead.

Shared coding conventions allow teams to collaborate efficiently. This rule checks that all `Private Const field names comply with the provided regular expression.

The default configuration is:

Declaring multiple variable on one line is difficult to read.

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:

The Select…Case statement should be used only to clearly define some new branches in the control flow. As soon as a case clause contains too many statements this highly decreases the readability of the overall control flow statement. In such case, the content of the case clause should be extracted into a dedicated procedure.

Shared naming conventions allow teams to collaborate efficiently. This rule checks that all subroutine and function names match a provided regular expression.

The default configuration is the one recommended by Microsoft:

Having all branches of a Select Case or If chain with the same implementation indicates a problem.

In the following code:

Sharing some naming conventions is a key point to make it possible for a team to efficiently collaborate.

This rule allows to check that all interface names match a provided regular expression.

The default configuration is the one recommended by Microsoft:

A Do … Loop without a While or Until condition must be terminated by an unstructured Exit Do statement. It is safer and more readable to use structured loops instead.

Inconsistent naming conventions can lead to confusion and errors when working in a team. This rule ensures that all generic type parameter names follow a consistent naming convention by checking them against a provided regular expression.

The default configuration follows Microsoft’s recommended convention:

Methods declared as Public, Protected, or Protected Friend can be accessed from other assemblies, which means you should validate parameters to be within the expected constraints. In general, checking against Nothing is recommended in defensive programming.

This rule raises an issue when a parameter of a publicly accessible method is not validated against Nothing before being dereferenced.

There’s no need to null test in conjunction with an TypeOf … Is test. Nothing is not an instance of anything, so a null check is redundant.

There is no good excuse for an empty class. If it’s being used simply as a common extension point, it should be replaced with an Interface. If it was stubbed in as a placeholder for future development it should be fleshed-out. In any other case, it should be eliminated.

Visual Basic .NET, unlike many other programming languages, has no “fall-through” for its Select cases. Each case already has an implicit Exit Select as its last instruction. It therefore is redundant to explicitly add one.

Indexed properties are meant to represent access to a logical collection. When multiple parameters are required, this design guideline may be violated, and refactoring the property into a method is preferable.

Shared coding conventions allow teams to collaborate efficiently. This rule checks that all enumeration value names match a provided regular expression.

The default configuration is the one recommended by Microsoft:

The … IsNot … syntax is more compact and more readable than the Not … Is … syntax.

Prefer the use of `Try … Catch blocks instead of On Error statements.

Visual Basic .NET and Visual Basic 2005 offer structured exception handling that provides a powerful, more readable alternative to the On Error Goto error handling from previous versions of Microsoft Visual Basic. Structured exception handling is more powerful because it allows you to nest error handlers inside other error handlers within the same procedure. Furthermore, structured exception handling uses a block syntax similar to the If…Else…End If` statement. This makes Visual Basic .NET and Visual Basic 2005 code more readable and easier to maintain.

Shared coding conventions allow teams to collaborate efficiently. This rule checks that all `Private field names match the provided regular expression.

Note that this rule does not apply to Private Shared ReadOnly fields, which are checked by another rule.

The default configuration is:

The … = {} syntax is more compact, more readable and less error-prone.

Shared coding conventions allow teams to collaborate efficiently. This rule checks that all namespace names match a provided regular expression.

The default configuration is the one recommended by Microsoft:

StringBuilder is more efficient than string concatenation, especially when the operator is repeated over and over as in loops.

Numbers are infinite, but the types that hold them are not. Each numeric type has hard upper and lower bounds. Try to calculate numbers beyond those bounds, and the result will be an OverflowException. When the compilation is configured to remove integer overflow checking, the value will be silently wrapped around from the expected positive value to a negative one, or vice versa.

Method for creating empty arrays Array.Empty(Of TElement) was introduced in .NET 4.6 to optimize object instantiation and memory allocation. It also improves code readability by making developer’s intent more explicit. This new method should be preferred over empty array declaration.

The requirement for a final Case Else clause is defensive programming.

This clause should either take appropriate action or contain a suitable comment as to why no action is taken.

Nested `Select Case structures are difficult to understand because you can easily confuse the cases of an inner Select Case as belonging to an outer statement. Therefore nested Select Case statements should be avoided.

Specifically, you should structure your code to avoid the need for nested Select Case statements, but if you cannot, then consider moving the inner Select Case` to another function.

Subscribing to events without unsubscribing later on can lead to memory leaks or even duplicate subscriptions, i.e. code which is executed multiple times by mistake.

Even if there is no problem right now, the code is more difficult to review and a simple refactoring can create a bug. For example the lifetime of the event publisher could change and prevent subscribers from being garbage collected.

There are patterns to automatically unsubscribe, but the simplest and most readable solution remains to unsubscribe from events explicitly using `RemoveHandler.

This rule raises an issue when a class subscribes to an even using AddHandler without explicitly unsubscribing with RemoveHandler`.

Passing parameters by reference requires developers to understand the subtle differences between reference and value types. It is preferable to avoid passing parameters by reference when possible.

”After” and “Before” prefixes or suffixes should not be used to indicate pre and post events. The concepts of before and after should be given to events using the present and past tense.

One of the principles of a unit test is that it must have full control of the system under test. This is problematic when production code includes calls to static methods, which cannot be changed or controlled. Date/time functions are usually provided by system libraries as static methods.

This can be improved by wrapping the system calls in an object or service that can be controlled inside the unit test.

If a lock is acquired and released within a method, then it must be released along all execution paths of that method.

Failing to do so will expose the conditional locking logic to the method’s callers and hence be deadlock-prone.

It is needlessly complex to invert the result of a boolean comparison. The opposite comparison should be made instead.

Regular expressions have their own syntax that is understood by regular expression engines. Those engines will throw an exception at runtime if they are given a regular expression that does not conform to that syntax.

To avoid syntax errors, special characters should be escaped with backslashes when they are intended to be matched literally and references to capturing groups should use the correctly spelled name or number of the group.

Operating systems have global directories where any user has write access. Those folders are mostly used as temporary storage areas like `/tmp in Linux based systems. An application manipulating files from these folders is exposed to race conditions on filenames: a malicious user can try to create a file with a predictable name before the application does. A successful attack can result in other files being accessed, modified, corrupted or deleted. This risk is even higher if the application runs with elevated permissions.

In the past, it has led to the following vulnerabilities:

This rule raises an issue whenever it detects a hard-coded path to a publicly writable directory like /tmp (see examples bellow). It also detects access to environment variables that point to publicly writable directories, e.g., TMP and TMPDIR.

Putting multiple statements on a single line lowers the code readability and makes debugging the code more complex.

Unresolved directive in <stdin> - include::{noncompliant}[]

Write one statement per line to improve readability.

Unresolved directive in <stdin> - include::{compliant}[]

The information that an enumeration type is actually an enumeration or a set of flags should not be duplicated in its name.

When executing an OS command and unless you specify the full path to the executable, then the locations in your application’s PATH environment variable will be searched for the executable. That search could leave an opening for an attacker if one of the elements in PATH is a directory under his control.

Having a permissive Cross-Origin Resource Sharing policy is security-sensitive. It has led in the past to the following vulnerabilities:

Same origin policy in browsers prevents, by default and for security-reasons, a javascript frontend to perform a cross-origin HTTP request to a resource that has a different origin (domain, protocol, or port) from its own. The requested target can append additional HTTP headers in response, called CORS, that act like directives for the browser and change the access control policy / relax the same origin policy.

When declaring a Windows Communication Foundation (WCF) OperationContract method as one-way, that service method won’t return any result, not even an underlying empty confirmation message. These are fire-and-forget methods that are useful in event-like communication. Therefore, specifying a return type has no effect and can confuse readers.

Development tools and frameworks usually have options to make debugging easier for developers. Although these features are useful during development, they should never be enabled for applications deployed in production. Debug instructions or error messages can leak detailed information about the system, like the application’s path or file names.

A boolean literal can be represented in two different ways: {true} or {false}. They can be combined with logical operators ({ops}) 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.

This rule is meant to be used as a way to track code which is marked as being deprecated. Deprecated code should eventually be removed.

Properties with only setters are confusing and counterintuitive. Instead, a property getter should be added if possible, or the property should be replaced with a setter method.

When optional parameter values are not passed to base method calls, the value passed in by the caller is ignored. This can cause the function to behave differently than expected, leading to errors and making the code difficult to debug.

Fields should not be part of an API, and therefore should always be private. Indeed, they cannot be added to an interface for instance, and validation cannot be added later on without breaking backward compatibility. Instead, developers should encapsulate their fields into properties. Explicit property getters and setters can be introduced for validation purposes or to smooth the transition to a newer system.

Successful Zip Bomb attacks occur when an application expands untrusted archive files without controlling the size of the expanded data, which can lead to denial of service. A Zip bomb is usually a malicious archive file of a few kilobytes of compressed data but turned into gigabytes of uncompressed data. To achieve this extreme compression ratio, attackers will compress irrelevant data (eg: a long string of repeated bytes).

Having a field in a child class with a name that differs from a parent class’ field only by capitalization is sure to cause confusion. Such child class fields should be renamed.

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.

An empty {operationName} is generally considered bad practice and can lead to confusion, readability, and maintenance issues. Empty {operationName}s bring no functionality and are misleading to others as they might think the {operationName} implementation fulfills a specific and identified requirement.

There are several reasons for a {operationName} not to have a body:

Clear-text protocols such as `ftp, telnet, or http lack encryption of transported data, as well as the capability to build an authenticated connection. It means that an attacker able to sniff traffic from the network can read, modify, or corrupt the transported content. These protocols are not secure as they expose applications to an extensive range of risks:

Even in the context of isolated networks like offline environments or segmented cloud environments, the insider threat exists. Thus, attacks involving communications being sniffed or tampered with can still happen.

For example, attackers could successfully compromise prior security layers by:

In such cases, encrypting communications would decrease the chances of attackers to successfully leak data or steal credentials from other network components. By layering various security practices (segmentation and encryption, for example), the application will follow the defense-in-depth principle.

Note that using the http` protocol is being deprecated by major web browsers.

In the past, it has led to the following vulnerabilities:

The use of parentheses, even those not required to enforce a desired order of operations, can clarify the intent behind a piece of code. However, redundant pairs of parentheses could be misleading and should be removed.

Array designators should always be located on the type for better code readability. Otherwise, developers must look both at the type and the variable name to know whether or not a variable is an array.

Arbitrary OS command injection vulnerabilities are more likely when a shell is spawned rather than a new process, indeed shell meta-chars can be used (when parameters are user-controlled for instance) to inject OS commands.

Rejecting requests with significant content length is a good practice to control the network traffic intensity and thus resource consumption in order to prevent DoS attacks.

Formatted SQL queries can be difficult to maintain, debug and can increase the risk of SQL injection when concatenating untrusted values into the query. However, this rule doesn’t detect SQL injections (unlike rule S3649), the goal is only to highlight complex/formatted queries.

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.

Shared naming conventions allow teams to collaborate efficiently.

This rule raises an issue when a class name does not match a provided regular expression.

Assemblies should explicitly indicate whether they are meant to be COM visible or not. If the ComVisibleAttribute is not present, the default is to make the content of the assembly visible to COM clients.

Note that COM visibility can be overridden for individual types and members.

switch statements are useful when there are many different cases depending on the value of the same expression.

For just one or two cases, however, the code will be more readable with if statements.

Conditional expressions which are always true or false can lead to unreachable code.

There is no reason to re-assign a variable to itself. Either this statement is redundant and should be removed, or the re-assignment is a mistake and some other value or variable was intended for the assignment instead.

Serialization event handlers that don’t have the correct signature will not be called, bypassing augmentations to automated serialization and deserialization events.

A method is designated a serialization event handler by applying one of the following serialization event attributes:

Serialization event handlers take a single parameter of type StreamingContext, return void, and have private visibility.

This rule raises an issue when any of these constraints are not respected.

Assemblies should conform with the Common Language Specification (CLS) in order to be usable across programming languages. To be compliant an assembly has to indicate it with System.CLSCompliantAttribute.

Constant members are copied at compile time to the call sites, instead of being fetched at runtime.

As an example, say you have a library with a constant `Version member set to 1.0, and a client application linked to it. This library is then updated and Version is set to 2.0. Unfortunately, even after the old DLL is replaced by the new one, Version will still be 1.0 for the client application. In order to see 2.0, the client application would need to be rebuilt against the new version of the library.

This means that you should use constants to hold values that by definition will never change, such as Zero`. In practice, those cases are uncommon, and therefore it is generally better to avoid constant members.

This rule only reports issues on public constant fields, which can be reached from outside the defining assembly.

According to the Single Responsibility Principle, introduced by Robert C. Martin in his book “Principles of Object Oriented Design”, a class should have only one responsibility:

Classes which rely on many other classes tend to aggregate too many responsibilities and should be split into several smaller ones.

Nested classes dependencies are not counted as dependencies of the outer class.

Nested ternaries are hard to read and can make the order of operations complex to understand.

Unresolved directive in <stdin> - include::{noncompliant}[]

Instead, use another line to express the nested operation in a separate statement.

Unresolved directive in <stdin> - include::{compliant}[]

Clear, communicative naming is important in code. It helps maintainers and API users understand the intentions for and uses of a unit of code. Using “exception” in the name of a class that does not extend Exception or one of its subclasses is a clear violation of the expectation that a class’ name will indicate what it is and/or does.

For clarity, all overloads of the same method should be grouped together. That lets both users and maintainers quickly understand all the current available options.

Duplicated string literals make the process of refactoring complex and error-prone, as any change would need to be propagated on all occurrences.

Types are declared in namespaces in order to prevent name collisions and as a way to organize them into the object hierarchy. Types that are defined outside any named namespace are in a global namespace that cannot be referenced in code.

Altering or bypassing the accessibility of classes, methods, or fields through reflection violates the encapsulation principle. This can break the internal contracts of the accessed target and lead to maintainability issues and runtime errors.

This rule raises an issue when reflection is used to change the visibility of a class, method or field, and when it is used to directly update a field value.

CoSetProxyBlanket and CoInitializeSecurity both work to set the permissions context in which the process invoked immediately after is executed. Calling them from within that process is useless because it’s too late at that point; the permissions context has already been set.

Specifically, these methods are meant to be called from non-managed code such as a C++ wrapper that then invokes the managed, i.e. C# or VB.NET, code.

Not specifying a timeout for regular expressions can lead to a Denial-of-Service attack. Pass a timeout when using System.Text.RegularExpressions to process untrusted input because a malicious user might craft a value for which the evaluation lasts excessively long.

ASP.NET 1.1+ comes with a feature called Request Validation, preventing the server to accept content containing un-encoded HTML. This feature comes as a first protection layer against Cross-Site Scripting (XSS) attacks and act as a simple Web Application Firewall (WAF) rejecting requests potentially containing malicious content.

While this feature is not a silver bullet to prevent all XSS attacks, it helps to catch basic ones. It will for example prevent <script type=“text/javascript” src=“https://malicious.domain/payload.js”> to reach your Controller.

Note: Request Validation feature being only available for ASP.NET, no Security Hotspot is raised on ASP.NET Core applications.

This rule verifies that single-line comments are not located at the ends of lines of code. The main idea behind this rule is that in order to be really readable, trailing comments would have to be properly written and formatted (correct alignment, no interference with the visual structure of the code, not too long to be visible) but most often, automatic code formatters would not handle this correctly: the code would end up less readable. Comments are far better placed on the previous empty line of code, where they will always be visible and properly formatted.

The overloading mechanism should be used in place of optional parameters for several reasons:

To customize the default behavior for an export in the Managed Extensibility Framework (MEF), applying the PartCreationPolicyAttribute is necessary. For the PartCreationPolicyAttribute to be meaningful in the context of an export, the class must also be annotated with the ExportAttribute.

This rule raises an issue when a class is annotated with the PartCreationPolicyAttribute but not with the ExportAttribute.

{upper_function}s with a long parameter list are difficult to use because maintainers must figure out the role of each parameter and keep track of their position.

Unresolved directive in <stdin> - include::{language}/noncompliant.adoc[]

The solution can be to:

Unresolved directive in <stdin> - include::{language}/split-example.adoc[]

Unresolved directive in <stdin> - include::{language}/struct-example.adoc[]

This rule raises an issue when a {function} has more parameters than the provided threshold.

Two {func_name}s having the same implementation are suspicious. It might be that something else was intended. Or the duplication is intentional, which becomes a maintenance burden.

Hardcoding IP addresses is security-sensitive. It has led in the past to the following vulnerabilities:

Today’s services have an ever-changing architecture due to their scaling and redundancy needs. It is a mistake to think that a service will always have the same IP address. When it does change, the hardcoded IP will have to be modified too. This will have an impact on the product development, delivery, and deployment:

Last but not least it has an effect on application security. Attackers might be able to decompile the code and thereby discover a potentially sensitive address. They can perform a Denial of Service attack on the service, try to get access to the system, or try to spoof the IP address to bypass security checks. Such attacks can always be possible, but in the case of a hardcoded IP address solving the issue will take more time, which will increase an attack’s impact.

When the syntax new Guid() (i.e. parameterless instantiation) is used, it must be that one of three things is wanted:

This rule raises an issue when a parameterless instantiation of the Guid struct is found.

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 rule aims to identify and address unattended TODO tags to ensure a clean and maintainable codebase. This description explores why this is a problem and how it can be fixed to improve the overall code quality.

ViewBag and ViewData dictionaries enable controllers to pass data to their views as weakly typed collections. Reading the provided values is dynamically resolved at runtime without any compile-time checking. This can lead to unexpected behavior, since reading a missing value does not produce an exception.

Controllers should pass data to their views via a strongly typed view model class.

The ExcludeFromCodeCoverageAttribute is used to exclude portions of code from code coverage reporting. It is a bad practice to retain code that is not covered by unit tests. In .Net 5, the Justification property was added to the ExcludeFromCodeCoverageAttribute as an opportunity to document the rationale for the exclusion. This rule raises an issue when no such justification is given.

When you annotate an Enum with the Flags attribute, you must not rely on the values that are automatically set by the language to the Enum members, but you should define the enumeration constants in powers of two (1, 2, 4, 8, and so on). Automatic value initialization will set the first member to zero and increment the value by one for each subsequent member. As a result, you won’t be able to use the enum members with bitwise operators.

In Unix file system permissions, the “others” category refers to all users except the owner of the file system resource and the members of the group assigned to this resource.

Granting permissions to this category can lead to unintended access to files or directories that could allow attackers to obtain sensitive information, disrupt services or elevate privileges.

`if statements with conditions that are always false have the effect of making blocks of code non-functional. if statements with conditions that are always true are completely redundant, and make the code less readable.

There are three possible causes for the presence of such code:

In any of these cases, unconditional if` statements should be removed.

Having a variable with the same name in two unrelated classes is fine, but do the same thing within a class hierarchy and you’ll get confusion at best, chaos at worst.

Cryptographic hash algorithms such as MD2, MD4, MD5, MD6, HAVAL-128, HMAC-MD5, DSA (which uses SHA-1), RIPEMD, RIPEMD-128, RIPEMD-160, HMACRIPEMD160 and SHA-1 are no longer considered secure, because it is possible to have collisions (little computational effort is enough to find two or more different inputs that produce the same hash).

It may be a good idea to raise an exception in a constructor if you’re unable to fully flesh the object in question, but not in an exception constructor. If you do, you’ll interfere with the exception that was originally being thrown. Further, it is highly unlikely that an exception raised in the creation of an exception will be properly handled in the calling code, and the unexpected, unhandled exception will lead to program termination.

Fields marked with `System.Runtime.Serialization.OptionalFieldAttribute are serialized just like any other field. But such fields are ignored on deserialization, and retain the default values associated with their types. Therefore, deserialization event handlers should be declared to set such fields during the deserialization process.

This rule raises when at least one field with the System.Runtime.Serialization.OptionalFieldAttribute attribute is declared but one (or both) of the following event handlers System.Runtime.Serialization.OnDeserializingAttribute or System.Runtime.Serialization.OnDeserializedAttribute` are not present.

The use of a non-standard algorithm is dangerous because a determined attacker may be able to break the algorithm and compromise whatever data has been protected. Standard algorithms like `SHA-256, SHA-384, SHA-512, …​ should be used instead.

This rule tracks creation of java.security.MessageDigest` subclasses.

Because it is easy to extract strings from an application source code or binary, credentials should not be hard-coded. This is particularly true for applications that are distributed or that are open-source.

In the past, it has led to the following vulnerabilities:

Credentials should be stored outside of the code in a configuration file, a database, or a management service for secrets.

This rule flags instances of hard-coded credentials used in database and LDAP connections. It looks for hard-coded credentials in connection strings, and for variable names that match any of the patterns from the provided list.

It’s recommended to customize the configuration of this rule with additional credential words such as “oauthToken”, “secret”, …​

There are valid cases for passing a variable multiple times into the same method call, but usually doing so is a mistake, and something else was intended for one of the arguments.

Marking a method with the Pure attribute indicates that the method doesn’t make any visible state changes. Therefore, a Pure method should return a result. Otherwise, it indicates a no-operation call.

Using Pure on a void method is either by mistake or the method is not doing a meaningful task.

It should be clear to a casual reader what code a test is testing and what results are expected. Unfortunately, that’s not usually the case with the ExpectedException attribute since an exception could be thrown from almost any line in the method.

This rule detects MSTest and NUnit ExpectedException attribute.

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.

Disposing an object twice in the same method, either with the {usingArg} or by calling Dispose directly, is confusing and error-prone. For example, another developer might try to use an already-disposed object, or there can be runtime errors for specific paths in the code.

In addition, even if the documentation explicitly states that calling the Dispose method multiple times should not throw an exception, some implementations still do it. Thus it is safer to not dispose of an object twice when possible.

If an exception is already being thrown within the try block or caught in a catch block, throwing another exception in the finally block will override the original exception. This means that the original exception’s message and stack trace will be lost, potentially making it challenging to diagnose and troubleshoot the root cause of the problem.

When creating a custom Markup Extension that accepts parameters in WPF, the ConstructorArgument markup must be used to identify the discrete properties that match these parameters. However since this is done via a string, the compiler won’t give you any warning in case there are typos.

This rule raises an issue when the string argument to ConstructorArgumentAttribute doesn’t match any parameter of any constructor.

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.

The AssemblyVersion attribute is used to specify the version number of an assembly. An assembly is a compiled unit of code, which can be marked with a version number by applying the attribute to an assembly’s source code file.

The AssemblyVersion attribute is useful for many reasons:

If no AssemblyVersion is provided, the same default version will be used for every build. Since the version number is used by .NET Framework to uniquely identify an assembly, this can lead to broken dependencies.

The Attributed Programming Model, also known as Attribute-oriented programming (@OP), is a programming model used to embed attributes within codes.

In this model, objects are required to conform to a specific structure so that they can be used by the Managed Extensibility Framework (MEF).

MEF provides a way to discover available components implicitly, via composition. A MEF component, called a part, declaratively specifies:

The ExportAttribute declares that a part “exports”, or provides to the composition container, an object that fulfills a particular contract.

During composition, parts with imports that have matching contracts will have those dependencies filled by the exported object.

If the type doesn’t implement the interface it is exporting there will be an issue at runtime (either a cast exception or just a container not filled with the exported type) leading to unexpected behaviors/crashes.

The rule raises an issue when a class doesn’t implement or inherit the type declared in the ExportAttribute.

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.

A naming convention in software development is a set of guidelines for naming code elements like variables, functions, and classes. {identifier_capital_plural} 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 {identifier} names match a provided regular expression.

Date and time should not be used as a type for primary keys

Exception types should be “Public"

"Obsolete” attributes should include explanations

Having two Cases in the same Select statement or branches in the same If structure with the same implementation is at best duplicate code, and at worst a coding error.