Vbnet

Invalid casts should be avoided

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:

Public Sub Method(Value As Object)
Dim i As Integer
i = DirectCast(Value, Integer)  ' Direct casting from object holding an integer type to Integer
i = CType(Value, Integer)       ' Conversion from the underlying type to Integer
End Sub

Return statements should be used instead of assigning values to function names

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.

Public Function FunctionName() As Integer
FunctionName = 42 ' Noncompliant
End Function

Public Function FunctionNameFromVariable() As Integer
Dim Value As Integer = 42
FunctionNameFromVariable = Value ' Noncompliant
End Function

Arrays should not be created for ParamArray parameters

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.

Class SurroundingClass
Public Sub Base()
    Method(New String() { "s1", "s2" }) ' Noncompliant: unnecessary
    Method(New String(12) {}) ' Compliant
End Sub

Public Sub Method(ParamArray args As String())
    ' Do something
End Sub
End Class

Short-circuit logic should be used in boolean contexts

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.

If GetTrue() Or GetFalse() Then ' Noncompliant: both sides evaluated
End If

Expressions should not be too complex

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

If ((condition1 AndAlso condition2) OrElse (condition3 AndAlso condition4)) AndAlso condition5) Then  'Noncompliant
...
End If

Indexed properties should be named Item

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).

Module Module1
Dim array = {"apple", "banana", "orange", "strawberry"}

ReadOnly Property Foo(ByVal index As Integer)  ' Noncompliant
    Get
        Return array(index)
    End Get
End Property
End Module

Loops with at most one iteration should be refactored

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:

Public Function Method(items As IEnumerable(Of Object)) As Object
For i As Integer = 0 To 9
    Console.WriteLine(i)
    Exit For ' Noncompliant: loop only executes once
Next

For Each item As Object In items
    Return item ' Noncompliant: loop only executes once
Next
Return Nothing
End Function

With statements should be used for a series of calls to the same object

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

Module Module1
Dim product = New With {.Name = "paperclips", .RetailPrice = 1.2, .WholesalePrice = 0.6, .A = 0, .B = 0, .C = 0}

Sub Main()
    product.Name = ""           ' Noncompliant
    product.RetailPrice = 0
    product.WholesalePrice = 0
    product.A = 0
    product.B = 0
    product.C = 0
End Sub
End Module

Windows Forms entry points should be marked with STAThread

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.

Imports System.Windows.Forms

Public Class Foo
Shared Sub Main()
Dim winForm As Form = New Form
Application.Run(winForm)
End Sub
End Class

Non-private constants should comply with a naming convention

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:

Pascal casing, starting with an upper case character, e.g. BackColor
Short abbreviations of 2 letters can be capitalized, e.g. GetID
Longer abbreviations need to be lower cased, e.g. GetHtml

Module Module1
Public Const foo = 0  ' Noncompliant
End Module

DebuggerDisplayAttribute strings should reference existing members

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.

<DebuggerDisplay("Name: {Name}")> ' Noncompliant - Name doesn't exist in this context
Public Class Person

Public Property FullName As String

End Class

Events should comply with a naming convention

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:

Pascal casing, starting with an upper case character, e.g. BackColor
Short abbreviations of 2 letters can be capitalized, e.g. GetID
Longer abbreviations need to be lower cased, e.g. GetHtml

Class Foo
Event fooEvent() ' Noncompliant
End Class

NameOf should be used

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.

Public Sub DoSomething(param As Integer, secondParam As String)
If (param < 0) 
    Throw New Exception("param") ' Noncompliant
End If
If secondParam is Nothing
  Throw New Exception("secondParam should be valid") ' Noncompliant
End If
End Sub

Thread.Sleep should not be used in tests

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.

<TestMethod>
Public Sub SomeTest()
Threading.Thread.Sleep(500) ' Noncompliant
' assertions...
End Sub

Exceptions should not be created without being thrown

Value types should implement IEquatable<T>

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.

Structure MyStruct ' Noncompliant

Public Property Value As Integer

End Structure

Exit statements should not be used

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.

Public Class Sample
Dim condition As Boolean

Public Sub MySub()
If condition Then
  Exit Sub                  ' Noncompliant
End If

For index = 1 To 10
  If index = 5 Then
      Exit For               ' Noncompliant
  End If
  ' ...
Next
End Sub
Function MyFunction() As Object
' ...
MyFunction = 42
Exit Function              ' Noncompliant
End Function
End Class

Signed types should be preferred to unsigned ones

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

Module Module1
Sub Main()
    Dim foo1 As UShort   ' Noncompliant
    Dim foo2 As UInteger ' Noncompliant
    Dim foo3 As ULong    ' Noncompliant
End Sub
End Module

Unnecessary bit operations should not be performed

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.

anyValue And -1 ' Noncompliant
anyValue        ' Compliant

ToString() method should not return Nothing

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.

Public Overrides Function ToString() As String
Return Nothing ' Noncompliant
End Function

Non-private Shared ReadOnly fields should comply with a naming convention

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:

Pascal casing, starting with an upper case character, e.g. BackColor
Short abbreviations of 2 letters can be capitalized, e.g. GetID
Longer abbreviations need to be lower cased, e.g. GetHtml

Class Foo
Public Shared ReadOnly foo As Integer  ' Noncompliant
End Class

Right operands of shift operators should be integers

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.

Dim o As Object = 5
Dim x As Integer = 5

x = o >> 5 ' Noncompliant
x = x << o ' Noncompliant

Identical expressions should not be used on both sides of a binary operator

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 >>.

If (a = a) Then
doZ()
End If

If a = b OrElse a = b Then 
doW()
End If

Dim j = 5 / 5
j = 5 \ 5 
j = 5 Mod 5 
Dim k = 5 - 5

Dim i = 42
i /= i 
i -= i

Array literals should be used instead of array creation expressions

IIf should not be used

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.

Public Class Foo
Public Sub Bar() 
    Dim var As Object = IIf(Date.Now.Year = 1999, "Lets party!", "Lets party like it is 1999!") ' Noncompliant
End Sub 
End Class

Related If/ElseIf statements should not have the same condition

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.

If param = 1 Then
OpenWindow()
ElseIf param = 2 Then
CloseWindow()
ElseIf param = 1 Then ' Noncompliant: condition has already been checked
MoveWindowToTheBackground() ' unreachable code
End If

Non-existent operators like =+ should not be used

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.

Dim target As Integer = -5
Dim num As Integer = 3

target =- num ' Noncompliant: target = -3. Is that the intended behavior?
target =+ num ' Noncompliant: target = 3

Method parameters should follow a naming convention

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:

Camel casing, starting with a lower case character, e.g. backColor
Short abbreviations of 2 letters can be capitalized only when not at the beginning, e.g. id, productID
Longer abbreviations need to be lower cased, e.g. html

Module Module1
Sub GetSomething(ByVal ID As Integer) ' Noncompliant
End Sub
End Module

Collection sizes and array length comparisons should make sense

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.

If Collection.Count >= 0 Then ... 'Noncompliant always true

If array.Length >= 0 Then ... 'Noncompliant always true

CType should be used for casting and explicit conversions

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.

Public Class Foo
Public Sub Bar(value as Object)
    Dim stringValue As String = CStr(value)
End Sub
End Class

Method parameters and caught exceptions should not be reassigned

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.

Module Module1
Sub Foo(ByVal a As Integer)
    a = 42                  ' Noncompliant
End Sub
End Module

If ... ElseIf constructs should end with Else clauses

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.

If x = 0 Then
DoSomething()
ElseIf x = 1 Then
DoSomethingElse()
End If

ByVal should not be used

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.

Sub Foo(ByVal bar As String)
' ...
End Sub

Event handlers should comply with a naming convention

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:

Either in Pascal case, i.e. starting with an upper case letter, e.g. OnMyButtonClicked
Or, a subject, in Pascal or camel case, followed by an underscore followed by an event name, in Pascal case, e.g. btn1_Clicked

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

Module Module1
Sub subject__SomeEvent() Handles X.SomeEvent   ' Noncompliant - two underscores
End Sub
End Module

The & operator should be used to concatenate strings

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.

Module Module1
Sub Main()
    Console.WriteLine("1" + 2) ' Noncompliant - will display "3"
End Sub
End Module

Enumeration types should comply with a naming convention

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:

Pascal casing, starting with an upper case character, e.g. BackColor
Short abbreviations of 2 letters can be capitalized, e.g. GetID
Longer abbreviations need to be lower case, e.g. GetHtml
If the enum is marked as [Flags] then its name should be plural (e.g. MyOptions), otherwise, names should be singular (e.g. MyOption)

Public Enum foo ' Noncompliant
FooValue = 0
End Enum

Extension methods should not extend Object

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.

Imports System.Runtime.CompilerServices

Module MyExtensions
<Extension>
Sub SomeExtension(obj As Object) ' Noncompliant
    ' ...
End Sub
End Module

Flags enumerations zero-value members should be named None

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:

<Flags()>
Enum Days
Monday = 1    ' 0b00000001
Tuesday = 2   ' 0b00000010
Wednesday = 4 ' 0b00000100
Thursday = 8  ' 0b00001000
Friday = 16   ' 0b00010000
Saturday = 32 ' 0b00100000
Sunday = 64   ' 0b01000000
End Enum

Not boolean operator should not be repeated

StringBuilder data should be used

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.

Public Sub DoSomething(ByVal strings As List(Of String))
Dim sb As StringBuilder = New StringBuilder() ' Noncompliant
sb.Append("Got: ")

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

Line continuations should not be used

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

Module Module1
Sub Main()
    ' Noncompliant
    Console.WriteLine("Hello" _
                      & "world")
End Sub
End Module

Control flow statements If, For, For Each, Do, While, Select and Try should not be nested too deeply

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.

If condition1 ' Compliant - depth = 1
' ...
If condition2 ' Compliant - depth = 2
' ...
For i = 0 to 10 ' Compliant - depth = 3, not exceeding the limit
  ' ...
  If condition4 ' Noncompliant - depth = 4 
    If condition5 ' Depth = 5, exceeding the limit, but issues are only reported on depth = 4
      ' ...
    End If
    Return
  End If
Next
End If
End If

Type inheritance should not be recursive

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:

Function NthFibonacciNumber(ByVal n As Integer) As Integer
If n <= 1 Then
    Return 1
Else
    Return NthFibonacciNumber(n - 1) + NthFibonacciNumber(n - 2)
End If
End Function

catch clauses should do more than rethrow

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

Dim s As String = ""
Try
s = File.ReadAllText(fileName)
Catch e As Exception
Throw
End Try

Private Shared ReadOnly fields should comply with a naming convention

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:

Optionally, can start with an underscore character or “s_”, e.g. _foo, s_foo`
Camel casing, starting with a lower case character, e.g. backColor
Short abbreviations of 2 letters can be capitalized only when not at the beginning, e.g. “id” in productID
Longer abbreviations need to be lower cased, e.g. html

Class Foo
Private Shared ReadOnly Foo As Integer  ' Noncompliant
End Class

Empty nullable value should not be accessed

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.

Sub Sample(condition As Boolean)
Dim nullableValue As Integer? = If(condition, 42, Nothing)
Console.WriteLine(nullableValue.Value)             ' Noncompliant: InvalidOperationException is raised

Dim nullableCast As Integer? = If(condition, 42, Nothing)
Console.WriteLine(CType(nullableCast, Integer))    ' Noncompliant: InvalidOperationException is raised
End Sub

General or reserved exceptions should never be thrown

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.

Public Sub DoSomething(obj As Object)
If obj Is Nothing Then
' Noncompliant
Throw New NullReferenceException("obj") ' Noncompliant: This reserved exception should not be thrown manually
End If
' ...
End Sub

Properties should comply with a naming convention

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:

Pascal casing, starting with an upper case character, e.g. BackColor
Short abbreviations of 2 letters can be capitalized, e.g. GetID
Longer abbreviations need to be lower cased, e.g. GetHtml

Module Module1
Public Property foo As Integer   ' Noncompliant
End Module

Property procedures should access the expected fields

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.

Class C
Private _x As Integer
Private _Y As Integer

Public ReadOnly Property Y As Integer
    Get
        Return _x ' Noncompliant: The returned field should be '_y'
    End Get
End Property
End Class

End statements should not be used

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.

Module Module1
Sub Print(ByVal str As String)
   Try
        ...
        End       ' Noncompliant
    Finally
        ' do something important here
        ...
    End Try
End Sub
End Module

Non-private fields should comply with a naming convention

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:

Pascal casing, starting with an upper case character, e.g. BackColor
Short abbreviations of 2 letters can be capitalized, e.g. GetID
Longer abbreviations need to be lower cased, e.g. GetHtml

Class Foo
Public foo As Integer  ' Noncompliant
End Class

IndexOf checks should not be for positive numbers

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

strings.IndexOf(someString) = -1  ' Test for "index not found"
strings.IndexOf(someString) < 0   ' Test for "index not found"
strings.IndexOf(someString) >= 0  ' Test for "index found"

Select Case statements should not have too many Case clauses

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.

Public Class TooManyCase

Public Function MapValues(Ch As Char) As Integer 
    Select Case Ch ' Noncompliant: 5 cases, "Case Else" excluded, more than maximum = 4
        Case "a"c
            Return 1
        Case "b"c, "c"c
            Return 2
        Case "d"c
            Return 3
        Case "e"c
            Return 4
        Case "f"c, "g"c, "h"c
            Return 5
        Case Else
            Return 6
    End Select
End Function

End Class

GoTo statements should not be used

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.

Sub GoToStatementDemo()
    Dim number As Integer = 1
    Dim sampleString As String
    ' Evaluate number and branch to appropriate label.
    If number = 1 Then GoTo Line1 Else GoTo Line2
Line1:
    sampleString = "Number equals 1"
    GoTo LastLine
Line2:
    ' The following statement never gets executed because number = 1.
    sampleString = "Number equals 2"
LastLine:
    ' Write "Number equals 1" in the Debug window.
    Debug.WriteLine(sampleString)
End Sub

Private constants should comply with a naming convention

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:

Optionally, can start with an underscore character or “s_”, e.g. _foo, s_foo`
Camel casing, starting with a lower case character, e.g. backColor
Short abbreviations of 2 letters can be capitalized only when not at the beginning, e.g. “id” in productID
Longer abbreviations need to be lower cased, e.g. html

Module Module1
Private Const Foo = 0  ' Noncompliant
End Module

Multiple variables should not be declared on the same line

Any() should be used to test for emptiness

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:

if the collection is an 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.
if the collection is part of a LINQ query that contains .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 Function HasContent(Strings As IEnumerable(Of String)) As Boolean
Return Strings.Count() > 0      ' Noncompliant
End Function

Private Function HasContent2(Strings As IEnumerable(Of String)) As Boolean
Return Strings.Count() >= 1     ' Noncompliant
End Function

Private Function IsEmpty(Strings As IEnumerable(Of String)) As Boolean
Return Strings.Count() = 0      ' Noncompliant
End Function

Select...Case clauses should not have too many lines of code

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.

Select Case number
Case 1 To 5 ' Noncompliant: 4 statements in the case
    MethodCall1("")
    MethodCall2("")
    MethodCall3("")
    MethodCall4("")
Case Else
    ' ...
End Select

Functions and procedures should comply with a naming convention

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:

Pascal casing, starting with an upper case character, e.g. BackColor
Short abbreviations of 2 letters can be capitalized, e.g. GetID
Longer abbreviations need to be lower cased, e.g. GetHtml
Event handlers with a handles clause and two-parameter methods with EventArgs second parameter are not covered by this rule.

Module Module1
Sub bad_subroutine()                      ' Noncompliant
End Sub

Public Function Bad_Function() As Integer ' Noncompliant
Return 42
End Function
End Module

All branches in a conditional structure should not have exactly the same implementation

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

In the following code:

Dim b As Integer = If(a > 12, 4, 4)  // Noncompliant

If b = 0 Then  // Noncompliant
DoTheThing()
Else
DoTheThing()
End If

Select Case i  // Noncompliant
Case 1
    DoSomething()
Case 2
    DoSomething()
Case 3
    DoSomething()
Case Else
    DoSomething()
End Select

Interface names should comply with a naming convention

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:

Must start with an upper case ‘I’ character, e.g. IFoo
Followed by Pascal casing, starting with an upper case character, e.g. IEnumerable
Short abbreviations of 2 letters can be capitalized, e.g. IFooID
Longer abbreviations need to be lower cased, e.g. IFooHtml

Interface Foo  ' Noncompliant
End Interface

Do loops should not be used without a While or Until condition

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.

Module Module1
Sub Main()
    Dim i = 1

    Do                        ' Non-Compliant
        If i = 10 Then
            Exit Do
        End If

        Console.WriteLine(i)

        i = i + 1
    Loop
End Sub
End Module

Generic type parameter names should comply with a naming convention

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:

Generic type parameter names must start with an upper case ‘T’, e.g. T
The rest of the name should use Pascal casing, starting with an upper case character, e.g. TKey
Short abbreviations of 2 letters can be capitalized, e.g. TFooID
Longer abbreviations should be lowercased, e.g. TFooHtml

Public Class Foo(Of tkey) ' Noncompliant
End Class

Arguments of public methods should be validated against Nothing

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.

Public Class Sample

Public Property Message As String

Public Sub PublicMethod(Arg As Exception)
    Message = Arg.Message   ' Noncompliant
End Sub

Protected Sub ProtectedMethod(Arg As Exception)
    Message = Arg.Message   ' Noncompliant
End Sub

End Class

Null checks should not be used with TypeOf Is

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.

If (x IsNot Nothing And TypeOf x Is MyClass)
' ...
End If

If (x Is Nothing Or TypeOf x IsNot MyClass)
' ...
End If

Classes should not be empty

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.

Public Class Empty ' Noncompliant

End Class

Exit Select statements should not be used redundantly

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.

Module Module1
Sub Main()
Dim x = 0
Select Case x
  Case 0
    Console.WriteLine("0")
    Exit Select                ' Noncompliant
  Case Else
    Console.WriteLine("Not 0")
    Exit Select                ' Noncompliant
End Select
End Sub
End Module

Indexed properties with more than one parameter should not be used

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.

Module Module1
ReadOnly Property Sum(ByVal a As Integer, ByVal b As Integer) ' Noncompliant
    Get
        Return a + b
    End Get
End Property
End Module

Enumeration values should comply with a naming convention

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:

Pascal casing, starting with an upper case character, e.g. BackColor
Short abbreviations of 2 letters can be capitalized, e.g. GetID
Longer abbreviations need to be lower cased, e.g. GetHtml

Enum Foo
fooValue   ' Noncompliant
End Enum

IsNot should be used instead of Not ... Is ...

On Error statements should not be used

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.

Sub DivideByZero()
On Error GoTo nextstep
Dim result As Integer
Dim num As Integer
num = 100
result = num / 0
nextstep:
System.Console.WriteLine("Error")
End Sub

Private fields should comply with a naming convention

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:

Optionally, can start with an underscore character or “s_”, e.g. _foo, s_foo`
Camel casing, starting with a lower case character, e.g. backColor
Short abbreviations of 2 letters can be capitalized only when not at the beginning, e.g. “id” in productID
Longer abbreviations need to be lower cased, e.g. html

Class Foo
Private Foo As Integer  ' Noncompliant
End Class

Arrays should be initialized using the ... = {} syntax

Namespace names should comply with a naming convention

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:

Pascal casing, starting with an upper case character, e.g. Microsoft, System
Short abbreviations of 2 letters can be capitalized, e.g. System.IO
Longer abbreviations need to be lower cased

Namespace foo  ' Noncompliant
End Namespace

Strings should not be concatenated using + or & in a loop

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

Module Module1
Sub Main()
    Dim foo = ""
    foo &= "Result: "       ' Compliant - outside of loop

    For i = 1 To 9
        foo &= i            ' Noncompliant
    Next
End Sub
End Module

Calculations should not overflow

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.

Public Function Transform(Value As Integer) As Integer
If Value <= 0 Then Return Value
Dim Number As Integer = Integer.MaxValue
Return Number + Value       ' Noncompliant
End Function

Array.Empty(Of TElement) should be used to instantiate empty arrays

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.

Public Sub Method()
Dim Values1(-1) As Integer ' Noncompliant
Dim Values2 As Integer() = New Integer() {} ' Noncompliant
Dim Values3 As Integer() = {} ' Noncompliant
Dim Values4() As Integer = {} ' Noncompliant
End Sub

Select statements should end with a Case Else clause

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.

Select Case param ' Noncompliant - Case Else clause is missing
Case 0
DoSomething()
Case 1
DoSomethingElse()
End Select

Select Case statements should not be nested

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.

Public Sub Foo(A As Integer, B As Integer)
Select Case A
    Case 0
        ' ...
    Case 1
        Select Case B   ' Noncompliant; nested Select Case
            Case 2
                ' ...
            Case 3
                ' ...
            Case 4
                ' ...
            Case Else
                ' ...
        End Select
    Case 2
        ' ...
    Case Else
        ' ...
End Select
End Sub

Explicit Event subscriptions should be explicitly unsubscribed.

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`.

Class MyEventProcucer
Public Shared Event EventFired As EventHandler
End Class

Public Class MyEventSubscriber
Implements IDisposable

Public Sub New()
    AddHandler MyEventProcucer.EventFired, AddressOf c_EventFired  'Noncompliant
End Sub

Private Sub c_EventFired(sender As Object, e As EventArgs)
End Sub

Public Sub Dispose() Implements IDisposable.Dispose
End Sub
End Class

Parameters should not be passed by reference

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.

Module Module1
Sub Foo(ByRef result As Integer) ' Non-Compliant
    result = 42
End Sub

Sub Main()
    Dim result As Integer
    Foo(result)
    Console.WriteLine(result)
End Sub
End Module

Event names should not have Before or After as a prefix or suffix

”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.

Class Foo
Event BeforeClose() ' Noncompliant
Event AfterClose()  ' Noncompliant
End Class

Use a testable date/time provider

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.

Public Class Foo
Public Function HelloTime() As String
    Return $"Hello at {DateTime.UtcNow}"
End Function
End Class

Locks should be released on all paths

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.

Class Example
Private obj As Object = New Object()

Public Sub DoSomethingWithMonitor()
    Monitor.Enter(obj) ' Noncompliant: not all paths release the lock

    If IsInitialized() Then
        ' ...
        Monitor.Exit(obj)
    End If
End Sub
End Class

Boolean checks should not be inverted

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

If Not (a = 2) Then  // Noncompliant
Dim b as Boolean = Not (i < 10)  // Noncompliant

Regular expressions should be syntactically valid

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.

Sub Regexes(Input As String)
Dim Rx As New Regex("[A")                                                       ' Noncompliant: unmatched "["
Dim Match = Regex.Match(Input, "[A")                                            ' Noncompliant
Dim NegativeLookahead As New Regex("a(?!b)", RegexOptions.NonBacktracking)      ' Noncompliant: negative lookahead without backtracking
Dim NegativeLookbehind As New Regex("(?<!a)b", RegexOptions.NonBacktracking)    ' Noncompliant: negative lookbehind without backtracking
End Sub

Using publicly writable directories is security-sensitive

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.

/tmp
/var/tmp
/usr/tmp
/dev/shm
/dev/mqueue
/run/lock
/var/run/lock
/Library/Caches
/Users/Shared
/private/tmp
/private/var/tmp
\Windows\Temp
\Temp
\TMP`

Dim RandomPath = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName())

' Creates a new file with write, non inheritable permissions which is deleted on close.
Using FileStream As New FileStream(RandomPath, FileMode.CreateNew, FileAccess.Write, FileShare.None, 4096, FileOptions.DeleteOnClose)
Using Writer As New StreamWriter(FileStream) ' Sensitive
' ...
End Using
End Using

Statements should be on separate lines

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}[]

Dim a = 0 : Dim b = 0  ' Noncompliant

Enumeration type names should not have Flags or Enum suffixes

Searching OS commands in PATH is security-sensitive

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.

Dim p As New Process()
p.StartInfo.FileName = "C:\Apps\binary.exe" ' Compliant

Having a permissive Cross-Origin Resource Sharing policy is security-sensitive

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.

Response.AppendHeader("Access-Control-Allow-Origin", "trustedwebsite.com") // Compliant

Map values should not be replaced unconditionally

One-way OperationContract methods should have void return type

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.

<ServiceContract>
Interface IMyService
<OperationContract(IsOneWay:=True)>
Function SomethingHappened(ByVal parameter As Integer) As Integer ' Noncompliant
End Interface

Delivering code in production with debug features activated is security-sensitive

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.

Imports Microsoft.AspNetCore.Builder
Imports Microsoft.AspNetCore.Hosting

Namespace MyMvcApp
Public Class Startup
    Public Sub Configure(ByVal app As IApplicationBuilder, ByVal env As IHostingEnvironment)
        ' Those calls are Sensitive because it seems that they will run in production
        app.UseDeveloperExceptionPage() 'Sensitive
        app.UseDatabaseErrorPage() 'Sensitive
    End Sub
End Class
End Namespace

Boolean literals should not be redundant

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.

If BooleanMethod() = True Then ' Noncompliant
' ...
End If
If BooleanMethod() = False Then ' Noncompliant
' ...
End If
If BooleanMethod() OrElse False Then ' Noncompliant
' ...
End If
DoSomething(Not False) ' Noncompliant
DoSomething(BooleanMethod() = True) ' Noncompliant

Dim booleanVariable = If(BooleanMethod(), True, False) ' Noncompliant
booleanVariable = If(BooleanMethod(), True, exp) ' Noncompliant
booleanVariable = If(BooleanMethod(), False, exp) ' Noncompliant
booleanVariable = If(BooleanMethod(), exp, True) ' Noncompliant
booleanVariable = If(BooleanMethod(), exp, False) ' Noncompliant

Deprecated code should be removed

Write-only properties should not be used

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.

Module Module1
WriteOnly Property Foo() As Integer ' Non-Compliant
    Set(ByVal value As Integer)
        ' ... some code ...
    End Set
End Property
End Module

Optional parameters should be passed to base calls

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.

Public Class BaseClass
Public Overridable Sub MyMethod(ByVal Optional i As Integer = 1)
    Console.WriteLine(i)
End Sub
End Class

Public Class DerivedClass
Inherits BaseClass

Public Overrides Sub MyMethod(ByVal Optional i As Integer = 1)
    ' ...
    MyBase.MyMethod() ' Noncompliant: caller's value is ignored
End Sub

Private Shared Function Main(ByVal args As String()) As Integer
    Dim dc As DerivedClass = New DerivedClass()
    dc.MyMethod(12) ' prints 1
End Function
End Class

Fields should be private

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.

Class Foo
Public Foo = 42          ' Noncompliant
End Class

Expanding archive files without controlling resource consumption is security-sensitive

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).

Const ThresholdRatio As Double = 10
Const ThresholdSize As Integer = 1024 * 1024 * 1024 ' 1 GB
Const ThresholdEntries As Integer = 10000
Dim TotalSizeArchive, TotalEntryArchive, TotalEntrySize, Cnt As Integer
Dim Buffer(1023) As Byte
Using ZipToOpen As New FileStream("ZipBomb.zip", FileMode.Open), Archive As New ZipArchive(ZipToOpen, ZipArchiveMode.Read)
For Each Entry As ZipArchiveEntry In Archive.Entries
    Using s As Stream = Entry.Open
        TotalEntryArchive += 1
        TotalEntrySize = 0
        Do
            Cnt = s.Read(Buffer, 0, Buffer.Length)
            TotalEntrySize += Cnt
            TotalSizeArchive += Cnt
            If TotalEntrySize / Entry.CompressedLength > ThresholdRatio Then Exit Do    ' Ratio between compressed And uncompressed data Is highly suspicious, looks Like a Zip Bomb Attack
        Loop While Cnt > 0
    End Using
    If TotalSizeArchive > ThresholdSize Then Exit For       ' The uncompressed data size Is too much for the application resource capacity
    If TotalEntryArchive > ThresholdEntries Then Exit For   ' Too much entries in this archive, can lead to inodes exhaustion of the system
Next
End Using

Child class fields should not differ from parent class fields only by capitalization

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.

Public Class Fruit

Protected PlantingSeason As String

' ...

End Class

Public Class Raspberry
Inherits Fruit

Protected Plantingseason As String  ' Noncompliant

' ...

End Class

Mergeable if statements should be combined

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.

If condition1 Then
If condition2 Then ' Noncompliant
    ' ...
End If
End If

Methods should not be empty

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:

It is an unintentional omission, and should be fixed to prevent an unexpected behavior in production.
It is not yet, or never will be, supported. In this case an exception should be thrown.
The method is an intentionally-blank override. In this case a nested comment should explain the reason for the blank override.

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

Using clear-text protocols is security-sensitive

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:

sensitive data exposure
traffic redirected to a malicious endpoint
malware-infected software update or installer
execution of client-side code
corruption of critical information

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:

bypassing isolation mechanisms
compromising a component of the network
getting the credentials of an internal IAM account (either from a service account or an actual person)

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:

Dim UrlHttp As String = "http://example.com"                ' Noncompliant
Dim UrlFtp As String = "ftp://anonymous@example.com"        ' Noncompliant
Dim UrlTelnet As String = "telnet://anonymous@example.com"  ' Noncompliant

Redundant pairs of parentheses should be removed

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.

If a AndAlso ((x + y > 0)) Then ' Noncompliant
' ...
End If

Return ((x + 1))  ' Noncompliant

Array designators () should be on the type, not the variable

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.

Module Module1
Sub Main()
    Dim foo() As String ' Noncompliant
End Sub
End Module

Using shell interpreter when executing OS commands is security-sensitive

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.

Imports System.Security
Imports System.Diagnostics

Namespace N
Class A
    Public Sub Foo(ByVal process As Process)
        Process.Start("/usr/bin/file.exe") ' Compliant
    End Sub
End Class
End Namespace

Allowing requests with excessive content length is security-sensitive

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.

Imports Microsoft.AspNetCore.Mvc

Public Class MyController
Inherits Controller

<HttpPost>
<DisableRequestSizeLimit> ' Sensitive: No size  limit
<RequestSizeLimit(10485760)> ' Sensitive: 10485760 B = 10240 KB = 10 MB is more than the recommended limit of 8MB
Public Function PostRequest(Model model) As IActionResult
' ...
End Function

<HttpPost>
<RequestFormLimits(MultipartBodyLengthLimit = 10485760)> ' Sensitive: 10485760 B = 10240 KB = 10 MB is more than the recommended limit of 8MB
Public Function MultipartFormRequest(Model model) As IActionResult
' ...
End Function

End Class

Formatting SQL queries is security-sensitive

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.

Public Sub Foo(ByVal context As DbContext, ByVal value As String)
context.Database.ExecuteSqlCommand("SELECT * FROM mytable WHERE mycol=@p0", value) ' Compliant, the query is parameterized
End Sub

Unused procedure parameters should be removed

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.

Private Sub DoSomething(ByVal a As Integer, ByVal b as Integer) ' "b" is unused
Compute(a)
End Sub

Private Function DoSomething2(ByVal a As Integer, ByVal b As Integer) As Integer ' "a" is unused 
Compute(b)
Return b
End Function

Class names should comply with a naming convention

Assemblies should explicitly specify COM visibility

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.

Namespace MyLibrary  ' Noncompliant

End Namespace

Select statements should have at least 3 Case clauses

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.

Select Case variable
Case 0
    doSomething()
Case Else
    doSomethingElse()
End Select

Conditionally executed code should be reachable

Variables should not be self-assigned

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.

Public Sub SetName(name As String)
name = name ' Noncompliant
End Sub

Serialization event handlers should be implemented correctly

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.

<Serializable>
Public Class Foo
<OnSerializing>
Public Sub OnSerializing(ByVal context As StreamingContext) ' Noncompliant: should be private
End Sub

<OnSerialized>
Private Function OnSerialized(ByVal context As StreamingContext) As Integer '  Noncompliant: should return void
End Function

<OnDeserializing>
Private Sub OnDeserializing() ' Noncompliant: should have a single parameter of type StreamingContext
End Sub

<OnSerializing>
Public Sub OnSerializing2(Of T)(ByVal context As StreamingContext) ' Noncompliant: should have no type parameters
End Sub

<OnDeserialized>
Private Sub OnDeserialized(ByVal context As StreamingContext, ByVal str As String) ' Noncompliant: should have a single parameter of type StreamingContext
End Sub
End Class

Assemblies should be marked as CLS compliant

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.

<Assembly: CLSCompliant(True)>

Namespace MyLibrary

End Namespace

Public constant members should not be used

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.

Public Class Foo
Public Const Version = 1.0           ' Noncompliant
End Class

Classes should not be coupled to too many other classes

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.

Class Foo        ' Non-Compliant - depends on too many other classes
Dim a1 As T1   ' Foo is coupled to T1
Dim a2 As T2   ' Foo is coupled to T2
Dim a3 As T3   ' Foo is coupled to T3
Dim a4 As T4   ' etc.
Dim a5 As T5
Dim a6 As T6
Dim a7 As T7
Dim a8 As T8
Dim a9 As T9
Dim a10 As T10
Dim a11 As T11
Dim a12 As T12
Dim a13 As T13
Dim a14 As T14
Dim a15 As T15
Dim a16 As T16
Dim a17 As T17
Dim a18 As T18
Dim a19 As T19
Dim a20 As T20
Dim a21 As T21
End Class

If operators should not be nested

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}[]

Public Function GetReadableStatus(job As Job) As String
Return If(job.IsRunning, "Running", If(job.HasErrors, "Failed", "Succeeded")) ' Noncompliant
End Function

Classes named like Exception should extend Exception or a subclass

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.

Public Class FruitException ' Noncompliant - this has nothing to do with Exception
Private expected As Fruit
Private unusualCharacteristics As String
Private appropriateForCommercialExploitation As Boolean
' ...
End Class

Public Class CarException ' Noncompliant - does not derive from any Exception-based class
Public Sub New(message As String, inner As Exception)
    ' ...
End Sub
End Class

Method overloads should be grouped together

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.

Interface IMyInterface 
Function DoTheThing() As Integer
Function DoTheOtherThing() As String // Noncompliant
Function DoTheThing(ByVal Path As String) As Integer
End Interface

String literals should not be duplicated

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

Public Class Foo

Private Name As String = "foobar" ' Noncompliant

Public ReadOnly Property DefaultName As String = "foobar" ' Noncompliant

Public Sub New(Optional Value As String = "foobar") ' Noncompliant

    Dim Something = If(Value, "foobar") ' Noncompliant

End Sub

End Class

Types should be defined in named namespaces

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.

Public Class Foo
End Class

Public Structure Bar
End Structure

Reflection should not be used to increase accessibility of classes, methods, or fields

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.

Imports System.Reflection

Dim dynClass = Type.GetType("MyInternalClass")
' Sensitive. Using BindingFlags.NonPublic will return non-public members
Dim bindingAttr As BindingFlags = BindingFlags.NonPublic Or BindingFlags.Static
Dim dynMethod As MethodInfo = dynClass.GetMethod("mymethod", bindingAttr)
Dim result = dynMethod.Invoke(dynClass, Nothing)

CoSetProxyBlanket and CoInitializeSecurity should not be used

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.

Public Class Noncompliant

<DllImport("ole32.dll")>
Public Shared Function CoSetProxyBlanket(<MarshalAs(UnmanagedType.IUnknown)>pProxy As Object, dwAuthnSvc as UInt32, dwAuthzSvc As UInt32, <MarshalAs(UnmanagedType.LPWStr)> pServerPrincName As String, dwAuthnLevel As UInt32, dwImpLevel As UInt32, pAuthInfo As IntPtr, dwCapabilities As UInt32) As Integer
End Function

Public Enum RpcAuthnLevel
    [Default] = 0
    None = 1
    Connect = 2
    [Call] = 3
    Pkt = 4
    PktIntegrity = 5
    PktPrivacy = 6
End Enum

Public Enum RpcImpLevel
    [Default] = 0
    Anonymous = 1
    Identify = 2
    Impersonate = 3
    [Delegate] = 4
End Enum

Public Enum EoAuthnCap
    None = &H00
    MutualAuth = &H01
    StaticCloaking = &H20
    DynamicCloaking = &H40
    AnyAuthority = &H80
    MakeFullSIC = &H100
    [Default] = &H800
    SecureRefs = &H02
    AccessControl = &H04
    AppID = &H08
    Dynamic = &H10
    RequireFullSIC = &H200
    AutoImpersonate = &H400
    NoCustomMarshal = &H2000
    DisableAAA = &H1000
End Enum

<DllImport("ole32.dll")>
Public Shared Function CoInitializeSecurity(pVoid As IntPtr, cAuthSvc As Integer, asAuthSvc As IntPtr, pReserved1 As IntPtr, level As RpcAuthnLevel, impers As RpcImpLevel, pAuthList As IntPtr, dwCapabilities As EoAuthnCap, pReserved3 As IntPtr) As Integer
End Function

Public Sub DoSomething()
    Dim Hres1 As Integer = CoSetProxyBlanket(Nothing, 0, 0, Nothing, 0, 0, IntPtr.Zero, 0) ' Noncompliant
    Dim Hres2 As Integer = CoInitializeSecurity(IntPtr.Zero, -1, IntPtr.Zero, IntPtr.Zero, RpcAuthnLevel.None, RpcImpLevel.Impersonate, IntPtr.Zero, EoAuthnCap.None, IntPtr.Zero) ' Noncompliant
End Sub

End Class

Not specifying a timeout for regular expressions is security-sensitive

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.

Ask Yourself Whether

the input passed to the regular expression is untrusted.
the regular expression contains patterns vulnerable to catastrophic backtracking.

There is a risk if you answered yes to any of those questions.

Recommended Secure Coding Practices

It is recommended to specify a matchTimeout when executing a regular expression.
Make sure regular expressions are not vulnerable to Denial-of-Service attacks by reviewing the patterns.
Consider using a non-backtracking algorithm by specifying RegexOptions.NonBacktracking.

Public Sub RegexPattern(Input As String)
Dim EmailPattern As New Regex(".+@.+", RegexOptions.None)
Dim IsNumber as Boolean = Regex.IsMatch(Input, "[0-9]+")
Dim IsLetterA as Boolean = Regex.IsMatch(Input, "(a+)+")
End Sub

Disabling ASP.NET Request Validation feature is security-sensitive

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.

<ValidateInput(True)>
Public Function Welcome(Name As String) As ActionResult
...
End Function

Comments should not be located at the end of lines of code

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.

Module Module1
Sub Main()
Console.WriteLine("Hello, world!") ' Noncompliant - My first program!
Console.WriteLine("Hello, world!") ' CompliantOneWord
End Sub
End Module

Optional parameters should not be used

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

Optional parameter values are baked into the method call site code, thus, if a default value has been changed, all referencing assemblies need to be rebuilt, otherwise the original values will be used.
The Common Language Specification (CLS) allows compilers to ignore default parameter values, and thus require the caller to explicitly specify the values. For example, if you want to consume a method with default argument from another .NET compatible language (for instance C++/CLI), you will have to provide all arguments. When using method overloads, you could achieve similar behavior as default arguments.
Optional parameters prevent muddying the definition of the function contract. Here is a simple example: if there are two optional parameters, when one is defined, is the second one still optional or mandatory?

Sub Notify(ByVal Company As String, Optional ByVal Office As String = "QJZ") ' Noncompliant

End Sub

PartCreationPolicyAttribute should be used with ExportAttribute

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.

Imports System.ComponentModel.Composition

<PartCreationPolicy(CreationPolicy.Any)> ' Noncompliant
Public Class FooBar
Inherits IFooBar
End Class

Procedures should not have too many parameters

{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:

Split the {function} into smaller ones

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

Find a better data structure for the parameters that group data in a way that makes sense for the specific application domain

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

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

Class BaseClass

Sub New(Param1 As Integer)
    ' ...
End Sub

End Class

Class DerivedClass
Inherits BaseClass

Public Sub New(Param1 As Integer, Param2 As Integer, Param3 As Integer, Param4 As Integer, Param5 As Long)
' Compliant by exception: Param1 is used in the base class constructor, so it does not count in the sum of the parameter list.
    MyBase.New(Param1)
    ' ...
End Sub

End Class

Methods should not have identical implementations

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.

Private Const CODE As String = "bounteous"
Private callCount As Integer = 0

Public Function GetCode() As String
callCount = callCount + 1
Return CODE
End Function

Public Function GetName() As String ' Noncompliant: duplicates GetCode
callCount = callCount + 1
Return CODE
End Function

Using hardcoded IP addresses is security-sensitive

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:

The developers will have to do a rapid fix every time this happens, instead of having an operation team change a configuration file.
It misleads to use the same address in every environment (dev, sys, qa, prod).

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.

Dim ip = ConfigurationManager.AppSettings("myapplication.ip") ' Compliant
Dim address = IPAddress.Parse(ip)

new Guid() should not be used

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

An empty GUID, in which case Guid.Empty is clearer.
A randomly-generated GUID, in which case Guid.NewGuid() should be used.
A new GUID with a specific initialization, in which case the initialization parameter is missing.

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

Public Sub Foo()
Dim G1 As New Guid        ' Noncompliant - what's the intent?
Dim G2 As Guid = Nothing  ' Noncompliant
End Sub

Track uses of TODO tags

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.

Sub DoSomething() 
' TODO
End Sub

View data dictionaries should be replaced by models

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.

Public Class HomeController
Inherits Controller

Public Function Article() As ActionResult
    ViewBag.Title = "Title" ' Noncompliant, model should be used
    ViewData("Text") = "Text" ' Noncompliant, model should be used
    Return View()
End Function

End Class

ExcludeFromCodeCoverage attributes should include a justification

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.

Public Structure Coordinates

Public ReadOnly Property X As Integer
Public ReadOnly Property Y As Integer

<ExcludeFromCodeCoverage> ' Noncompliant
Public Overrides Function Equals(obj As Object) As Boolean
    If Not (TypeOf obj Is Coordinates) Then
        Return False
    End If

    Dim coordinates = DirectCast(obj, Coordinates)
    Return X = coordinates.X AndAlso
           Y = coordinates.Y
End Function

<ExcludeFromCodeCoverage> ' Noncompliant
Public Overrides Function GetHashCode() As Integer
    Dim hashCode As Long = 1861411795
    hashCode = (hashCode * -1521134295 + X.GetHashCode()).GetHashCode()
    hashCode = (hashCode * -1521134295 + Y.GetHashCode()).GetHashCode()
    Return hashCode
End Function
End Structure

Flags enumerations should explicitly initialize all their members

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.

<Flags()>
Enum FruitType    ' Non-Compliant
None
Banana
Orange
Strawberry
End Enum

Module Module1
Sub Main()
Dim bananaAndStrawberry = FruitType.Banana Or FruitType.Strawberry 
Console.WriteLine(bananaAndStrawberry.ToString()) ' Will display only "Strawberry"
End Sub
End Module

Setting loose POSIX file permissions is security-sensitive

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.

Dim safeAccessRule = new FileSystemAccessRule("Everyone", FileSystemRights.FullControl, AccessControlType.Deny)

Dim fileSecurity = File.GetAccessControl("path")
fileSecurity.AddAccessRule(safeAccessRule)
File.SetAccessControl("path", fileSecurity)

Useless if(true) {...} and if(false){...} blocks should be removed

`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:

An if statement was changed during debugging and that debug code has been committed.
Some value was left unset.
Some logic is not doing what the programmer thought it did.

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

If True Then ' Noncompliant
  DoSomething()
End If

If False Then ' Noncompliant
  DoSomethingElse()
End If

Child class fields should not shadow parent class fields

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.

Public Class Fruit

Protected Ripe As Season
Protected Flesh As Color

' ...

End Class

Public Class Raspberry
Inherits Fruit

Private Ripe As Boolean         ' Noncompliant
Private Shared FLESH As Color   ' Noncompliant

' ...

End Class

Using weak hashing algorithms is security-sensitive

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).

Imports System.Security.Cryptography

Sub ComputeHash()
Dim sha256 = New SHA256CryptoServiceProvider() ' Compliant
Dim sha384 = New SHA384CryptoServiceProvider() ' Compliant
Dim sha512 = New SHA512CryptoServiceProvider() ' Compliant

' ...
End Sub

Exception constructors should not throw exceptions

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.

Class MyException
Inherits Exception

Public Sub MyException()
    If bad_thing Then
        Throw New Exception("A bad thing happened")
    End If
End Sub
End Class

Deserialization methods should be provided for OptionalField members

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.

<Serializable>
Public Class Foo ' Noncompliant
<OptionalField(VersionAdded:=2)>
Private optionalField As Integer = 5
End Class

Using non-standard cryptographic algorithms is security-sensitive

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.

Dim mySHA256 As SHA256 = SHA256.Create()

Hard-coded credentials are security-sensitive

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”, …

Dim username As String = "admin"
Dim password As String = GetEncryptedPassword()
Dim usernamePassword As String = String.Format("user={0}&password={1}", GetEncryptedUsername(), GetEncryptedPassword())
Dim url As String = $"scheme://{username}:{password}@domain.com"

Dim url2 As String= "http://guest:guest@domain.com" ' Compliant
Const Password_Property As String = "custom.password" ' Compliant

Duplicate values should not be passed as arguments

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.

If compare(myPoint.x, myPoint.x) <> 0 Then ' Noncompliant
'...
End If

If compare(getNextValue(), getNextValue()) <> 0 Then ' Noncompliant
'...
End If

Methods with Pure attribute should return a value

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.

Class Person
Private age As Integer

<Pure> ' Noncompliant: The method makes a state change
Private Sub ConfigureAge(ByVal age As Integer)
    Me.age = age
End Sub

<Pure>
Private Sub WriteAge() ' Noncompliant
    Console.WriteLine(Me.age)
End Sub

End Class

[ExpectedException] should not be used

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.

<TestMethod>
<ExpectedException(GetType(ArgumentNullException))> ' Noncompliant
Public Sub TestNullArg()
'...
End Sub

Unused local variables should be removed

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.

Using t = New TestTimer()
End Using

Objects should not be disposed more than once

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.

Dim foo As New Disposable()
foo.Dispose()
foo.Dispose() ' Noncompliant

Exceptions should not be thrown in finally blocks

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.

Try
' Some work which end up throwing an exception
Throw New ArgumentException()
Finally
' Cleanup
Throw New InvalidOperationException() ' Noncompliant: will mask the ArgumentException
End Try

ConstructorArgument parameters should exist in constructors

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.

Imports System

Namespace MyLibrary
Public Class MyExtension
    Inherits MarkupExtension

    Public Sub New()
    End Sub

    Public Sub New(ByVal value1 As Object)
        Value1 = value1
    End Sub

    <ConstructorArgument("value2")> ' Noncompliant
    Public Property Value1 As Object
End Class
End Namespace

Cognitive Complexity of methods should not be too high

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.

Function Abs(ByVal n As Integer) As Integer ' Noncompliant: cognitive complexity = 5
If n >= 0 Then    ' +1
    Return n
Else              ' +2, due to nesting
    If n = Integer.MinValue Then      ' +1
        Throw New ArgumentException("The absolute value of int.MinValue is outside of int boundaries")
    Else                              ' +1
        Return -n
    End If
End If
End Function

Assemblies should have version information

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:

Versioning: The attribute allows developers to track and manage different versions of an assembly. By incrementing the version number for each new release, you can easily identify and differentiate between different versions of the same assembly. This is particularly useful when distributing and deploying software, as it helps manage updates and compatibility between different versions.
Dependency management: When an assembly references another assembly, it can specify the specific version of the dependency it requires. By using the AssemblyVersion attribute, you can ensure that the correct version of the referenced assembly is used. This helps avoid compatibility issues and ensures that the expected behavior and functionality are maintained.
GAC management: The GAC, also known as Global Assembly Cache, is a central repository for storing shared assemblies on a system. The AssemblyVersion attribute plays a crucial role in managing assemblies in the GAC. Different versions of an assembly can coexist in the GAC, allowing applications to use the specific version they require.

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.

Imports System.Reflection
<Assembly: AssemblyTitle("MyAssembly")> ' Noncompliant
Namespace MyLibrary
' ...
End Namespace

Classes should implement their ExportAttribute interfaces

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:

both its dependencies, known as imports
and what capabilities it makes available, known as exports

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.

<Export(GetType(ISomeType))>
Public Class SomeType  ' Noncompliant: doesn't implement 'ISomeType'.
End Class

Boolean expressions should not be gratuitous

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.

Dim d As String = Nothing
Dim v1 = If(d, "value")

Local variable names should comply with a naming convention

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.

Module Module1
Sub Main()
    Dim Foo = 0 ' Noncompliant
End Sub
End Module

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

Exception types should be Public

Obsolete attributes should include explanations

Two branches in a conditional structure should not have exactly the same implementation

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.

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

Start Here

Setup

Pull Request Review

Scan center

Integrations

IDE

Rule Reference

Resources

Ask Yourself Whether

Recommended Secure Coding Practices