data.All(x => { x.Property = “foo”; return true; });
Get Started
- CodeAnt AI
- Control Center
- Pull Request Review
- IDE
- Compliance
- Anti-Patterns
- Code Governance
- Infrastructure Security Database
- Application Security Database
C #
Jump statements, such as return, yield break, goto, and continue
let you change the default flow of program execution, but jump statements that direct the control flow to the original direction are just a waste of keystrokes.
A cast is an explicit conversion, which is a way to tell the compiler the intent to convert from one type to another.
When a class implements the IEquatable<T> interface, it enters a contract that, in effect, states “I know how to compare two instances of type T or any type derived from T for equality.”. However if that class is derived, it is very unlikely that the base class will know how to make a meaningful comparison. Therefore that implicit contract is now broken.
Alternatively IEqualityComparer<T> provides a safer interface and is used by collections or Equals could be made virtual.
This rule raises an issue when an unsealed, public or protected class implements IEquatable<T> and the Equals is neither virtual nor abstract.
Caller information attributes provide a way to get information about the caller of a method through optional parameters. But they only work right if their values aren’t provided explicitly. So if you define a method with caller info attributes in the middle of the parameter list, the caller is forced to use named arguments if they want to use the method properly.
This rule raises an issue when the following attributes are used on parameters before the end of the parameter list:
Passing a parameter by reference, which is what happens when you use the `out or ref parameter modifiers, means that the method will receive a pointer to the argument, rather than the argument itself. If the argument was a value type, the method will be able to change the argument’s values. If it was a reference type, then the method receives a pointer to a pointer, which is usually not what was intended. Even when it is what was intended, this is the sort of thing that’s difficult to get right, and should be used with caution.
This rule raises an issue when out or ref is used on a non-Optional parameter in a public method. Optional` parameters are covered by S3447.
It is important to be careful when searching for characters within a substring. Let’s consider the following example:
There’s no reason for a `Main method to throw anything. After all, what’s going to catch it?
Instead, the method should itself gracefully handle any exceptions that may bubble up to it, attach as much contextual information as possible, and perform whatever logging or user communication is necessary, and Exit` with a non-zero (i.e. non-success) exit code if necessary.
When a method in a derived class has:
the same name as a method in the base class
but types of parameters that are ancestors (for example string in the base class and object in the derived class)
the result is that the base method becomes hidden.
As shown in the following code snippet, when an instance of the derived class is used, invoking the method with an argument that matches the less derived parameter type will invoke the derived class method instead of the base class method:
double.NaN and float.NaN are not equal to anything, not even themselves.
When anything is compared with NaN using one of the comparison operators >, >=, <, ⇐ or the equality operator ==, the result will always be false. In contrast, when anything is compared with NaN using the inequality operator !=, the result will always be true.
Instead, the best way to see whether a variable is equal to NaN is to use the float.IsNaN and double.IsNaN methods, which work as expected.
When an object has a field annotated with ThreadStatic
, that field is shared within a given thread, but unique across threads. Since a class’ static initializer is only invoked for the first thread created, it also means that only the first thread will have the expected initial values.
Instead, allow such fields to be initialized to their default values or make the initialization lazy.
The rules for method resolution are complex and perhaps not properly understood by all coders. The `params keyword can make method declarations overlap in non-obvious ways, so that slight changes in the argument types of an invocation can resolve to different methods.
This rule raises an issue when an invocation resolves to a method declaration with params, but could also resolve to another non-params` method too.
There’s no point in creating an array solely for the purpose of passing it to a params parameter. Simply pass the elements directly. They will be consolidated into an array automatically.
When only a single public parameterless constructor is defined in a class, then that constructor can be removed because the compiler would generate it automatically. Similarly, empty static
constructors and empty destructors are also wasted keystrokes.
`System.Collections.Generic.List<T> is a generic collection that is designed for performance and not inheritance. For example, it does not contain virtual members that make it easier to change the behavior of an inherited class. That means that future attempts to expand the behavior will be spoiled because the extension points simply aren’t there. Instead, one of the following generic collections should be used:
System.Collections.Generic.IEnumerable<T>
System.Collections.Generic.IReadOnlyCollection<T>
System.Collections.Generic.ICollection<TKey>
System.Collections.Generic.IReadOnlyList<T>
System.Collections.Generic.IList<TKey>
System.Collections.ObjectModel.Collection<T>
System.Collections.ObjectModel.ReadOnlyCollection<T>
System.Collections.ObjectModel.KeyedCollection<TKey, Titem>
This rule raises an issue every time a System.Collections.Generic.List<T>` is exposed:
As an externally visible member.
As the return type of an externally visible method.
As a parameter type of an an externally visible method.
Short-circuit evaluation is an evaluation strategy for Boolean operators, that doesn’t evaluates the second argument of the operator if it is not needed to determine the result of the operation.
C# provides logical operators that implement short-circuit evaluation: && and ||, as well as non-short-circuit versions: & and |. Unlike short-circuit operators, non-short-circuit ones evaluate both operands and afterwards perform the logical operation.
For example false && FunctionCall() always results in false, even when FunctionCall invocation would raise an exception. Instead, false & FunctionCall() also evaluates FunctionCall(), and results in an exception if FunctionCall() invocation raises an exception.
Similarly, true || 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.
When concatenating strings, it is very easy to forget a whitespace.
In some scenarios this might cause runtime errors, one of which is while creating an SQL query via concatenation:
In order to produce a formatted string, both string.Create and either FormattableString.Invariant or FormattableString.CurrentCulture can be used. However, string.Create rents array buffers from ArrayPool<char> making it more performant, as well as preventing unnecessary allocations and future stress on the Garbage Collector.
This applies to .NET versions after .NET 6, when these string.Create overloads were introduced.
Suppose you override Object.Equals in a type, you must also override Object.GetHashCode. If two objects are equal according to the Equals method, then calling GetHashCode on each of them must yield the same integer. If this is not the case, many collections, such as a Hashtable or a Dictionary won’t handle class instances correctly.
In order to not have unpredictable behavior, Equals and GetHashCode should be either both inherited, or both overridden.
In the interests of readability, code that can be simplified should be simplified. To that end, there are several ways IEnumerable language integrated queries (LINQ) can be simplified. This not only improves readabilty but can also lead to improved performance.
A method using the `VarArgs calling convention is not Common Language Specification (CLS) compliant and might not be accessible across programming languages, while the params keyword works the same way and is CLS compliant.
This rule raises an issue when a public or protected type contains a public or protected method that uses the VarArgs` calling convention.
The NET Framework 2.0 introduced the generic interface `System.Collections.Generic.IEnumerable<T> and it should be preferred over the older, non generic, interfaces.
This rule raises an issue when a public type implements System.Collections.IEnumerable`.
When division is performed on ints, the result will always be an int. You can assign that result to a double, float or decimal with automatic type conversion, but having started as an int, the result will likely not be what you expect. If the result of int
division is assigned to a floating-point variable, precision will have been lost before the assignment. Instead, at least one operand should be cast or promoted to the final type before the operation takes place.
The rules for method resolution can be complex and may not be fully understood by all developers. The situation becomes even more challenging when dealing with method overloads that have optional parameter values.
This rule raises an issue when an overload with default parameter values is hidden by another overload that does not have the optional parameters.
Locking on a class field synchronizes not on the field itself, but on the object assigned to it. Thus, there are some good practices to follow to avoid problems related to thread synchronization.
Locking on a non-readonly field makes it possible for the field’s value to change while a thread is in the code block, locked on the old value. This allows another thread to lock on the new value and access the same block concurrently.
Invoking a method designed to return a string representation of an object which is already a string is a waste of keystrokes. Similarly, explicitly invoking `ToString() when the compiler would do it implicitly is also needless code-bloat.
This rule raises an issue when ToString() is invoked:
on a string
on a non-string operand to concatenation
on an argument to string.Format`
When a loop is filtering, selecting or aggregating, those functions can be handled with a clearer, more concise LINQ expression instead.
Catching NullReferenceException is generally considered a bad practice because it can hide bugs in your code. Instead of catching this exception, you should aim to prevent it. This makes your code more robust and easier to understand. In addition, constantly catching and handling NullReferenceException can lead to performance issues. Exceptions are expensive in terms of system resources, so they should be used cautiously and only for exceptional conditions, not for regular control flow.
The difference between private and protected visibility is that child classes can see and use protected members, but they cannot see private ones. Since a sealed class cannot have children, marking its members protected
is confusingly pointless.
public static mutable fields of classes which are accessed directly should be protected to the degree possible. This can be done by reducing the accessibility of the field or by changing the return type to an immutable type.
This rule raises issues for public static fields with a type inheriting/implementing System.Array or System.Collections.Generic.ICollection<T>.
In general, async void
test methods are not executed by test frameworks, therefore it’s better to avoid them altogether.
In ASP.NET Core MVC, the routing middleware utilizes a series of rules and conventions to identify the appropriate controller and action method to handle a specific HTTP request. This process, known as conventional routing, is generally established using the MapControllerRoute method. This method is typically configured in one central location for all controllers during the application setup.
Conversely, attribute routing allows routes to be defined at the controller or action method level. It is possible to mix both mechanisms. Although it’s permissible to employ diverse routing strategies across multiple controllers, combining both mechanisms within one controller can result in confusion and increased complexity, as illustrated below.
Using string.Equals to determine if a string is empty is significantly slower than using string.IsNullOrEmpty() or checking for
++string.Length
In Blazor, the [JSInvokable] attribute is used to annotate a method, enabling it to be invoked from JavaScript code. The prerequisite for this functionality is that the method must be declared as public. Otherwise, a runtime error will be triggered when an attempt is made to call the method from JavaScript.
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:
Needing to cast from an interface to a concrete type indicates that something is wrong with the abstractions in use, likely that something is missing from the interface. Instead of casting to a discrete type, the missing functionality should be added to the interface
. Otherwise there is a risk of runtime exceptions.
The purpose of an abstract class is to provide some heritable behaviors while also defining methods which must be implemented by sub-classes.
A `class with no abstract methods that was made abstract purely to prevent instantiation should be converted to a concrete class (i.e. remove the abstract keyword) with a protected constructor.
A class with only abstract methods and no inheritable behavior should be converted to an interface`.
The SupplyParameterFromQuery attribute can be used to specify that a component parameter, of a routable component, comes from the query string.
Component parameters supplied from the query string support the following types:
bool, DateTime, decimal, double, float, Guid, int, long, string.
Nullable variants of the preceding types.
Arrays of the preceding types, whether they’re nullable or not nullable.
Query parameters should have one of the supported types. Otherwise, an unhandled exception will be raised at runtime.
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 (static void Main` method) of an assembly using Windows Forms is not marked as STA.
In the interests of keeping code clean, the simplest possible conditional syntax should be used. That means
using the `??= operator for a self-assign-if-not-null operation,
using the ?? operator for an assign-if-not-null operation, and
using the ternary operator ?:` for assignment to a single variable.
Because composite format strings are interpreted at runtime, rather than validated by the compiler, they can contain errors that lead to unexpected behaviors or runtime errors. This rule statically validates the good behavior of composite formats when calling the methods of
`String.Format
StringBuilder.AppendFormat
Console.Write
Console.WriteLine
TextWriter.Write
TextWriter.WriteLine
Debug.WriteLine(String, Object[])
Trace.TraceError(String, Object[])
Trace.TraceInformation(String, Object[])
Trace.TraceWarning(String, Object[])
TraceSource.TraceInformation(String, Object[])`.
Classes that declare an implementation of `Serializable should provide a serializable constructor. Without such a constructor, you’ll be unable to deserialize the class.
Serialization constructors should be private for sealed types and protected` otherwise.
Many string operations, the `Compare and Equals methods in particular, provide an overload that accepts a StringComparison enumeration value as a parameter. Calling these overloads and explicitly providing this parameter makes your code clearer and easier to maintain.
This rule raises an issue when a string comparison operation doesn’t use the overload that takes a StringComparison` parameter.
Because `IEnumerables are lazy-evaluated, each iteration causes a re-retrieval of the values, which could involve considerable overhead. For instance, when the IEnumerable is backed by a database, each iteration requires an additional round of database interactions. For that reason, any time the set represented by an IEnumerable must be iterated multiple times, it should first be converted to a List, which will retrieve the values and store them in memory. From that point they can be iterated as often as needed without an additional performance hit.
This rule raises an issue for each iteration of an IEnumerable` after the first one.
Since the compiler will automatically invoke the base type’s no-argument constructor, there’s no need to specify its invocation explicitly. Also, when only a single public parameterless constructor is defined in a class, then that constructor can be removed because the compiler would generate it automatically. Similarly, empty static
constructors and empty destructors are also wasted keystrokes.
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 CS0103 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.
Usually IntPtr and UIntPtr fields are used to store pointers to unmanaged resources and the finalizer will free the unmanaged resource pointed to by the pointer fields. If the garbage collector finalises the object while the managed resources are still in use it could lead to serious, hard to diagnose bug. To prevent this from happening the object should be kept alive by calling GC.KeepAlive(this)
in methods calling unmanaged code on this pointers.
The ISerializable interface gives you control over how your class is serialized, but does not itself make the class serializable. Such classes must also have the [Serializable]
attribute.
Using the equality == and inequality != operators to compare two objects generally works. The operators can be overloaded, and therefore the comparison can resolve to the appropriate method. However, when the operators are used on interface instances, then == resolves to reference equality, which may result in unexpected behavior if implementing classes override Equals. Similarly, when a class overrides Equals, but instances are compared with non-overloaded ==
, there is a high chance that value comparison was meant instead of the reference one.
A static field in a generic type is not shared among instances of different closed constructed types, thus LengthLimitedSingletonCollection<int>.instances and LengthLimitedSingletonCollection<string>.instances will point to different objects, even though instances is seemingly shared among all LengthLimitedSingletonCollection<>
generic classes.
If you need to have a static field shared among instances with different generic arguments, define a non-generic base class to store your static members, then set your generic type to inherit from the base class.
Calling GetType() on a nullable value type object returns the underlying value type. Therefore, comparing the returned Type object to typeof(Nullable<SomeType>) will either throw an NullReferenceException or the result will always be true or false and can be known at compile time.
Enumerable.Sum() always executes addition in a checked context, so an OverflowException will be thrown if the value exceeds MaxValue, even if an unchecked context was specified. Therefore, using this method inside an unchecked context will only make the code more confusing, since the behavior will still be checked.
This rule raises an issue when an unchecked context is specified for a Sum on integer types.
Calling GetType on a Type variable will always return the System.Type representation, which is equivalent to typeof(System.Type). This also applies to passing a Type argument to IsInstanceOfType which always returns false.
In both cases, the results are entirely predictable and should be avoided.
When exceptions occur, it is usually a bad idea to simply ignore them. Instead, it is better to handle them properly, or at least to log them.
This rule only reports on empty catch clauses that catch generic Exception
s.
Strings and integral types are typically used as indexers. When some other type is required, it typically indicates design problems, and potentially a situation where a method should be used instead.
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 a string in the throw` statement contains the name of one of the method parameters.
Catching System.Exception seems like an efficient way to handle multiple possible exceptions. Unfortunately, it traps all exception types, including the ones that were not intended to be caught. To prevent any misunderstandings, the exception filters should be used. Alternatively each exception type should be in a separate catch block.
ASP.Net has a feature to validate HTTP requests to prevent potentially dangerous content to perform a cross-site scripting (XSS) attack. There is no reason to disable this mechanism even if other checks to prevent XXS attacks are in place.
This rule raises an issue if a method with parameters is marked with System.Web.Mvc.HttpPostAttribute and not System.Web.Mvc.ValidateInputAttribute(true)
.
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.
The Monitor.Pulse
call releases the object on which it was called and wakes up the first thread waiting for the lock on that object. Significantly, it only releases one lock, and if multiple locks are held when it is called deadlocks could result.
Creating a new Exception without actually throwing does not achieve the intended purpose.
In the interests of making code as usable as possible, interfaces and delegates with generic parameters should use the `out and in modifiers when possible to make the interfaces and delegates covariant and contravariant, respectively.
The out keyword can be used when the type parameter is used only as a return type in the interface or delegate. Doing so makes the parameter covariant, and allows interface and delegate instances created with a sub-type to be used as instances created with a base type. The most notable example of this is IEnumerable<out T>, which allows the assignment of an IEnumerable<string> instance to an IEnumerable<object> variable, for instance.
The in keyword can be used when the type parameter is used only as a method parameter in the interface or a parameter in the delegate. Doing so makes the parameter contravariant, and allows interface and delegate instances created with a base type to be used as instances created with a sub-type. I.e. this is the inversion of covariance. The most notable example of this is the Action<in T> delegate, which allows the assignment of an Action<object> instance to a Action<string>` variable, for instance.
If you’re using a struct, 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.
Adding params to a method override has no effect. The compiler accepts it, but the callers won’t be able to benefit from the added modifier.
An assertion is a piece of code that’s used during development when the compilation debug mode is activated. It allows a program to check itself as it runs. When an assertion is true, that means everything is operating as expected.
In non-debug mode, all Debug.Assert calls are automatically left out (via the Conditional(“DEBUG”) mechanism). So, by contract, the boolean expressions that are evaluated by those assertions must not contain any side effects. Otherwise, when leaving the debug mode, the functional behavior of the application is not the same anymore.
The rule will raise if the method name starts with any of the following remove, delete, add, pop, update, retain, insert, push, append, clear, dequeue, enqueue, dispose, put, or set, although SetEquals will be ignored.
Unfortunately, it is possible to make constructor calls recursive. When this happens, you get a class that cannot be instantiated.
As a general rule, no constructor should make a call to another constructor in the same class that requires fewer arguments than the calling constructor received. I.e. the constructor that accepts the most arguments is the one that has the fullest picture of how the class should look. It should perform class initialization.
Unnecessary keywords simply clutter the code and should be removed. Specifically:
`partial on type declarations that are completely defined in one place
sealed on members of sealed classes
unsafe method or block inside construct already marked with unsafe, or when there are no unsafe constructs in the block
checked and unchecked` blocks with no integral-type arithmetic operations
Certain bitwise operations are not needed and should not be performed because their results are predictable.
Specifically, using & -1 with any value always results in the original value.
That is because the binary representation of -1 on a integral numeric type supporting negative numbers, such as int or long, is based on two’s complement and made of all 1s: 0b111…111.
Performing & between a value and 0b111…111 means applying the & operator to each bit of the value and the bit 1, resulting in a value equal to the provided one, bit by bit.
Certain mathematical comparisons will always return the same value, and should not be performed.
Specifically, the following comparisons will return either always true or always false depending on the kind of comparison:
comparing a char with a numeric constant that is outside of the range of char
comparing a float with a numeric constant that is outside of the range of float
comparing a long with a numeric constant that is outside of the range of long
comparing a ulong with a numeric constant that is outside of the range of ulong
etc.
Calling ToString() on an object should always return a string. Thus, overriding the ToString method should never return null, as it breaks the method’s implicit contract, and as a result the consumer’s expectations.
The switch statement is a conditional statement that executes a sequence of instructions based on patterns matching the provided value.
Transparency attributes can be declared at several levels. If two different attributes are declared at two different levels, the attribute that prevails is the one in the highest level.
For example, you can declare that a class is SecuritySafeCritical and that a method of this class is SecurityCritical. In this case, the method will be SecuritySafeCritical and the SecurityCritical
attribute attached to it is ignored.
When a private static
method is only invoked by a nested class, there’s no reason not to move it into that class. It will still have the same access to the outer class’ static members, but the outer class will be clearer and less cluttered.
Properties are accessed like fields which makes them easier to use.
This rule raises an issue when the name of a public or protected method starts with Get
, takes no parameter, and returns a value that is not an array.
Synchronization can be expensive in terms of time when multiple threads need to pass through the same bottleneck (method with `[MethodImpl(MethodImplOptions.Synchronized)]).
If you have a piece of code calling a method with [MethodImpl(MethodImplOptions.Synchronized)] attribute once, then it only has to wait its turn to pass through the bottleneck once. But call it in a loop, and your code has to get back in line for the bottleneck over and over.
Instead, it would be better to get into the bottleneck, and then do the looping. I.e. consider refactoring the code to perform the loop inside the method.
This rule raises an issue when a method with [MethodImpl(MethodImplOptions.Synchronized)]` is called in a loop.
When you annotate a field with the ThreadStatic attribute, it is an indication that the value of this field is unique for each thread. But if you don’t mark the field as static, then the ThreadStatic attribute is ignored.
The ThreadStatic attribute should either be removed or replaced with the use of ThreadLocal<T> class, which gives a similar behavior for non-static fields.
Finalizers come with a performance cost due to the overhead of tracking the life cycle of objects. An empty one is consequently costly with no benefit or justification.
Properties and Get method should have names that makes them clearly distinguishable.
This rule raises an issue when the name of a public or protected member starts with ‘Get’ and otherwise matches the name of a public or protected property.
In C#, delegates can be added together to chain their execution, and subtracted to remove their execution from the chain.
Subtracting a chain of delegates from another one might yield unexpected results as shown hereunder - and is likely to be a bug.
Once you modify a closure, any use of it could provide unexpected results.
When a `System.Globalization.CultureInfo or IFormatProvider object is not supplied, the default value that is supplied by the overloaded member might not have the effect that you want in all locales.
You should supply culture-specific information according to the following guidelines:
If the value will be displayed to the user, use the current culture. See CultureInfo.CurrentCulture.
If the value will be stored and accessed by software (persisted to a file or database), use the invariant culture. See CultureInfo.InvariantCulture.
If you do not know the destination of the value, have the data consumer or provider specify the culture.
This rule raises an issue when a method or constructor calls one or more members that have overloads that accept a System.IFormatProvider parameter, and the method or constructor does not call the overload that takes the IFormatProvider parameter. This rule ignores calls to .NET Framework methods that are documented as ignoring the IFormatProvider parameter as well as the following methods:
Activator.CreateInstance
ResourceManager.GetObject
ResourceManager.GetString`
In C#, the Object.ReferenceEquals method is used to compare two reference type variables. If you use this method to compare two value types, such as int, float, or bool you will not get the expected results because value type variables contain an instance of the type and not a reference to it.
Due to value type variables containing directly an instance of the type, they can’t have the same reference, and using Object.ReferenceEquals to compare them will always return false even if the compared variables have the same value.
After an `awaited Task has executed, you can continue execution in the original, calling thread or any arbitrary thread. Unless the rest of the code needs the context from which the Task was spawned, Task.ConfigureAwait(false) should be used to keep execution in the Task thread to avoid the need for context switching and the possibility of deadlocks.
This rule raises an issue when code in a class library targeting .Net Framework awaits a Task and continues execution in the original calling thread.
The rule does not raise for .Net Core libraries as there is no SynchronizationContext` in .Net Core.
When you create a `DataTable or DataSet, you should set the locale explicitly. By default, the locale for these types is the current culture. For data that is stored in a database or file and is shared globally, the locale should ordinarily be set to the invariant culture (CultureInfo.InvariantCulture).
This rule raises an issue when System.Data.DataTable or System.Data.DataSet instances are created without explicitly setting the locale property (DataTable.Locale or DataSet.Locale`).
Because the is operator performs a cast if the object is not null, using is to check type and then casting the same argument to that type, necessarily performs two casts. The same result can be achieved more efficiently with a single cast using as
, followed by a null-check.
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 dynamic, the compiler’s type checking is turned off, so you can pass anything to the right of a shift operator and have it compile. And if the argument can’t be implicitly converted to int at runtime, then a RuntimeBinderException will be raised.
Using the same value on both sides of certain operators is a code defect. In the case of logical operators, it is either a copy/paste error and, therefore, a bug, or it is simply duplicated code and should be simplified. For bitwise operators and most binary mathematical operators, having the same value on both sides of an operator yields predictable results and should be simplified as well to avoid further code defects.
This rule raises for the following operators.
Equality operators (== and !=)
Comparison operators (< =, <code, >=)
The following Logical Operators:
Logical OR (| )
Conditional logical OR (||)
Logical AND (&)
Conditional logical AND (&&)
Logical exclusive OR (^)
The following arithmetic operators:
Subtraction (-)
Division ()
Remainder operator (%)
Subtraction assignment operator (-=)
Divide assignment operator (=)
Floating point numbers in C# (and in most other programming languages) are not precise. They are a binary approximation of the actual value. This means that even if two floating point numbers appear to be equal, they might not be due to the tiny differences in their binary representation.
Even simple floating point assignments are not simple:
An async method with a void return type does not follow the task asynchronous programming (TAP) model since the return type should be Task or Task<TResult>
Doing so prevents control over the asynchronous execution, such as:
waiting for the execution to complete
catching any exception that might occur during execution
testing execution behavior
The `IEquatable<T> interface has only one method in it: Equals(<T>). If you’ve already written Equals(T), there’s no reason not to explicitly implement IEquatable<T>. Doing so expands the utility of your class by allowing it to be used where an IEquatable is called for.
Note: Classes that implement IEquatable<T> should also be sealed`.
A chain of if/else if 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.
Having an infinite loop or recursion will lead to a program failure or a program never finishing the execution.
String literals embedded in the source code will not be localized properly.
This rule raises an issue when a literal string is passed as a parameter or property and one or more of the following cases is true:
The `LocalizableAttribute attribute of the parameter or property is set to true.
The parameter or property name contains “Text”, “Message”, or “Caption”.
The name of the string parameter that is passed to a Console.Write or Console.WriteLine` method is either “value” or “format”.
A jagged array is an array whose elements are arrays. It is recommended over a multidimensional array because the arrays that make up the elements can be of different sizes, which avoids wasting memory space.
There’s no point in chaining multiple `OrderBy calls in a LINQ; only the last one will be reflected in the result because each subsequent call completely reorders the list. Thus, calling OrderBy multiple times is a performance issue as well, because all of the sorting will be executed, but only the result of the last sort will be kept.
Instead, use ThenBy` for each call after the first.
The use of ref or out in combination with Optional attribute is both confusing and contradictory. [Optional] indicates that the parameter doesn’t have to be provided, while out and ref mean that the parameter will be used to return data to the caller (ref additionally indicates that the parameter may also be used to pass data into the method).
Thus, making it [Optional] to provide the parameter in which you will be passing back the method results doesn’t make sense. In fact, the compiler will raise an error on such code. Unfortunately, it raises the error on method calls where the [Optional] parameter has been omitted, not the source of the problem, the method declaration.
Shared naming conventions allow teams to collaborate efficiently.
This rule raises an issue when a type name is not PascalCased.
For example, the classes
DefaultValue does not make the compiler set the default value, as its name may suggest. What you probably wanted to use is DefaultParameterValue.
The DefaultValue attribute from the System.ComponentModel namespace, is sometimes used to declare a member’s default value. This can be used, for instance, by the reset feature of a visual designer or by a code generator.
Field-like events are events that do not have explicit add and remove accessors.
The compiler automatically initializes class fields, auto-properties and events to their default values before setting them with any initialization values, so there is no need to explicitly set a member to its default value. Further, under the logic that cleaner code is better code, it’s considered poor style to do so.
An inheritance list entry is redundant if:
It is `Object - all classes extend Object implicitly.
It is int for an enum`
It is a base class of another listed inheritance.
Such redundant declarations should be removed because they needlessly clutter the code and can be confusing.
The parameter to `Assembly.Load includes the full specification of the dll to be loaded. Use another method, and you might end up with a dll other than the one you expected.
This rule raises an issue when Assembly.LoadFrom, Assembly.LoadFile, or Assembly.LoadWithPartialName` is called.
This rule addresses the issue of incomplete assertions that can occur when using certain test frameworks. Incomplete assertions can lead to tests that do not effectively verify anything. The rule enforces the use of complete assertions in specific cases, namely:
Fluent Assertions: Should() is not followed by an assertion invocation.
Debug statements are always useful during development. But include them in production code - particularly in code that runs client-side - and you run the risk of inadvertently exposing sensitive information.
In Blazor, when a route parameter constraint is applied, the value is automatically cast to the corresponding component parameter type. If the constraint type does not match the component parameter type, it can lead to confusion and potential runtime errors due to unsuccessful casting. Therefore, it is crucial to ensure that the types of route parameters and component parameters match to prevent such issues and maintain code clarity.
`switch statements and expressions are useful when there are many different cases depending on the value of the same expression.
When a switch statement or expression is simple enough, the code will be more readable with a single if, if-else` or ternary conditional operator.
There’s no point in having a public member in a non-public
type because objects that can’t access the type will never have the chance to access the member.
This rule raises an issue when a type has methods, fields, or inner types with higher visibility than the type itself has.
The abstract modifier in a class declaration is used to indicate that a class is intended only to be a base class of other classes, not instantiated on its own.
Since abstract classes cannot be instantiated, there is no need for public or internal constructors. If there is basic initialization logic that should run when an extending class instance is created, you can add it in a private, private protected or protected constructor.
When working with anonymous functions, it is important to keep in mind that each time you create one, it is a completely new instance.
In this example, even though the same lambda expression is used, the expressions are stored separately in the memory and are therefore not equal or the same.
The value of a `static readonly field is computed at runtime while the value of a const field is calculated at compile time, which improves performance.
This rule raises an issue when a static readonly` field is initialized with a value that is computable at compile time.
As specified by Microsoft, the list of types that can have a constant value are:
C# type | .Net Fwk type |
---|---|
bool | System.Boolean |
byte | System.Byte |
sbyte | System.SByte |
char | System.Char |
decimal | System.Decimal |
double | System.Double |
float | System.Single |
int | System.Int32 |
uint | System.UInt32 |
long | System.Int64 |
ulong | System.UInt64 |
short | System.Int16 |
ushort | System.UInt16 |
string | System.String |
Non-encoded control characters and whitespace characters are often injected in the source code because of a bad manipulation. They are either invisible or difficult to recognize, which can result in bugs when the string is not what the developer expects. If you actually need to use a control character use their encoded version:
This rule raises an issue when the following characters are seen in a string literal:
ASCII control character. (character index < 32 or = 127)
Unicode whitespace characters.
Unicode C0 control characters
Unicode characters
U+200B, U+200C, U+200D, U+2060, U+FEFF, U+2028, U+2029
A method is identified as a test method if it is marked with one of the following attributes:
[TestMethod] or [DataTestMethod] (for MSTest).
[Fact] or [Theory] (for xUnit).
[Test], [TestCase], [TestCaseSource], or [Theory] (for NUnit).
However, non-public methods are not considered test methods and will not be executed, regardless of whether they have a test attribute. Additionally, methods with the async void modifier or methods that contain generics <T> anywhere in their signatures are also excluded from being recognized as tests and will not be executed.
In C#, the throw statement can be used in two different ways:
by specifying an expression
without specifying an expression
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
.
With the advent of .NET framework version 2, certain practices have become obsolete.
In particular, exceptions should now extend `System.Exception instead of System.ApplicationException. Similarly, generic collections should be used instead of the older, non-generic, ones. Finally when creating an XML view, you should not extend System.Xml.XmlDocument.
This rule raises an issue when an externally visible type extends one of these types:
System.ApplicationException
System.Xml.XmlDocument
System.Collections.CollectionBase
System.Collections.DictionaryBase
System.Collections.Queue
System.Collections.ReadOnlyCollectionBase
System.Collections.SortedList
System.Collections.Stack`
Re-assigning a variable to itself is a defect as it has no actual effect and indicates meaning to do something else. It usually means that:
The statement is redundant and should be removed
The re-assignment is a mistake, and another value or variable was intended for the assignment instead
GC.Collect is a method that forces or suggests to the garbage collector to run a collection of objects in the managed heap that are no longer being used and free their memory.
Calling GC.Collect is rarely necessary and can significantly affect application performance. That’s because it is a tracing garbage collector and needs to examine every object in memory for cleanup and analyze all reachable objects from every application’s root (static fields, local variables on thread stacks, etc.).
To perform tracing and memory releasing correctly, the garbage collection may need to block all threads currently in execution. That is why, as a general rule, the performance implications of calling GC.Collect far outweigh the benefits.
This rule raises an issue when any overload of Collect is invoked.
When you implement `IComparable or IComparable<T> on a class you should also override Equals(object) and overload the comparison operators (==, !=, <, <=, >, >=). That’s because the CLR cannot automatically call your CompareTo implementation from Equals(object) or from the base comparison operator implementations. Additionally, it is best practice to override GetHashCode along with Equals.
This rule raises an issue when a class implements IComparable without also overriding Equals(object)` and the comparison operators.
Default arguments are determined by the static type of the object.
The `IDisposable interface is a mechanism to release unmanaged resources, if not implemented correctly this could result in resource leaks or more severe bugs.
This rule raises an issue when the recommended dispose pattern, as defined by Microsoft, is not adhered to. See the Compliant Solution section for examples.
Satisfying the rule’s conditions will enable potential derived classes to correctly dispose the members of your class:
sealed classes are not checked.
If a base class implements IDisposable your class should not have IDisposable in the list of its interfaces. In such cases it is recommended to override the base class’s protected virtual void Dispose(bool) method or its equivalent.
The class should not implement IDisposable explicitly, e.g. the Dispose() method should be public.
The class should contain protected virtual void Dispose(bool) method. This method allows the derived classes to correctly dispose the resources of this class.
The content of the Dispose() method should be invocation of Dispose(true) followed by GC.SuppressFinalize(this)
If the class has a finalizer, i.e. a destructor, the only code in its body should be a single invocation of Dispose(false).
If the class inherits from a class that implements IDisposable it must call the Dispose, or Dispose(bool) method of the base class from within its own implementation of Dispose or Dispose(bool)`, respectively. This ensures that all resources from the base class are properly released.
One of the possible ways of performing type-testing is via the is operator: food is Pizza.
The is operator is often used before a direct cast to the target type, as a more flexible and powerful alternative to the as operator, especially when used to perform pattern matching.
By convention, namespaces within a project should start with the project default namespace, and end with the file’s position within the project. Anything else may confuse maintainers and callers.
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)
Because serialization constructors allocate and initialize objects, security checks that are present on regular constructors must also be present on a serialization constructor. Failure to do so would allow callers that could not otherwise create an instance to use the serialization constructor to do this.
This rule raises an issue when a type implements the System.Runtime.Serialization.ISerializable interface, is not a delegate or interface, is declared in an assembly that allows partially trusted callers and has a constructor that takes a System.Runtime.Serialization.SerializationInfo object and a System.Runtime.Serialization.StreamingContext
object which is not secured by a security check, but one or more of the regular constructors in the type is secured.
Using the base
keyword to access a member in anonymous methods, iterator results, and lambda and query expressions results in the compiler creating extra classes under the covers. Those extra classes are “unverifiable”, meaning that the under some trust levels, the code will not be allowed to run.
Instead, the access should be made from a helper method.
GetHashCode is used to file an object in a Dictionary or Hashtable. If GetHashCode uses non-readonly fields and those fields change after the object is stored, the object immediately becomes mis-filed in the Hashtable. Any subsequent test to see if the object is in the Hashtable will return a false negative.
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
.
When a static constructor serves no other purpose that initializing static fields, it comes with an unnecessary performance cost because the compiler generates a check before each static
method or instance constructor invocation.
Instead, inline initialization is highly recommended.
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:
In C#, the type of a variable can often be inferred by the compiler. The use of the [var keyword](https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/implicitly-typed-local-variables) allows you to avoid repeating the type name in a variable declaration and object instantiation because the declared type can often be inferred by the compiler.
Additionally, initializations providing the default value can also be omitted, helping to make the code more concise and readable.
Unnecessarily verbose declarations and initializations should be simplified. Specifically, the following should be omitted when they can be inferred:
array element type
array size
`new DelegateType
new Nullable<Type>`
object or collection initializers ({})
type of lambda expression parameters
parameter declarations of anonymous methods when the parameters are not used.
Shared naming conventions allow teams to collaborate efficiently.
This rule raises an issue when a method or a property name is not PascalCased.
For example, the method
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.
Nested control flow statements if, switch, for, foreach, 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.
Creating objects that are not used is a vulnerability that can lead to unexpected behavior.
If this was done intentionally due to side effects in the object’s constructor, the code should be moved to a dedicated method.
If an enum member’s name contains the word “reserved” it implies it is not currently used and will be change in the future. However changing an enum member is a breaking change and can create significant problems. There is no need to reserve an enum
member since a new member can be added in the future, and such an addition will usually not be a breaking change.
This rule raises an issue when the name of an enumeration member contains “reserved”.
When writing managed code, there is no need to worry about memory allocation or deallocation as it is taken care of by the garbage collector. However, certain objects, such as Bitmap, utilize unmanaged memory for specific purposes like pointer arithmetic. These objects may have substantial unmanaged memory footprints while having minimal managed footprints. Unfortunately, the garbage collector only recognizes the small managed footprint and does not promptly reclaim the corresponding unmanaged memory (by invoking the finalizer method of Bitmap) for efficiency reasons.
In addition, it’s essential to manage other system resources besides memory. The operating system has limits on the number of file descriptors (e.g., FileStream) or sockets (e.g., WebClient) that can remain open simultaneously. Therefore, it’s crucial to Dispose of these resources promptly when they are no longer required, instead of relying on the garbage collector to invoke the finalizers of these objects at an unpredictable time in the future.
This rule keeps track of private fields and local variables of specific types that implement IDisposable or IAsyncDisposable. It identifies instances of these types that are not properly disposed, closed, aliased, returned, or passed to other methods. This applies to instances that are either directly created using the new operator or instantiated through a predefined list of factory methods.
Here is the list of predefined factory methods tracked by this rule:
System.IO.File.Create()
System.IO.File.Open()
System.Drawing.Image.FromFile()
System.Drawing.Image.FromStream()
Recursion is a technique used to define a problem in terms of the problem itself, usually in terms of a simpler version of the problem itself.
For example, the implementation of the generator for the n-th value of the Fibonacci sequence comes naturally from its mathematical definition, when recursion is used:
A field marked `readonly can only be assigned as part of its declaration or in a constructor. While readonly reference types (e.g. classes) can still have their state changed subsequently, the same is not true of value types such as struct s. Thus, calling a method that updates object state on a readonly value type field simply has no effect (but runs without error!). The result is code that probably doesn’t do what you thought it did.
This rule raises an issue when a method that is not marked [Pure] is invoked on a readonly` value type field.
The foreach statement was introduced in the C# language prior to generics to make it easier to work with the non-generic collections available at that time such as ArrayList. The foreach statements allow you to downcast elements of a collection of Objects to any other type.
The problem is that to achieve the cast, the foreach statements silently perform explicit type conversion, which at runtime can result in an InvalidCastException.
C# code iterating on generic collections or arrays should not rely on foreach statement’s silent explicit conversions.
Decreasing the accessibility level of an inherited method that is not overridable to private will shadow the name of the base method and can lead to confusion.
Calling an overridable method from a constructor could result in failures or strange behaviors when instantiating a subclass which overrides the method.
When constructing an object of a derived class, the constructor of the parent class is invoked first, and only then the constructor of the derived class is called. This sequential construction process applies to multiple levels of inheritance as well, starting from the base class and progressing to the most derived class.
If an overridable method is called within the constructor of the parent class, it may inadvertently invoke an overridden implementation in the derived class. This can lead to unexpected failures or strange behaviors because the object’s construction is still in progress and may not have reached a fully initialized state. Consequently, the overridden method may rely on uninitialized members or have assumptions about the object’s state that are not yet valid.
For example:
Nullable value types can hold either a value or null.
The value held 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 null. A nullable type should always be tested before accessing the value to avoid raising exceptions.
Trivial properties, which include no logic but setting and getting a backing field should be converted to auto-implemented properties, yielding cleaner and more readable code.
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 be let to propagate up the stack trace so that they can be dealt with appropriately. When a general exception is thrown, it forces consumers to catch exceptions they do not intend to handle, which they then have to re-throw.
Besides, when working with a general type of exception, the only way to distinguish between multiple exceptions is to check their message, which is error-prone and difficult to maintain. Legitimate exceptions may be unintentionally silenced and errors may be hidden.
For instance, if an exception such as 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.
When implementing operator overloads, it is very important to make sure that all related operators and methods are consistent in their implementation.
The following guidelines should be followed:
When providing `operator == you should also provide operator != and vice-versa.
When providing operator == you should also provide Equals(Object) and GetHashCode().
When providing operator +, -, *, / or % you should also provide operator ==`, respecting previous guidelines.
This rule raises an issue when any of these guidelines are not followed on publicly-visible type (public, protected or protected internal).
In Blazor, using lambda expressions as event handlers when the UI elements are rendered in a loop can lead to negative user experiences and performance issues. This is particularly noticeable when rendering a large number of elements.
The reason behind this is that Blazor rebuilds all lambda expressions within the loop every time the UI elements are rendered.
When a base type explicitly implements a public interface method, that method is only accessible in derived types through a reference to the current instance (namely `this). If the derived type explicitly overrides that interface method, the base implementation becomes inaccessible.
This rule raises an issue when an unsealed, externally visible type provides an explicit method implementation of a public interface` and does not provide an alternate, externally visible method with the same name.
When a derived type is used as a parameter instead of the base type, it limits the uses of the method. If the additional functionality that is provided in the derived type is not required then that limitation isn’t required, and should be removed.
This rule raises an issue when a method declaration includes a parameter that is a derived type and accesses only members of the base type.
Properties provide a way to enforce encapsulation by providing accessors 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 a getter or setter.
Composite format strings in C# are evaluated at runtime, which means they are not verified by the compiler. Introducing an ill-formed format item, or indexing mismatch can lead to unexpected behaviors or runtime errors. The purpose of this rule is to perform static validation on composite format strings used in various string formatting functions to ensure their correct usage. This rule validates the proper behavior of composite formats when invoking the following methods:
String.Format
StringBuilder.AppendFormat
Console.Write
Console.WriteLine
TextWriter.Write
TextWriter.WriteLine
Debug.WriteLine(String, Object[])
Trace.TraceError(String, Object[])
Trace.TraceInformation(String, Object[])
Trace.TraceWarning(String, Object[])
TraceSource.TraceInformation(String, Object[])
Overriding a method just to call the same method from the base class without performing any other actions is useless and misleading. The only time this is justified is in sealed overriding methods, where the effect is to lock in the parent class behavior. This rule ignores overrides of Equals and GetHashCode
.
NOTE: In some cases it might be dangerous to add or remove empty overrides, as they might be breaking changes.
Calling Environment.Exit(exitCode) or Application.Exit()
terminates the process and returns an exit code to the operating system..
Each of these methods should be used with extreme care, and only when the intent is to stop the whole application.
The output of an as
cast will be null if the input to the cast cannot safely be cast to the desired type. So it makes sense that after such a cast you would null-check the output. But it doesn’t make sense to check the input.
A nested type is a type argument that is also a generic type. Calling a method with such a nested type argument requires complicated and confusing code. It should be avoided as much as possible.
SupplyParameterFromQuery attribute is used to specify that a component parameter of a routable component comes from the query string.
In the case of non-routable components, the SupplyParameterFromQuery does not contribute to the functionality, and removing it will not affect the behavior.
Most checks against an IndexOf value compare it with -1 because 0 is a valid index.
When switch 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.
The rule is reporting when an exception is thrown from certain methods and constructors. These methods are expected to behave in a specific way and throwing an exception from them can lead to unexpected behavior and break the calling code.
When you do not use the return value of a method with no side effects, it indicates that something is wrong. Either this method is unnecessary, or the source code does not behave as expected and could lead to code defects. For example, there are methods, such as DateTime.AddYears, that don’t change the value of the input object, but instead, they return a new object whose value is the result of this operation, and as a result that you will have unexpected effects if you do not use the return value.
This rule raises an issue when the results of the following methods are ignored:
Any method on build-in types
Any method on Immutable collections
Special cases:
Although string.Intern has a side effect, ignoring its return value is still suspicious as it is the only reference ensured to point to the intern pool.
LINQ methods can have side effects if they are misused. For example:
Such code should be rewritten as a loop because Enumerable.All<TSource> method should be used to determine if all elements satisfy a condition and not to change their state.
The MD5 algorithm and its successor, SHA-1, are no longer considered secure, because it is too easy to create hash collisions with them. That is, it takes too little computational effort to come up with a different input that produces the same MD5 or SHA-1 hash, and using the new, same-hash value gives an attacker the same access as if he had the originally-hashed value. This applies as well to the other Message-Digest algorithms: MD2, MD4, MD6.
This rule tracks usage of the System.Security.Cryptography.CryptoConfig.CreateFromName(), and System.Security.Cryptography.HashAlgorithm.Create() methods to instantiate MD5, DSA, HMACMD5, HMACRIPEMD160, RIPEMD-160 or SHA-1 algorithms, and of derived class instances of System.Security.Cryptography.SHA1 and System.Security.Cryptography.MD5
.
Consider using safer alternatives, such as SHA-256, or SHA-3.
Mutable collections are those whose state can be changed. For instance, `Array and List<T> are mutable, but System.Collections.ObjectModel.ReadOnlyCollection<T> and System.Collections.Immutable.ImmutableList<T> are not. Mutable collection class members should not be returned to a caller or accepted and stored directly. Doing so leaves you vulnerable to unexpected changes in your class state.
Instead use and store a copy of the mutable collection, or return an immutable collection wrapper, e.g. System.Collections.ObjectModel.ReadOnlyCollection<T>.
Note that you can’t just return your mutable collection through the IEnumerable<T>` interface because the caller of your method/property could cast it down to the mutable type and then change it.
This rule checks that mutable collections are not stored or returned directly.
While the properties of a readonly reference type field can still be changed after initialization, those of a readonly value type field, such as a struct, cannot.
If the member could be either a class or a struct then assignment to its properties could be unreliable, working sometimes but not others.
When using the postfix increment operator, it is important to know that the result of the expression x++ is the value before the operation x.
This means that in some cases, the result might not be what you expect:
When assigning x++ to x, it’s the same as assigning x to itself, since the value is assigned before the increment takes place
When returning x++, the returning value is x, not x+1
The same applies to the postfix and prefix decrement operators.
When a reference parameter (keyword `ref) is used, the passed argument type must exactly match the reference parameter type. This means that to be able to pass a derived type, it must be cast and assigned to a variable of the proper type. Use of generic methods eliminates that cumbersome down casting and should therefore be preferred.
This rule raises an issue when a method contains a ref parameter of type System.Object`.
Exceptions types should provide the following constructors:
public MyException()
public MyException(string)
public MyException(string, Exception)
The absence of these constructors can complicate exception handling and limit the information that can be provided when an exception is thrown.
Declaring multiple variable on one line is difficult to read.
Array covariance is the principle that if an implicit or explicit reference conversion exits from type `A to B, then the same conversion exists from the array type A[] to B[].
While this array conversion can be useful in readonly situations to pass instances of A[] where B[] is expected, it must be used with care, since assigning an instance of B into an array of A will cause an ArrayTypeMismatchException` to be thrown at runtime.
The ability to target the common language runtime from several languages means that clashes are possible when a word that is reserved in one language is used as the name of a namespace, type or member in another.
This rule raises an issue when a keyword from C++/CLI, C# or Visual Basic is used as an identifier.
The recommended way to access Azure Durable Entities is through generated proxy objects with the help of interfaces.
The following restrictions, during interface design, are enforced:
Entity interfaces must be defined in the same assembly as the entity class. This is not detected by the rule.
Entity interfaces must only define methods.
Entity interfaces must not contain generic parameters.
Entity interface methods must not have more than one parameter.
Entity interface methods must return void, Task, or Task<T>.
If any of these rules are violated, an InvalidOperationException is thrown at runtime when the interface is used as a type argument to IDurableEntityContext.SignalEntity<TEntityInterface>, IDurableEntityClient.SignalEntityAsync<TEntityInterface> or IDurableOrchestrationContext.CreateEntityProxy<TEntityInterface>. The exception message explains which rule was broken.
This rule raises an issue in case any of the restrictions above is not respected.
Empty interfaces are either useless or used as a marker. Custom attributes are a better alternative to marker interfaces. See the How to fix it section for more information.
When you call Any(), it clearly communicates the code’s intention, which is to check if the collection is empty. Using `Count()
Locking on a local variable can undermine synchronization because two different threads running the same method in parallel will potentially lock on different instances of the same object, allowing them to access the synchronized block at the same time.
It makes little sense to create an extension method when it is possible to just add that method to the class itself.
This rule raises an issue when an extension is declared in the same namespace as the class it is extending.
Enumerations are commonly used to identify distinct elements from a set of values.
However, they can also serve as bit flags, enabling bitwise operations to combine multiple elements within a single value.
Passing a collection as an argument to the collection’s own method is a code defect. Doing so might either have unexpected side effects or always have the same result.
IDisposable is an interface implemented by all types which need to provide a mechanism for releasing unmanaged resources.
Unlike managed memory, which is taken care of by the garbage collection,
The interface declares a Dispose method, which the implementer has to define.
The method name Dispose should be used exclusively to implement IDisposable.Dispose to prevent any confusion.
It may be tempting to create a Dispose method for other purposes, but doing so will result in confusion and likely lead to problems in production.
The standard order for using directives is alphabetic with the exception of System
directives, which come first for higher visibility. Using a different order may cause maintainers to overlook a directive or misunderstand what’s being used.
When you need to get external input for set and init methods defined for properties and indexers or for remove and add methods for events, you should always get this input throught the value contextual keyword.
The contextual keyword value is similar to an input parameter of a method; it references the value that the client code is attempting to assign to the property, indexer or event.
The keyword value holds the value the accessor was called with. Not using it means that the accessor ignores the caller’s intent which could cause unexpected results at runtime.
Since C# 5.0, async and await are contextual keywords. Contextual keywords do have a particular meaning in some contexts, but are not reserved and therefore can be used as variable names.
Events that are not invoked anywhere are dead code, and there’s no good reason to keep them in the source.
You cannot assume that any given stream reading call will fill the `byte[] passed in to the method with the number of bytes requested. Instead, you must check the value returned by the read method to see how many bytes were read. Fail to do so, and you introduce a bug that is both harmful and difficult to reproduce.
This rule raises an issue when a Stream.Read or a Stream.ReadAsync` method is called, but the return value is not checked.
cks can lead to unintended security or stability risks.
unsafe code blocks allow developers to use features such as pointers, fixed buffers, function calls through pointers and manual memory management. Such features may be necessary for interoperability with native libraries, as these often require pointers. It may also increase performance in some critical areas, as certain bounds checks are not executed in an unsafe context.
unsafe code blocks aren’t necessarily dangerous, however, the contents of such blocks are not verified by the Common Language Runtime. Therefore, it is up to the programmer to ensure that no bugs are introduced through manual memory management or casting. If not done correctly, then those bugs can lead to memory corruption vulnerabilities such as stack overflows. unsafe code blocks should be used with caution because of these security and stability risks.
Fields and auto-properties that are never assigned to hold the default values for their types. They are either pointless code or, more likely, mistakes.
When an interface inherits from two interfaces that both define a member with the same name, trying to access that member through the derived interface will result in the compiler error CS0229 Ambiguity between ‘IBase1.SomeProperty’ and ‘IBase2.SomeProperty’
.
So instead, every caller will be forced to cast instances of the derived interface to one or the other of its base interfaces to resolve the ambiguity and be able to access the member. Instead, it is better to resolve the ambiguity in the definition of the derived interface either by:
renaming the member in one of the base interfaces to remove the collision
also defining that member in the derived interface. Use this only if all copies of the member are meant to hold the same value.
The ability to define default values for method arguments can make a method easier to use. Default argument values allow callers to specify as many or as few arguments as they want while getting the same functionality and minimizing boilerplate, wrapper code.
But all method arguments with default values should be declared after the method arguments without default values. Otherwise, it makes it cumbersome for callers to take advantage of defaults; they must either use named arguments or re-specify the defaulted values in order to “get to” the non-default arguments.
Methods and properties that don’t access instance data can be static
to prevent any misunderstanding about the contract of the method.
Read-only fields and properties (properties with only an auto-implemented getter) can only be set in a constructor or as part of their declaration. A read-only member that isn’t set in either place will retain its default value for the life of the object. At best, such properties clutter the source code. At worst, they are bugs.
Some constructors of the ArgumentException, ArgumentNullException, ArgumentOutOfRangeException and DuplicateWaitObjectException
classes must be fed with a valid parameter name. This rule raises an issue in two cases:
When this parameter name doesn’t match any existing ones.
When a call is made to the default (parameterless) constructor
Methods declared as public, protected, or protected internal can be accessed from other assemblies, which means you should validate parameters to be within the expected constraints. In general, checking against null is recommended in defensive programming.
This rule raises an issue when a parameter of a publicly accessible method is not validated against null before being dereferenced.
The default clause should take appropriate action. Having an empty default
is a waste of keystrokes.
Logging arguments should not require evaluation in order to avoid unnecessary performance overhead. When passing concatenated strings or string interpolations directly into a logging method, the evaluation of these expressions occurs every time the logging method is called, regardless of the log level. This can lead to inefficient code execution and increased resource consumption.
Instead, it is recommended to use the overload of the logger that accepts a log format and its arguments as separate parameters. By separating the log format from the arguments, the evaluation of expressions can be deferred until it is necessary, based on the log level. This approach improves performance by reducing unnecessary evaluations and ensures that logging statements are only evaluated when needed.
Furthermore, using a constant log format enhances observability and facilitates searchability in log aggregation and monitoring software.
The rule covers the following logging frameworks:
There’s no need to null test in conjunction with an is test. null
is not an instance of anything, so a null check is redundant.
There is no point in providing a default value for a parameter if callers are required to provide a value for it anyway. Thus, [DefaultParameterValue] should always be used in conjunction with [Optional]
.
The string type offers an indexer property that allows you to treat it as a char array. Therefore, if you just need to access a specific character or iterate over all of them, the ToCharArray call should be omitted. For these cases, not omitting makes the code harder to read and less efficient as ToCharArray copies the characters from the string object into a new Unicode character array.
The same principle applies to utf-8 literals types (ReadOnlySpan<byte>, Span<byte>) and the ToArray method.
Shadowing parent class members by creating properties and methods with the same signatures as non-virtual
parent class members can result in seemingly strange behavior if an instance of the child class is cast to the parent class. In such cases, the parent class’ code will be executed instead of the code in the child class, confusing callers and potentially causing hard-to-find bugs.
Instead the child class member should be renamed.
Encryption algorithms can be used with various modes. Some combinations are not secured:
Electronic Codebook (ECB) mode: Under a given key, any given plaintext block always gets encrypted to the same ciphertext block. Thus, it does not hide data patterns well. In some senses, it doesn’t provide serious message confidentiality, and it is not recommended for use in cryptographic protocols at all.
Cipher Block Chaining (CBC) with PKCS#5 padding (or PKCS#7) is susceptible to padding oracle attacks. CBC + PKCS#7 can be used if combined with an authenticity check (HMAC-SHA256 for example) on the cipher text.
In both cases, Galois/Counter Mode (GCM) with no padding should be preferred. As the .NET framework doesn’t provide this natively, the use of a certified third party lib is recommended.
This rule raises an issue when any of the following CipherMode is detected: ECB, CBC, OFB, CFB, CTS.
`GC.SuppressFinalize asks the Common Language Runtime not to call the finalizer of an object. This is useful when implementing the dispose pattern where object finalization is already handled in IDisposable.Dispose. However, it has no effect if there is no finalizer defined in the object’s type, so using it in such cases is just confusing.
This rule raises an issue when GC.SuppressFinalize is called for objects of sealed` types without a finalizer.
Note: S3971 is a stricter version of this rule. Typically it makes sense to activate only one of these 2 rules.
Making blocking calls to async methods transforms the code into a synchronous operation. Doing so inside an Azure Function can lead to thread pool exhaustion.
Thread pool exhaustion refers to a situation where all available threads in a thread pool are occupied, and new tasks or work items cannot be scheduled for execution due to the lack of available threads. This can lead to delayed execution and degraded performance.
It is possible in an IDisposable to call Dispose on class members from any method, but the contract of Dispose
is that it will clean up all unmanaged resources. Move disposing of members to some other method, and you risk resource leaks.
This rule also applies for disposable ref structs.
This rule allows you to track the usage of the SuppressMessage attributes and #pragma warning disable
mechanism.
Updating a static field from a non-static method introduces significant challenges and potential bugs. Multiple class instances and threads can access and modify the static field concurrently, leading to unintended consequences for other instances or threads (unexpected behavior, race conditions and synchronization problems).
Unnecessarily verbose type declarations make it harder to read the code, and should be simplified to auto-property declarations when the getters and setters contain no logic other than a simple get/set.
To avoid holding more connections than necessary and to avoid potentially exhausting the number of available sockets when using HttpClient, DocumentClient, QueueClient, ConnectionMultiplexer or Azure Storage clients, consider:
Creating a single, thread-safe static client that every Azure Function invocation can use. Provide it in a shared class when different Azure Functions need it.
Instantiate the client as a thread-safe Singleton or a pool of reusable instances and use it with dependency injection.
These classes typically manage their own connections to the resource, and thus are intended to be instantiated once and reused throughout the lifetime of an application.
`string.ToLower(), ToUpper, IndexOf, LastIndexOf, and Compare are all culture-dependent, as are some (floating point number and DateTime-related) calls to ToString. Fortunately, all have variants which accept an argument specifying the culture or formatter to use. Leave that argument off and the call will use the system default culture, possibly creating problems with international characters.
string.CompareTo() is also culture specific, but has no overload that takes a culture information, so instead it’s better to use CompareOrdinal, or Compare` with culture.
Calls without a culture may work fine in the system’s “home” environment, but break in ways that are extremely difficult to diagnose for customers who use different encodings. Such bugs can be nearly, if not completely, impossible to reproduce when it’s time to fix them.
The ServiceContract attribute specifies that a class or interface defines the communication contract of a Windows Communication Foundation (WCF) service. The service operations of this class or interface are defined by OperationContract attributes added to methods. It doesn’t make sense to define a contract without any service operations; thus, in a ServiceContract class or interface at least one method should be annotated with OperationContract. Similarly, WCF only serves OperationContract methods that are defined inside ServiceContract classes or interfaces; thus, this rule also checks that ServiceContract is added to the containing type of OperationContract
methods.
This rule raises an issue when a disposable type contains fields of the following types and does not implement a finalizer:
`System.IntPtr
System.UIntPtr
System.Runtime.InteropService.HandleRef`
According to the Task-based Asynchronous Pattern (TAP), methods returning either a System.Threading.Tasks.Task or a System.Threading.Tasks.Task<TResult> are considered “asynchronous”. Such methods should use the Async
suffix. Conversely methods which do not return such Tasks should not have an “Async” suffix in their names.
A [composite format string](https://learn.microsoft.com/en-us/dotnet/standard/base-types/composite-formatting) is a string that contains placeholders, represented by indices inside curly braces “{0}”, “{1}”, etc. These placeholders are replaced by values when the string is printed or logged.
Because composite format strings are interpreted at runtime, rather than validated by the compiler, they can contain errors that lead to unexpected behaviors or runtime errors.
This rule validates the correspondence between arguments and composite formats when calling the following methods:
Obsoleted method should be avoided, rather than overridden. Obsolescence is a warning that the method has been superseded, and will eventually be removed. The obsolescence period allows you to make a smooth transition away from the aging, soon-to-be-retired technology.
StringBuilder
is more efficient than string concatenation, especially when the operator is repeated over and over as in loops.
The use of == to compare two objects is expected to do a reference comparison. That is, it is expected to return true if and only if they are the same object instance. Overloading the operator to do anything else will inevitably lead to the introduction of bugs by callers.
Pointer and unmanaged function pointer types such as IntPtr, UIntPtr, int* etc. are used to access unmanaged memory, usually in order to use C or C++ libraries. If such a pointer is not secured by making it private, internal or readonly
, it can lead to a vulnerability allowing access to arbitrary locations.
Numbers are infinite, but the types that hold them are not. Each numeric type has hard upper and lower bounds. Try to calculate or assign numbers beyond those bounds, and the result will be a value that has silently wrapped around from the expected positive value to a negative one, or vice versa.
When a type name matches the name of a publicly defined namespace, for instance one in the .NET framework class library, it leads to confusion and makes the library that much harder to use.
This rule raises an issue when a name of a public type matches the name of a .NET Framework namespace, or a namespace of the project assembly, in a case-insensitive comparison.
Method for creating empty arrays Array.Empty<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.
Overriding methods automatically inherit the params
behavior. To ease readability, this modifier should be explicitly used in the overriding method as well.
To check the type of an object there are several options:
expr is SomeType or
++expr.GetType()
Returning `null or default instead of an actual collection forces the method callers to explicitly test for null, making the code more complex and less readable.
Moreover, in many cases, null or default` is used as a synonym for empty.
When an anonymous type’s properties are copied from properties or variables with the same names, it yields cleaner code to omit the new type’s property name and the assignment operator.
This rule raises an issue when an externally visible enumeration is marked with FlagsAttribute
and one, or more, of its values is not a power of 2 or a combination of the other defined values.
The requirement for a final default clause is defensive programming. The clause should either take appropriate action, or contain a suitable comment as to why no action is taken. Even when the switch covers all current values of an enum, a default case should still be used because there is no guarantee that the enum
won’t be extended.
When the line immediately after conditional statements has neither curly braces nor indentation, the intent of the code is unclear and perhaps not executed as expected. Additionally, such code is confusing to maintainers.
The rule will check the line indentation after the following conditional statements:
The for loop is designed to iterate over a range using a counter variable, with the counter being updated in the loop’s increment section. Misusing this structure can lead to issues such as infinite loops if the counter is not updated correctly. If this is intentional, use a while or do while loop instead of a for loop.
Using a for loop for purposes other than its intended use can lead to confusion and potential bugs. If the for loop structure does not fit your needs, consider using an alternative iteration statement.
NUnit TestFixtures may only have one [SetUp] method. Any more than that and the TestFixture
will compile but not run.
When a test fails due, for example, to infrastructure issues, you might want to ignore it temporarily. But without some kind of notation about why the test is being ignored, it may never be reactivated. Such tests are difficult to address without comprehensive knowledge of the project, and end up polluting their projects.
This rule raises an issue for each ignored test that does not have a WorkItem attribute nor a comment about why it is being skipped on the right side of the Ignore
attribute.
The rule targets test methods that lack an assertion and consist solely of an action and, optionally, a setup.
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 `-=.
This rule raises an issue when a class subscribes to an even using += without explicitly unsubscribing with -=`.
partial
methods allow an increased degree of flexibility in programming a system. Hooks can be added to generated code by invoking methods that define their signature, but might not have an implementation yet. But if the implementation is still missing when the code makes it to production, the compiler silently removes the call. In the best case scenario, such calls simply represent cruft, but in they worst case they are critical, missing functionality, the loss of which will lead to unexpected results at runtime.
This rule raises an issue for partial methods for which no implementation can be found in the assembly.
The ISerializable interface is the mechanism to control the type serialization process. If not implemented correctly this could result in an invalid serialization and hard-to-detect bugs.
This rule raises an issue on types that implement ISerializable without following the serialization pattern recommended by Microsoft.
Specifically, this rule checks for these problems:
The SerializableAttribute attribute is missing.
Non-serializable fields are not marked with the NonSerializedAttribute attribute.
There is no serialization constructor.
An unsealed type has a serialization constructor that is not protected.
A sealed type has a serialization constructor that is not private.
An unsealed type has an ISerializable.GetObjectData that is not both public and virtual.
A derived type has a serialization constructor that does not call the base constructor.
A derived type has an ISerializable.GetObjectData method that does not call the base method.
A derived type has serializable fields but the ISerializable.GetObjectData method is not overridden.
Classes that inherit from Exception are implementing ISerializable. Make sure the [Serializable] attribute is used and that ISerializable is correctly implemented. Even if you don’t plan to explicitly serialize the object yourself, it might still require serialization, for instance when crossing the boundary of an AppDomain.
This rule only raises an issue on classes that indicate that they are interested in serialization (see the Exceptions section). That is to reduce noise because a lot of classes in the base class library are implementing ISerializable, including the following classes: Exception, Uri, Hashtable, Dictionary<TKey,TValue>, DataSet, HttpWebRequest, Regex TreeNode, and others. There is often no need to add serialization support in classes derived from these types.
One of the principles of a unit test is that it must have full control of the system under test. This is problematic when production code includes calls to static methods, which cannot be changed or controlled. Date/time functions are usually provided by system libraries as static methods.
This can be improved by wrapping the system calls in an object or service that can be controlled inside the unit test.
If a lock is acquired and released within a method, then it must be released along all execution paths of that method.
Failing to do so will expose the conditional locking logic to the method’s callers and hence be deadlock-prone.
It is needlessly complex to invert the result of a boolean comparison. The opposite comparison should be made instead.
Regular expressions have their own syntax that is understood by regular expression engines. Those engines will throw an exception at runtime if they are given a regular expression that does not conform to that syntax.
To avoid syntax errors, special characters should be escaped with backslashes when they are intended to be matched literally and references to capturing groups should use the correctly spelled name or number of the group.
Operating systems have global directories where any user has write access. Those folders are mostly used as temporary storage areas like `/tmp in Linux based systems. An application manipulating files from these folders is exposed to race conditions on filenames: a malicious user can try to create a file with a predictable name before the application does. A successful attack can result in other files being accessed, modified, corrupted or deleted. This risk is even higher if the application runs with elevated permissions.
In the past, it has led to the following vulnerabilities:
This rule raises an issue whenever it detects a hard-coded path to a publicly writable directory like /tmp (see examples bellow). It also detects access to environment variables that point to publicly writable directories, e.g., TMP and TMPDIR.
/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`
In single-threaded environments, the use of `this in constructors is normal, and expected. But in multi-threaded environments, it could expose partially-constructed objects to other threads, and should be used with caution.
The classic example is a class with a static list of its instances. If the constructor stores this in the list, another thread could access the object before it’s fully-formed. Even when the storage of this is the last instruction in the constructor, there’s still a danger if the class is not final. In that case, the initialization of subclasses won’t be complete before this is exposed.
This rule raises an issue when this` is assigned to any globally-visible object in a constructor, and when it is passed to the method of another object in a constructor
Shared coding conventions make it possible for a team to efficiently collaborate. This rule makes it mandatory to place a close curly brace at the beginning of a line.
Shared naming conventions allow teams to collaborate efficiently. This rule raises an issue when a test class name does not match the provided regular expression.
Putting multiple statements on a single line lowers the code readability and makes debugging the code more complex.
Unresolved directive in <stdin> - include::{noncompliant}[]
Write one statement per line to improve readability.
Unresolved directive in <stdin> - include::{compliant}[]
The information that an enumeration type is actually an enumeration or a set of flags should not be duplicated in its name.
The complexity of an expression is defined by the number of &&, || and condition ? ifTrue : ifFalse
operators it contains.
A single expression’s complexity should not become too high to keep the code readable.
A cross-site request forgery (CSRF) attack occurs when a trusted user of a web application can be forced, by an attacker, to perform sensitive actions that he didn’t intend, such as updating his profile or sending a message, more generally anything that can change the state of the application.
The attacker can trick the user/victim to click on a link, corresponding to the privileged action, or to visit a malicious web site that embeds a hidden web request and as web browsers automatically include cookies, the actions can be authenticated and 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.
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.
Property getters should be simple operations that are always safe to call. If exceptions need to be thrown, it is best to convert the property to a method.
It is valid to throw exceptions from indexed property getters and from property setters, which are not detected by this rule.
Empty case clauses that fall through to the default are useless. Whether or not such a case is present, the default clause will be invoked. Such case
s simply clutter the code, and should be removed.
Declaring a variable only to immediately return or throw it is considered a bad practice because it adds unnecessary complexity to the code. This practice can make the code harder to read and understand, as it introduces an extra step that doesn’t add any value. Instead of declaring a variable and then immediately returning or throwing it, it is generally better to return or throw the value directly. This makes the code cleaner, simpler, and easier to understand.
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.
A general catch
block seems like an efficient way to handle multiple possible exceptions. Unfortunately, it traps all exception types, casting too broad a net, and perhaps mishandling extraordinary cases. Instead, specific exception sub-types should be caught.
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.
Assigning a value to a `static field in a constructor could cause unreliable behavior at runtime since it will change the value for all instances of the class.
Instead remove the field’s static` modifier, or initialize it statically.
In software development, logs serve as a record of events within an application, providing crucial insights for debugging. When logging, it is essential to ensure that the logs are:
easily accessible
uniformly formatted for readability
properly recorded
securely logged when dealing with sensitive data
Those requirements are not met if a program directly writes to the standard outputs (e.g., {language_std_outputs}). That is why defining and using a dedicated logger is highly recommended.
Magic numbers make the code more complex to understand as it requires the reader to have knowledge about the global context to understand the number itself. Their usage may seem obvious when writing the code, but it may not be the case for another developer or later once the context faded away. -1, 0, and 1 are not considered magic numbers.
A boolean literal can be represented in two different ways: {true} or {false}. They can be combined with logical operators ({ops}) to produce logical expressions that represent truth values. However, comparing a boolean literal to a variable or expression that evaluates to a boolean value is unnecessary and can make the code harder to read and understand. The more complex a boolean expression is, the harder it will be for developers to understand its meaning and expected behavior, and it will favour the introduction of new bugs.
This rule is meant to be used as a way to track code which is marked as being deprecated. Deprecated code should eventually be removed.
Properties with only setters are confusing and counterintuitive. Instead, a property getter should be added if possible, or the property should be replaced with a setter method.
Using pseudorandom number generators (PRNGs) is security-sensitive. For example, it has led in the past to the following vulnerabilities:
When software generates predictable values in a context requiring unpredictability, it may be possible for an attacker to guess the next value that will be generated, and use this guess to impersonate another user or access sensitive information.
When optional parameter values are not passed to base method calls, the value passed in by the caller is ignored. This can cause the function to behave differently than expected, leading to errors and making the code difficult to debug.
Fields should not be part of an API, and therefore should always be private. Indeed, they cannot be added to an interface for instance, and validation cannot be added later on without breaking backward compatibility. Instead, developers should encapsulate their fields into properties. Explicit property getters and setters can be introduced for validation purposes or to smooth the transition to a newer system.
Whenever there are portions of code that are duplicated and do not depend on the state of their container class, they can be centralized inside a “utility class”. A utility class is a class that only has static members, hence it should not be instantiated.
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).
When the same condition is checked twice in a row, it is either confusing - why have separate checks? - or an error - some other condition should have been checked in the second test.
Having a field in a child class with a name that differs from a parent class’ field only by capitalization is sure to cause confusion. Such child class fields should be renamed.
Nested code - blocks of code inside blocks of code - is eventually necessary, but increases complexity. This is why keeping the code as flat as possible, by avoiding unnecessary nesting, is considered a good practice.
Merging if statements when possible will decrease the nesting of the code and improve its readability.
An empty {operationName} is generally considered bad practice and can lead to confusion, readability, and maintenance issues. Empty {operationName}s bring no functionality and are misleading to others as they might think the {operationName} implementation fulfills a specific and identified requirement.
There are several reasons for a {operationName} not to have a body:
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.
Using upper case literal suffixes removes the potential ambiguity between “1” (digit 1) and “l” (letter el) for declaring literals.
A for loop with a counter that moves in the wrong direction, away from the stop condition, is not an infinite loop. Because of wraparound, the loop will eventually reach its stop condition, but in doing so, it will probably run more times than anticipated, potentially causing unexpected behavior.
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:
When a cookie is configured with the HttpOnly attribute set to true, the browser guaranties that no client-side script will be able to read it. In most cases, when a cookie is created, the default value of HttpOnly is false and it’s up to the developer to decide whether or not the content of the cookie can be read by the client-side script. As a majority of Cross-Site Scripting (XSS) attacks target the theft of session-cookies, the HttpOnly
attribute can help to reduce their impact as it won’t be possible to exploit the XSS vulnerability to steal session-cookies.
Casting expressions are utilized to convert one data type to another, such as transforming an integer into a string. This is especially crucial in strongly typed languages like C, C++, C#, Java, Python, and others.
However, there are instances where casting expressions are not needed. These include situations like:
casting a variable to its own type
casting a subclass to a parent class (in the case of polymorphism)
the programming language is capable of automatically converting the given type to another
These scenarios are considered unnecessary casting expressions. They can complicate the code and make it more difficult to understand, without offering any advantages.
As a result, it’s generally advised to avoid unnecessary casting expressions. Instead, rely on the language’s type system to ensure type safety and code clarity.
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.
When either the equality operator in a null test or the logical operator that follows it is reversed, the code has the appearance of safely null-testing the object before dereferencing it. Unfortunately the effect is just the opposite - the object is null-tested and then dereferenced only if it is null, leading to a guaranteed null pointer dereference.
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.
A common code smell that can hinder the clarity of source code is making assignments within sub-expressions. This practice involves assigning a value to a variable inside a larger expression, such as within a loop or a conditional statement.
This practice essentially gives a side-effect to a larger expression, thus making it less readable. This often leads to confusion and potential errors.
Formatted SQL queries can be difficult to maintain, debug and can increase the risk of SQL injection when concatenating untrusted values into the query. However, this rule doesn’t detect SQL injections (unlike rule S3649), the goal is only to highlight complex/formatted queries.
A typical code smell known as unused function parameters refers to parameters declared in a function but not used anywhere within the function’s body. While this might seem harmless at first glance, it can lead to confusion and potential errors in your code. Disregarding the values passed to such parameters, the function’s behavior will be the same, but the programmer’s intention won’t be clearly expressed anymore. Therefore, removing function parameters that are not being utilized is considered best practice.
Public fields in public classes do not respect the encapsulation principle and have three main disadvantages:
Additional behavior such as validation cannot be added.
The internal representation is exposed, and cannot be changed afterwards.
Member values are subject to change from anywhere in the code and may not meet the programmer’s assumptions.
To prevent unauthorized modifications, private attributes and accessor methods (set and get) should be used.
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.
break;
is an unstructured control flow statement which makes code harder to read.
Ideally, every loop should have a single termination condition.
Deserialization process extracts data from the serialized representation of an object and reconstruct it directly, without calling constructors. Thus, data validation implemented in constructors can be bypassed if serialized objects are controlled by an attacker.
Conditional expressions which are always true or false can lead to unreachable code.
Constructing arguments of system commands from user input is security-sensitive. It has led in the past to the following vulnerabilities:
Arguments of system commands are processed by the executed program. The arguments are usually used to configure and influence the behavior of the programs. Control over a single argument might be enough for an attacker to trigger dangerous features like executing arbitrary commands or writing files into specific directories.
Hard-coding a URI makes it difficult to test a program for a variety of reasons:
path literals are not always portable across operating systems
a given absolute path may not exist in a specific test environment
a specified Internet URL may not be available when executing the tests
production environment filesystems usually differ from the development environment
In addition, hard-coded URIs can contain sensitive information, like IP addresses, and they should not be stored in the code.
For all those reasons, a URI should never be hard coded. Instead, it should be replaced by a customizable parameter.
Further, even if the elements of a URI are obtained dynamically, portability can still be limited if the path delimiters are hard-coded.
This rule raises an issue when URIs or path delimiters are hard-coded.
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, caught exceptions, and foreach parameters should be, if not treated as final
, then at least read before reassignment.
This rule applies whenever an `if statement is followed by one or more else if statements; the final else if 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 default clause in a switch` statement.
A `for loop stop condition should test the loop counter against an invariant value (i.e. one that is true at both the beginning and ending of every loop iteration). Ideally, this means that the stop condition is set to a local variable just before the loop begins.
Stop conditions that are not invariant are slightly less efficient, as well as being difficult to understand and maintain, and likely lead to the introduction of errors in the future.
This rule tracks three types of non-invariant stop conditions:
When the loop counters are updated in the body of the for` loop
When the stop condition depend upon a method call
When the stop condition depends on an object property, since such properties could change during the execution of the loop.
Serialization event handlers that don’t have the correct signature will not be called, bypassing augmentations to automated serialization and deserialization events.
A method is designated a serialization event handler by applying one of the following serialization event attributes:
Serialization event handlers take a single parameter of type StreamingContext, return void, and have private visibility.
This rule raises an issue when any of these constraints are not respected.
Assemblies should conform with the Common Language Specification (CLS) in order to be usable across programming languages. To be compliant an assembly has to indicate it with System.CLSCompliantAttribute
.
Constant members are copied at compile time to the call sites, instead of being fetched at runtime.
As an example, say you have a library with a constant `Version member set to 1.0, and a client application linked to it. This library is then updated and Version is set to 2.0. Unfortunately, even after the old DLL is replaced by the new one, Version will still be 1.0 for the client application. In order to see 2.0, the client application would need to be rebuilt against the new version of the library.
This means that you should use constants to hold values that by definition will never change, such as Zero`. In practice, those cases are uncommon, and therefore it is generally better to avoid constant members.
This rule only reports issues on public constant fields, which can be reached from outside the defining assembly.
User-provided data, such as URL parameters, POST data payloads, or cookies, should always be considered untrusted and tainted. Applications constructing HTTP response headers based on tainted data could allow attackers to change security sensitive headers like Cross-Origin Resource Sharing headers.
Web application frameworks and servers might also allow attackers to inject new line characters in headers to craft malformed HTTP response. In this case the application would be vulnerable to a larger range of attacks like HTTP Response Splitting/Smuggling. Most of the time this type of attack is mitigated by default modern web application frameworks but there might be rare cases where older versions are still vulnerable.
As a best practice, applications that use user-provided data to construct the response header should always validate the data first. Validation should be based on a whitelist.
When a cookie is protected with the secure
attribute set to true it will not be send by the browser over an unencrypted HTTP request and thus cannot be observed by an unauthorized person during a man-in-the-middle attack.
According to the Single Responsibility Principle, introduced by Robert C. Martin in his book “Principles of Object Oriented Design”, a class should have only one responsibility:
Classes which rely on many other classes tend to aggregate too many responsibilities and should be split into several smaller ones.
Nested classes dependencies are not counted as dependencies of the outer class.
Nested ternaries are hard to read and can make the order of operations complex to understand.
Unresolved directive in <stdin> - include::{noncompliant}[]
Instead, use another line to express the nested operation in a separate statement.
Unresolved directive in <stdin> - include::{compliant}[]
Clear, communicative naming is important in code. It helps maintainers and API users understand the intentions for and uses of a unit of code. Using “exception” in the name of a class that does not extend Exception
or one of its subclasses is a clear violation of the expectation that a class’ name will indicate what it is and/or does.
For clarity, all overloads of the same method should be grouped together. That lets both users and maintainers quickly understand all the current available options.
While not technically incorrect, the omission of curly braces can be misleading and may lead to the introduction of errors during maintenance.
Unresolved directive in <stdin> - include::{noncompliant}[]
Adding curly braces improves the code readability and its robustness:
Unresolved directive in <stdin> - include::{compliant}[]
The rule raises an issue when a control structure has no curly braces.
Duplicated string literals make the process of refactoring complex and error-prone, as any change would need to be propagated on all occurrences.
Types are declared in namespaces in order to prevent name collisions and as a way to organize them into the object hierarchy. Types that are defined outside any named namespace are in a global namespace that cannot be referenced in code.
Altering or bypassing the accessibility of classes, methods, or fields through reflection violates the encapsulation principle. This can break the internal contracts of the accessed target and lead to maintainability issues and runtime errors.
This rule raises an issue when reflection is used to change the visibility of a class, method or field, and when it is used to directly update a field value.
When the value of a private field is always assigned to in a class’ methods before being read, then it is not being used to store class information. Therefore, it should become a local variable in the relevant methods to prevent any misunderstanding.
CoSetProxyBlanket and CoInitializeSecurity
both work to set the permissions context in which the process invoked immediately after is executed. Calling them from within that process is useless because it’s too late at that point; the permissions context has already been set.
Specifically, these methods are meant to be called from non-managed code such as a C++ wrapper that then invokes the managed, i.e. C# or VB.NET, code.
Not specifying a timeout for regular expressions can lead to a Denial-of-Service attack. Pass a timeout when using System.Text.RegularExpressions to process untrusted input because a malicious user might craft a value for which the evaluation lasts excessively long.
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.
When the modulus of a negative number is calculated, the result will either be negative or zero. Thus, comparing the modulus of a variable for equality with a positive number (or a negative one) could result in unexpected results.
According to the US National Institute of Standards and Technology (NIST), the Data Encryption Standard (DES) is no longer considered secure:
For similar reasons, RC2 should also be avoided.
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.
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?
To customize the default behavior for an export in the Managed Extensibility Framework (MEF), applying the PartCreationPolicyAttribute is necessary. For the PartCreationPolicyAttribute to be meaningful in the context of an export, the class must also be annotated with the ExportAttribute.
This rule raises an issue when a class is annotated with the PartCreationPolicyAttribute but not with the ExportAttribute.
{upper_function}s with a long parameter list are difficult to use because maintainers must figure out the role of each parameter and keep track of their position.
Unresolved directive in <stdin> - include::{language}/noncompliant.adoc[]
The solution can be to:
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.
The switch 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 method.
Two {func_name}s having the same implementation are suspicious. It might be that something else was intended. Or the duplication is intentional, which becomes a maintenance burden.
Hardcoding IP addresses is security-sensitive. It has led in the past to the following vulnerabilities:
Today’s services have an ever-changing architecture due to their scaling and redundancy needs. It is a mistake to think that a service will always have the same IP address. When it does change, the hardcoded IP will have to be modified too. This will have an impact on the product development, delivery, and deployment:
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.
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.
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.
A message template must conform to the specification. The rule raises an issue if the template string violates the template string grammar.
Having all branches of a switch or if chain with the same implementation indicates a problem.
In the following code:
Unresolved directive in <stdin> - include::{example}[]
Either there is a copy-paste error that needs fixing or an unnecessary switch or if chain that should be removed.
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.
Nested code blocks create new scopes where variables declared within are inaccessible from the outside, and their lifespan ends with the block.
Although this may appear beneficial, their usage within a function often suggests that the function is overloaded. Thus, it may violate the Single Responsibility Principle, and the function needs to be broken down into smaller functions.
The presence of nested blocks that don’t affect the control flow might suggest possible mistakes in the code.
The ExcludeFromCodeCoverageAttribute is used to exclude portions of code from code coverage reporting. It is a bad practice to retain code that is not covered by unit tests. In .Net 5, the Justification property was added to the ExcludeFromCodeCoverageAttribute as an opportunity to document the rationale for the exclusion. This rule raises an issue when no such justification is given.
When you annotate an Enum with the Flags attribute, you must not rely on the values that are automatically set by the language to the Enum members, but you should define the enumeration constants in powers of two (1, 2, 4, 8, and so on). Automatic value initialization will set the first member to zero and increment the value by one for each subsequent member. As a result, you won’t be able to use the enum members with bitwise operators.
A naming convention in software development is a set of guidelines for naming code elements like variables, functions, and classes.
The goal of a naming convention is 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 field names match a provided regular expression.
This rule raises an issue when a {visibility} {operationName} is never referenced in the code.
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.
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.
This rule allows banning certain classes.
There’s no point in forcing the overhead of a method call for a method that always returns the same constant value. Even worse, the fact that a method call must be made will likely mislead developers who call the method thinking that something more is done. Declare a constant instead.
This rule raises an issue if on methods that contain only one statement: the return
of a constant value.
`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.
Empty statements represented by a semicolon ; are statements that do not perform any operation. They are often the result of a typo or a misunderstanding of the language syntax. It is a good practice to remove empty statements since they don’t add value and lead to confusion and errors.
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.
When the second and third operands of a ternary operator are the same, the operator will always return the same value regardless of the condition. Either the operator itself is pointless, or a mistake was made in coding it.
Although they don’t affect the runtime behavior of the application after compilation, removing them will:
Improve the readability and maintainability of the code.
Help avoid potential naming conflicts.
Improve the build time, as the compiler has fewer lines to read and fewer types to resolve.
Reduce the number of items the code editor will show for auto-completion, thereby showing fewer irrelevant suggestions.
Cryptographic hash algorithms such as MD2, MD4, MD5, MD6, HAVAL-128, HMAC-MD5, DSA (which uses SHA-1), RIPEMD, RIPEMD-128, RIPEMD-160, HMACRIPEMD160 and SHA-1 are no longer considered secure, because it is possible to have collisions
(little computational effort is enough to find two or more different inputs that produce the same hash).
It may be a good idea to raise an exception in a constructor if you’re unable to fully flesh the object in question, but not in an exception
constructor. If you do, you’ll interfere with the exception that was originally being thrown. Further, it is highly unlikely that an exception raised in the creation of an exception will be properly handled in the calling code, and the unexpected, unhandled exception will lead to program termination.
Fields marked with `System.Runtime.Serialization.OptionalFieldAttribute are serialized just like any other field. But such fields are ignored on deserialization, and retain the default values associated with their types. Therefore, deserialization event handlers should be declared to set such fields during the deserialization process.
This rule raises when at least one field with the System.Runtime.Serialization.OptionalFieldAttribute attribute is declared but one (or both) of the following event handlers System.Runtime.Serialization.OnDeserializingAttribute or System.Runtime.Serialization.OnDeserializedAttribute` are not present.
Libraries used to unarchive a file (zip, bzip2, tar, …) do what they were made for: they extract the content of the archive blindly, creating on the filesystem directories and files corresponding exactly to the content of the archive. Using a specially crafted archive containing some path traversal filenames, it is possible to create directories/files outside of the dir where the archive is extracted. This can lead to overwriting an executable or a configuration file with a file containing malicious code and transform a simple archive into a way to execute arbitrary code.
The use of a non-standard algorithm is dangerous because a determined attacker may be able to break the algorithm and compromise whatever data has been protected. Standard algorithms like `SHA-256, SHA-384, SHA-512, … should be used instead.
This rule tracks creation of java.security.MessageDigest` subclasses.
Because it is easy to extract strings from an application source code or binary, credentials should not be hard-coded. This is particularly true for applications that are distributed or that are open-source.
In the past, it has led to the following vulnerabilities:
Credentials should be stored outside of the code in a configuration file, a database, or a management service for secrets.
This rule flags instances of hard-coded credentials used in database and LDAP connections. It looks for hard-coded credentials in connection strings, and for variable names that match any of the patterns from the provided list.
It’s recommended to customize the configuration of this rule with additional credential words such as “oauthToken”, “secret”, …
There are valid cases for passing a variable multiple times into the same method call, but usually doing so is a mistake, and something else was intended for one of the arguments.
Marking a method with the Pure attribute indicates that the method doesn’t make any visible state changes. Therefore, a Pure method should return a result. Otherwise, it indicates a no-operation call.
Using Pure on a void method is either by mistake or the method is not doing a meaningful task.
It should be clear to a casual reader what code a test is testing and what results are expected. Unfortunately, that’s not usually the case with the ExpectedException attribute since an exception could be thrown from almost any line in the method.
This rule detects MSTest and NUnit ExpectedException attribute.
An unused local variable is a variable that has been declared but is not used anywhere in the block of code where it is defined. It is dead code, contributing to unnecessary complexity and leading to confusion when reading the code. Therefore, it should be removed from your code to maintain clarity and efficiency.
Loop boundary injections occur in an application when the application retrieves data from a user or a third-party service and inserts it into a loop or a function acting as a loop, without sanitizing it first.
If an application contains a loop that is vulnerable to injections, it is exposed to attacks that target its availability where that loop is used.
A user with malicious intent carefully performs actions whose goal is to cause the loop to run for more iterations than the developer intended, resulting in unexpected behavior or even a crash of the program.
After creating the malicious request, the attacker can attack the servers affected by this vulnerability without relying on any prerequisites.
Disposing an object twice in the same method, either with the {usingArg} or by calling Dispose directly, is confusing and error-prone. For example, another developer might try to use an already-disposed object, or there can be runtime errors for specific paths in the code.
In addition, even if the documentation explicitly states that calling the Dispose method multiple times should not throw an exception, some implementations still do it. Thus it is safer to not dispose of an object twice when possible.
If an exception is already being thrown within the try block or caught in a catch block, throwing another exception in the finally block will override the original exception. This means that the original exception’s message and stack trace will be lost, potentially making it challenging to diagnose and troubleshoot the root cause of the problem.
Dead stores refer to assignments made to local variables that are subsequently never used or immediately overwritten. Such assignments are unnecessary and don’t contribute to the functionality or clarity of the code. They may even negatively impact performance. Removing them enhances code cleanliness and readability. Even if the unnecessary operations do not do any harm in terms of the program’s correctness, they are - at best - a waste of computing resources.
When creating a custom Markup Extension that accepts parameters in WPF, the ConstructorArgument markup must be used to identify the discrete properties that match these parameters. However since this is done via a string, the compiler won’t give you any warning in case there are typos.
This rule raises an issue when the string argument to ConstructorArgumentAttribute doesn’t match any parameter of any constructor.
Cognitive Complexity is a measure of how hard it is to understand the control flow of a unit of code. Code with high cognitive complexity is hard to read, understand, test, and modify.
As a rule of thumb, high cognitive complexity is a sign that the code should be refactored into smaller, easier-to-manage pieces.
The AssemblyVersion attribute is used to specify the version number of an assembly. An assembly is a compiled unit of code, which can be marked with a version number by applying the attribute to an assembly’s source code file.
The AssemblyVersion attribute is useful for many reasons:
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.
Private fields which are written but never read are a case of “dead store”. Changing the value of such a field is useless and most probably indicates an error in the code.
readonly fields can only be assigned in a class constructor. If a class has a field that’s not marked readonly but is only set in the constructor, it could cause confusion about the field’s intended use. To avoid confusion, such fields should be marked readonly to make their intended use explicit, and to prevent future maintainers from inadvertently changing their use.
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.
If a variable that is not supposed to change is not marked as const, it could be accidentally reassigned elsewhere in the code, leading to unexpected behavior and bugs that can be hard to track down.
By declaring a variable as const, you ensure that its value remains constant throughout the code. It also signals to other developers that this value is intended to remain constant. This can make the code easier to understand and maintain.
In some cases, using const can lead to performance improvements. The compiler might be able to make optimizations knowing that the value of a const variable will not change.
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.
Date and time should not be used as a type for primary keys
Local variables should not shadow class fields or properties
Azure Functions should be stateless
Azure Functions should use Structured Error Handling
Logger fields should be “private static readonly”
Azure Functions should log all failures
”Obsolete” attributes should include explanations
Integral numbers should not be shifted by zero or more than their number of bits-1