C # - CodeAnt AI

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.

void Foo()
{
goto A; // Noncompliant
A:
while (condition1)
{
if (condition2)
{
  continue; // Noncompliant
}
else
{
  DoTheThing();
}
}
return; // Noncompliant; this is a void method
}

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

void Method(object value)
{
int i;
i = (int)value;   // Casting (explicit conversion) from float to int
}

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.

using System;

namespace MyLibrary
{
public class Base : IEquatable<Base> // Noncompliant
{
public bool Equals(Base other) 
{
  if (other == null) { return false; }
  // do comparison of base properties
  return true;
}

public override bool Equals(object other)  => Equals(other as Base);
}

class A : Base 
{
public bool Equals(A other) 
{
  if (other == null) { return false; }
  // do comparison of A properties
  return base.Equals(other);
}

public override bool Equals(object other)  => Equals(other as A); 
}

class B : Base 
{
public bool Equals(B other) 
{
  if (other == null) { return false; }
  // do comparison of B properties
  return base.Equals(other); 
}

public override bool Equals(object other)  => Equals(other as B);
}

internal class Program
{
static void Main(string[] args)
{
    A a = new A();
    B b = new B();
     Console.WriteLine(a.Equals(b)); // This calls the WRONG equals. This causes Base.Equals(Base)
                                     // to be called which only compares the properties in Base and ignores the fact that 
                                     // a and b are different types. In the working example A.Equals(Object) would have been 
                                     // called and Equals would return false because it correctly recognizes that a and b are 
                                     // different types. If a and b have the same base properties they will be returned as equal. 
}
}
}

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:

void TraceMessage([CallerMemberName] string memberName = "",
[CallerFilePath] string filePath = "",
[CallerLineNumber] int lineNumber = 0,
string message = null)  // Noncompliant: decorated parameters appear before "message" parameter
{
/* ... */
}

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.

public void GetReply(
     ref MyClass input, // Noncompliant
     out string reply)  // Noncompliant
{ ... }

It is important to be careful when searching for characters within a substring. Let’s consider the following example:

if (str.SubString(startIndex).IndexOf(char1) == -1) // Noncompliant: a new string is going to be created by "Substring"
{
// ...
}

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.

public static void Main(string[] args) { // Noncompliant
if (args.Length == 0)
{
throw new ArgumentException();
}
// do stuff
}

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:

class BaseClass
{
internal void MyMethod(string str) => Console.WriteLine("BaseClass: Method(string)");
}

class DerivedClass : BaseClass
{
internal void MyMethod(object str) => Console.WriteLine("DerivedClass: Method(object)"); // Noncompliant
}

// ...
BaseClass baseObj = new BaseClass();
baseObj.MyMethod("Hello"); // Output: BaseClass: Method(string)

DerivedClass derivedObj = new DerivedClass();
derivedObj.MyMethod("Hello"); // Output: DerivedClass: Method(object) - DerivedClass method is hiding the BaseClass method

BaseClass derivedAsBase = new DerivedClass();
derivedAsBase.MyMethod("Hello"); // Output: BaseClass: Method(string)

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.

var a = double.NaN;

if (a == double.NaN) // Noncompliant: always false
{
Console.WriteLine("a is not a number");
}
if (a != double.NaN)  // Noncompliant: always true
{
Console.WriteLine("a is not NaN");
}

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.

public class Foo
{
[ThreadStatic]
public static object PerThreadObject = new object(); // Noncompliant. Will be null in all the threads except the first one.
}

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.

public class MyClass
{
private void Format(string a, params object[] b) { }

private void Format(object a, object b, object c) { }
}

// ...
MyClass myClass = new MyClass();

myClass.Format("", null, null); // Noncompliant, resolves to the first Format with params, but was that intended?

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.

public void Base() 
{
Method(new string[] { "s1", "s2" }); // Noncompliant: unnecessary
Method(new string[] { });            // Noncompliant
Method(new string[12]);              // Compliant
}

public void Method(params string[] args)
{
// ...
}

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.

class Sample
{ 
public Sample() { } // Noncompliant
static Sample() { }  // Noncompliant
~Sample() { } // Noncompliant

...
}

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

namespace Foo
{
public class Bar
{
  public List<T> Method1(T arg) // Noncompliant
  {
       //...
  }
}
}

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.

if (GetTrue() | GetFalse()) // Noncompliant: both sides evaluated
{
}

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:

string select = "SELECT p.FirstName, p.LastName, p.PhoneNumber" +
    "FROM Person as p" +    // Noncompliant: concatenation results in "p.PhoneNumberFROM"
    "WHERE p.Id = @Id";     // Noncompliant: concatenation results in "pWHERE"

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.

string Interpolate(string value) =>
FormattableString.Invariant($"Value: {value}");

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.

class MyClass   // Noncompliant: should also override GetHashCode
{
public override bool Equals(object obj)
{
    // ...
}
}

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.

public void Foo(IEnumerable<Vehicle> seq, List<int> list)
{
var result1 = seq.Select(x => x as Car).Any(x => x != null);               // Noncompliant; use OfType
var result2 = seq.Select(x => x as Car).Any(x => x != null && x.HasOwner); // Noncompliant; use OfType before calling Any
var result3 = seq.Where(x => x is Car).Select(x => x as Car);              // Noncompliant; use OfType
var result4 = seq.Where(x => x is Car).Select(x => (Car)x);                // Noncompliant; use OfType
var result5 = seq.Where(x => x.HasOwner).Any();                            // Noncompliant; use Any([predicate])

var num = list.Count();                                                    // Noncompliant; use the Count property
var arr = seq.ToList().ToArray();                                          // Noncompliant; ToList is not needed
var count = seq.ToList().Count(x => x.HasOwner);                           // Noncompliant; ToList is not needed
}

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.

using System;

namespace MyLibrary
{
public class Foo 
{
    public void Bar(__arglist) // Noncompliant
    { 
        ArgIterator argumentIterator = new ArgIterator(__arglist);
        for(int i = 0; i < argumentIterator.GetRemainingCount(); i++) 
        { 
            Console.WriteLine(
                __refvalue(argumentIterator.GetNextArg(), string));
        } 
    }
}
}

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

using System;
using System.Collections;

public class MyData
{
public MyData()
{
}
}

public class MyList : CollectionBase // Noncompliant
{
public void Add(MyData data)
{
InnerList.Add(data);
}

// ...
}

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.

static void Main()
{
decimal dec = 3/2; // Noncompliant
Method(3/2); // Noncompliant
}

static void Method(float f) { }

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.

MyClass.Print(1);  // which overload of Print will be called?

public static class MyClass
{
public static void Print(int number) { } 
public static void Print(int number, string delimiter = "\n") { } // Noncompliant, default parameter value is hidden by overload
}

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.

private Color color = new Color("red");
private void DoSomething()
{
// Synchronizing access via "color"
lock (color) // Noncompliant: lock is actually on object instance "red" referred to by the "color" field
{
//...
color = new Color("green"); // other threads now allowed into this block
// ...
}
}

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`

var s = "foo";
var t = "fee fie foe " + s.ToString();  // Noncompliant
var someObject = new object();
var u = "" + someObject.ToString(); // Noncompliant
var v = string.Format("{0}", someObject.ToString()); // Noncompliant

When a loop is filtering, selecting or aggregating, those functions can be handled with a clearer, more concise LINQ expression instead.

var result = new List<string>();
foreach (var element in collection)  // Noncompliant
{
if (condition(element))
{
result.Add(element);
}
}

foreach (var element in collection2) // Noncompliant
{
var someValue = element.Property;
if (someValue != null)
{
result.Add(someValue);
}
}

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.

public int GetLengthPlusTwo(string str)
{
try
{
    return str.Length + 2;
}
catch (NullReferenceException e)
{
    return 2;
}
}

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 sealed class MySealedClass 
{
protected string name = "Fred";  // Noncompliant
protected void SetName(string name) // Noncompliant
{
    // ...
}
}

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

public class A 
{
public static string[] strings1 = {"first","second"};  // Noncompliant
public static List<String> strings3 = new List<String>();  // Noncompliant
}

In general, async void test methods are not executed by test frameworks, therefore it’s better to avoid them altogether.

[TestMethod]
public async void MyIgnoredTestMethod()  // Noncompliant
{ /* ... */ }

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.

// Conventional mapping definition
app.MapControllerRoute(
name: "default",
pattern: "{controller=Home}/{action=Index}/{id?}");

public class PersonController
{
// Conventional routing:
// Matches e.g. /Person/Index/123
public IActionResult Index(int? id) => View();

// Attribute routing:
// Matches e.g. /Age/Ascending (and model binds "Age" to sortBy and "Ascending" to direction)
// but does not match /Person/List/Age/Ascending
[HttpGet(template: "{sortBy}/{direction}")]
public IActionResult List(string sortBy, SortOrder direction) => View();
}

Using string.Equals to determine if a string is empty is significantly slower than using string.IsNullOrEmpty() or checking for ++string.Length

"".Equals(name); // Noncompliant
!name.Equals(""); // Noncompliant
name.Equals(string.Empty); // Noncompliant

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.

@code {
[JSInvokable]
private static void MyStaticMethod() { } // Noncompliant

[JSInvokable]
internal void MyMethod() { } // Noncompliant
}

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

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

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

This rule raises when the following statements are misplaced:

public object Method(IEnumerable<object> items)
{
for (int i = 0; i < 10; i++)
{
    Console.WriteLine(i);
    break; // Noncompliant: loop only executes once
}

foreach (object item in items)
{
    return item; // Noncompliant: loop only executes once
}
return null;
}

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.

public interface IMyInterface
{
void DoStuff();
}

public class MyClass1 : IMyInterface
{
public int Data { get { return new Random().Next(); } }

public void DoStuff()
{
// TODO...
}
}

public static class DowncastExampleProgram
{
static void EntryPoint(IMyInterface interfaceRef)
{
MyClass1 class1 = (MyClass1)interfaceRef;  // Noncompliant
int privateData = class1.Data;

class1 = interfaceRef as MyClass1;  // Noncompliant
if (class1 != null)
{
  // ...
}
}
}

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

public abstract class Animal //Noncompliant; should be an interface
{
abstract void Move();
abstract void Feed();
}

public abstract class Color //Noncompliant; should be concrete with a protected constructor
{
private int red = 0;
private int green = 0;
private int blue = 0;

public int GetRed()
{
return red;
}
}

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.

Unhandled exception rendering component: Querystring values cannot be parsed as type '<type>'.
System.NotSupportedException: Querystring values cannot be parsed as type '<type>'
...

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.

using System; 
using System.Windows.Forms;

namespace MyLibrary
{
public class MyForm: Form
{
    public MyForm()
    {
        this.Text = "Hello World!";
    }

    public static void Main()  // Noncompliant
    {
        var form = new MyForm();
        Application.Run(form);
    }
}
}

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.

object a = null, b = null, x;

if (a != null) // Noncompliant; needlessly verbose
{
x = a;
}
else
{
x = b;
}

x = a != null ? a : b; // Noncompliant; better but could still be simplified

x = (a == null) ? new object() : a; // Noncompliant

if (condition) // Noncompliant
{
x = a;
}
else
{
x = b;
}

if (a == null)  // Noncompliant
a = new object();

var y = null ?? new object(); // Noncompliant

a = a ?? new object();  // Noncompliant for C# 8

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[])`.

s = string.Format("{0}", arg0, arg1); // Noncompliant, arg1 is declared but not used.
s = string.Format("{0} {2}", arg0, arg1, arg2); // Noncompliant, the format item with index 1 is missing so arg1 will not be used.
s = string.Format("value is " + value);  // Noncompliant; use an argument instead of concatenaion
s = string.Format("no argument here");

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.

[Serializable]
public class Person : ISerializable {  // Noncompliant; missing serializable constructor
public void GetObjectData (SerializationInfo info, StreamingContext context) {
// ...
}
}

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.

using System;

namespace MyLibrary
{
public class Foo
{
public bool HaveSameNames(string name1, string name2)
{
  return string.Compare(name1, name2) == 0; // Noncompliant
}
}
}

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.

IEnumerable<int> numbers = GetNumbers();

var count = numbers.Count(); // causes an iteration
var last = numbers.Last(); // Noncompliant; causes an iteration

foreach(var x in numbers)  // Noncompliant
{
// ...
}

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.

class X
{
public X() { } // Noncompliant
static X() { }  // Noncompliant
~X() { } // Noncompliant

...
}

class Y : X
{
public Y(int parameter) : base() // Noncompliant
{
/* does something with the parameter */
}
}

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.

[DebuggerDisplay("Name: {Name}")] // Noncompliant - Name doesn't exist in this context
public class Person
{
public string FullName { get; private set; }
}

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.

using System;

namespace MyLibrary
{
class Foo
{
  private IntPtr res;

  Foo()
  {
     GetRes (res);
  }

  ~Foo()
  {
     FreeRes (res);
  }

  void Bar()  // Noncompliant
  {
     UseRes(res); 
  }

  // Methods that would typically make calls to unmanaged code.
  void GetRes(IntPtr p)
  {
    // Allocate the resource ...
  }
  void FreeRes(IntPtr p)
  {
    // Free the resource and set the pointer to null ...
  }
  void UseRes(IntPtr p)
  {
    // Use the resource in unmanaged code ...
  }

}
}

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.

public class Person : ISerializable {  // Noncompliant; [Serializable] attribute missing
// ...
}

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.

public interface IMyInterface
{
}

public class MyClass : IMyInterface
{
public override bool Equals(object obj)
{
    //...
}
}

public class Program
{
public static void Method(IMyInterface instance1, IMyInterface instance2)
{
    if (instance1 == instance2) // Noncompliant, will do reference equality check, but was that intended? MyClass overrides Equals.
    {
        Console.WriteLine("Equal");
    }
}
}

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.

public class LengthLimitedSingletonCollection<T> where T : new() 
{
protected const int MaxAllowedLength = 5;
protected static Dictionary<Type, object> instances = new Dictionary<Type, object>(); // Noncompliant

public static T GetInstance() 
{
object instance;

if (!instances.TryGetValue(typeof(T), out instance)) 
{
  if (instances.Count >= MaxAllowedLength) 
  {
    throw new Exception();
  }
  instance = new T();
  instances.Add(typeof(T), instance);
}
return (T)instance;
}
}

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.

void DoChecks<T>(Nullable<T> value) where T : struct
{
bool areEqual = value.GetType() == typeof(Nullable<int>); // Noncompliant: always false
bool areNotEqual = value.GetType() != typeof(Nullable<int>); // Noncompliant: always true

Nullable<int> nullable = null;
bool nullComparison = nullable.GetType() != typeof(Nullable<int>); // Noncompliant: throws NullReferenceException
}

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.

void Add(List<int> list)
{
unchecked 
{
try 
{
  int total = list.Sum();
} 
catch (System.OverflowException e) 
{
  // Exception handling    
}
}
}

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.

typeof(Type).GetType(); // Can be used by convention to get an instance of 'System.RuntimeType'

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

string text = "";
try 
{
text = File.ReadAllText(fileName);
} 
catch (Exception exc) // Noncompliant
{  
}

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.

public int this[MyCustomClass index]  // Noncompliant
{  
// get and set accessors  
}

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.

void DoSomething(int someParameter, string anotherParam)
{
if (someParameter < 0) 
{
    throw new ArgumentException("Bad argument", "someParameter");  // Noncompliant
}
if (anotherParam == null)
{
    throw new Exception("anotherParam should not be null"); // Noncompliant
}
}

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.

try
{
// do something that might throw a FileNotFoundException or IOException
}
catch (Exception e) // Noncompliant
{
// log exception ...
}

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

public class FooBarController : Controller
{
[HttpPost] // Noncompliant
[ValidateInput(false)] 
public ActionResult Purchase(string input)
{
    return Foo(input);
}

[HttpPost] // Noncompliant
public ActionResult PurchaseSomethingElse(string input)
{
    return Foo(input);
}
}

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

[TestMethod]
public void SomeTest()
{
Thread.Sleep(500); // Noncompliant
// assertions...
}

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.

public void doSomething(Object obj) 
{  
lock (this) //first lock
{
lock (obj) {  // second lock
  // ...
  Monitor.Pulse(obj); // Noncompliant; only the second lock is released
}
}
}

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.

interface IConsumer<T>  // Noncompliant
{
bool Eat(T fruit);
}

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.

struct MyStruct  // Noncompliant
{
public int Value { get; set; }
}

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.

class Base
{
public virtual void Method(int[] numbers)
{
...
}
}
class Derived : Base
{
public override void Method(params int[] numbers) // Noncompliant, method can't be called with params syntax.
{
...
}
}

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.

Debug.Assert(list.Remove("dog"));

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.

class Foo
{
int start;

Foo() : this(0)  { }
Foo(int v) : this()   { }  // Noncompliant
}

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

public partial class MyClass // Noncompliant
{
public virtual void Method()
{
}
}

public sealed class MyOtherClass : MyClass
{
public sealed override void Method() // Noncompliant
{
}
}

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.

anyValue & -1 // Noncompliant
anyValue      // Compliant

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.

float f = 42.0f;
if (f <= double.MaxValue) { } // Noncompliant: always true
if (f > double.MaxValue) { }  // Noncompliant: always false

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.

public override string ToString ()
{
if (this.collection.Count == 0) 
{
return null; // Noncompliant
} 
else 
{
// ...
}
}

The switch statement is a conditional statement that executes a sequence of instructions based on patterns matching the provided value.

switch (temperatureInCelsius)
{
case < 35.0:
    Console.WriteLine("Hypothermia");
    break;
case >= 36.5 and <= 37.5:
    Console.WriteLine("Normal");
    break;
case > 37.5 and <= 40.0:
    Console.WriteLine("Fever or hyperthermia");
    break;        
case > 40.0:
    Console.WriteLine("Hyperpyrexia");
    break;
}

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.

using System;
using System.Security;

namespace MyLibrary
{
[SecuritySafeCritical]
public class Foo
{
    [SecurityCritical] // Noncompliant
    public void Bar()
    {
    }
}
}

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.

public class Outer
{
private const int base = 42;

private static void Print(int num)  // Noncompliant - static method is only used by the nested class, should be moved there
{
    Console.WriteLine(num + base);
}

public class Nested
{
    public void SomeMethod()
    {
        Outer.Print(1);
    }
}
}

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.

using System;

namespace MyLibrary
{
public class Foo
{
    private string name;

    public string GetName()  // Noncompliant
    {
        return name;
    }
}
}

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.

public void doSomething(int max) {
for (int i = 0; i < max; i++) {
doSynchronized(i);  // Noncompliant
}
}

[MethodImpl(MethodImplOptions.Synchronized)]
public void doSynchronized(int val) {
// ...
}

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.

public class MyClass 
{
[ThreadStatic]  // Noncompliant
private int count = 0;

// ...
}

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.

public class Foo 
{
~Foo() // Noncompliant
{ 
}
}

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.

using System;

namespace MyLibrary
{
public class Foo
{
    public DateTime Date
    {
        get { return DateTime.Today; }
    }

    public string GetDate() // Noncompliant
    {
        return this.Date.ToString();
    }
}
}

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.

MyDelegate first, second, third, fourth;
first = () => Console.Write("1");
second = () => Console.Write("2");
third = () => Console.Write("3");
fourth = () => Console.Write("4");

MyDelegate chain1234 = first + second + third + fourth; // Compliant - chain sequence = "1234"
MyDelegate chain12 = chain1234 - third - fourth; // Compliant - chain sequence = "12"


MyDelegate chain14 = first + fourth; // creates a new MyDelegate instance which is a list under the covers
MyDelegate chain23 = chain1234 - chain14; // Noncompliant; (first + fourth) doesn't exist in chain1234


// The chain sequence of "chain23" will be "1234" instead of "23"!
// Indeed, the sequence "1234" does not contain the subsequence "14", so nothing is subtracted
// (but note that "1234" contains both the "1" and "4" subsequences)
chain23 = chain1234 - (first + fourth); // Noncompliant

chain23(); // will print "1234"!

Once you modify a closure, any use of it could provide unexpected results.

var x = 0;
Func<int> f1 = () => x;  // Noncompliant
x = 1;
Console.WriteLine(f1());

var input = new[] { 1, 2, 3 };
var fs = new List<Func<int>>();
for (var i = 0; i < input.Length; i++) {
Func<int> f = () => input[i];  // Noncompliant
fs.Add(f);
}
Console.WriteLine(fs[0]()); //Access to modified closure yields Exception

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`

using System;

namespace MyLibrary
{
public class Foo
{
    public void Bar(String string1)
    {
        if(string.Compare(string1, string2, false) == 0) // Noncompliant
        {
            Console.WriteLine(string3.ToLower()); // Noncompliant
        }
    }
}
}

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.

using System;

struct MyStruct
{
int valueA;
int valueB;
}

static class MyClass
{
public static void Method(MyStruct struct1, MyStruct struct2)
{
    if (Object.ReferenceEquals(struct1, struct2)) // Noncompliant: this will be always false
    {
        // ...
    }
}
}

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.

var response = await httpClient.GetAsync(url);  // Noncompliant

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

using System;
using System.Data;

namespace MyLibrary
{
public class Foo
{
    public DataTable CreateTable()
    {
        DataTable table = new DataTable("Customers"); // Noncompliant table.Locale not set
        DataColumn key = table.Columns.Add("ID", typeof(Int32));

        key.AllowDBNull = false;
        key.Unique = true;
        table.Columns.Add("LastName", typeof(String));
        table.Columns.Add("FirstName", typeof(String));
        return table;
    }
}
}

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.

if (x is Fruit)  // Noncompliant
{
var f = (Fruit)x; // or x as Fruit
// ...
}

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.

dynamic d = 5;
var x = d >> 5.4;   // Noncompliant
x = d << null;      // Noncompliant
x <<= new object(); // Noncompliant

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 (=)

if ( a == a ) // always true
{ 
doZ();
}
if ( a != a ) // always false
{
doY();
}
if ( a == b && a == b ) // if the first one is true, the second one is too
{ 
doX();
}
if ( a == b || a == b ) // if the first one is true, the second one is too
{ 
doW();
}

int j = 5 / 5; // always 1
int k = 5 - 5; // always 0

c.Equals(c);    // always true
Object.Equals(c, c); // always true

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:

float f = 0.100000001f; // 0.1
double d = 0.10000000000000001; // 0.1

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

public async void button1_Click(object sender, EventArgs e)
{
await DoSomethingAsync();
}

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

class MyClass  // Noncompliant
{
public bool Equals(MyClass other)
{
//...
}
}

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.

if (param == 1)
{
OpenWindow();
}
else if (param == 2)
{
CloseWindow();
}
else if (param == 1) // Noncompliant: condition has already been checked
{
MoveWindowToTheBackground(); // unreachable code
}

Having an infinite loop or recursion will lead to a program failure or a program never finishing the execution.

public int Sum()
{
var i = 0;
var result = 0;
while (true) // Noncompliant: the program will never stop
{
    result += i;
    i++;
}
return result;
}

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

using System;
using System.Globalization;
using System.Reflection;
using System.Windows.Forms;

[assembly: NeutralResourcesLanguageAttribute("en-US")]
namespace MyLibrary
{
public class Foo
{
    public void SetHour(int hour)
    {
        if (hour < 0 || hour > 23)
        {
            MessageBox.Show("The valid range is 0 - 23."); // Noncompliant
        }
    }
}
}

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.

int [,] myArray =  // Noncompliant
{
    {1,2,3,4},
    {5,6,7,0},
    {8,0,0,0},
    {9,0,0,0}
};
// ...
myArray[1,1] = 0;

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.

var x = personList
.OrderBy(person => person.Age)
.OrderBy(person => person.Name)  // Noncompliant
.ToList();  // x is sorted by Name, not sub-sorted

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.

class MyClass
{
public void DoStuff([Optional] ref int i) // Noncompliant
{
Console.WriteLine(i);
}

public static void Main()
{
new MyClass().DoStuff(); // Compilation Error [CS7036]
}
}

Shared naming conventions allow teams to collaborate efficiently.

This rule raises an issue when a type name is not PascalCased.

For example, the classes

class my_class {...}
class SOMEName42 {...}

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.

public void DoStuff([DefaultValue(4)] int i) 
{ 
// i is not automatically assigned 4
}

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.

class X
{
public int field = 0; // Noncompliant
public object o = null; // Noncompliant
public object MyProperty { get; set; } = null; // Noncompliant
public event EventHandler MyEvent = null;  // Noncompliant
}

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.

public class MyClass : Object  // Noncompliant

enum MyEnum : int  // Noncompliant

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.

static void Main(string[] args)
{
Assembly.LoadFrom(...); // Noncompliant
Assembly.LoadFile(...); // Noncompliant
Assembly.LoadWithPartialName(...); // Noncompliant + deprecated
}

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.

string actual = "Using Fluent Assertions";
actual.Should(); // Noncompliant

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.

private void DoSomething()
{
// ...
Console.WriteLine("so far, so good..."); // Noncompliant
// ...
}

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.

@page "/my-route/{Param:datetime}"

@code {
[Parameter]
public string Param { get; set; } // Noncompliant
}

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

switch (variable)
{
case 0:
doSomething();
break;
default:
doSomethingElse();
break;
}

var foo = variable switch
{
0 => doSomething(),
_ => doSomethingElse(),
}

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.

internal class MyClass
{
public static decimal PI = 3.14m;  // Noncompliant

public int GetOne() // Noncompliant
{
    return 1;
}

protected record NestedType // Noncompliant: outer class is internal
{
    public bool FlipCoin() // Noncompliant: outer class is internal
    {
        return false;
    }
    // ...
}
}

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.

abstract class Base
{
public Base() // Noncompliant: should be private, private protected or protected.
{
  //...
}
}

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.

Func<int, int> lambda1 = x => x + 1;
Func<int, int> lambda2 = x => x + 1;

var result = lambda1 == lambda2; // result is false here

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

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

namespace myLib
{
public class Foo
{
static readonly int x = 1;  // Noncompliant
static readonly int y = x + 4; // Noncompliant
static readonly string s = "Bar";  // Noncompliant
}
}

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:

ASCII, for example \n and \t
Unicode, for example U+000D and U+0009

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

string tabInside = "A	B";                 // Noncompliant: contains a tabulation
string zeroWidthSpaceInside = "foo​bar";     // Noncompliant: contains a U+200B character inside
Console.WriteLine(zeroWidthSpaceInside);    // Prints "foo?bar"

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.

[TestMethod]
void TestNullArg()  // Noncompliant, method is not public
{  /* ... */  }

[TestMethod]
public async void MyIgnoredTestMethod()  // Noncompliant, this is an 'async void' method
{ /* ... */ }

[TestMethod]
public void MyIgnoredGenericTestMethod<T>(T foo)  // Noncompliant, method has generics in its signature
{ /* ... */ }

In C#, the throw statement can be used in two different ways:

by specifying an expression
without specifying an expression

try
{
}
catch(Exception exception)
{
// code that uses the exception
throw exception; // The exception stack trace is cleared up to this point.
}

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

if(collection.Count >= 0){...} // Noncompliant: always true

if(array.Length >= 0){...} // Noncompliant: 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`

using System;
using System.Collections;

namespace MyLibrary
{
public class MyCollection : CollectionBase  // Noncompliant
{
}
}

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

public class Choice {
private bool selected;

public void MakeChoice(bool selected)
{
    selected = selected; // Noncompliant
}
}

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.

static void Main(string[] args)
{
// ...
GC.Collect();                              // Noncompliant
GC.Collect(2, GCCollectionMode.Optimized); // Noncompliant
}

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.

public class Foo: IComparable  // Noncompliant
{
public int CompareTo(object obj) { /* ... */ }
}

Default arguments are determined by the static type of the object.

class Base
{
public virtual void Run(int distance = 42) { /* ... */ }
}

class Derived : Base
{
public override void Run(int distance = 5) { /* ... */ }
}

Base x = new Base();
x.Run(); // Here the default value of distance is 42
Derived d = new Derived();
d.Run(); // Here the default value of distance is 5
Base b = new Derived();
b.Run(); // Here the default value of distance is 42, not 5

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.

public class Foo1 : IDisposable // Noncompliant - provide protected overridable implementation of Dispose(bool) on Foo or mark the type as sealed.
{
public void Dispose() // Noncompliant - should contain only a call to Dispose(true) and then GC.SuppressFinalize(this)
{
    // Cleanup
}
}

public class Foo2 : IDisposable
{
void IDisposable.Dispose() // Noncompliant - Dispose() should be public
{
    Dispose(true);
    GC.SuppressFinalize(this);
}

public virtual void Dispose() // Noncompliant - Dispose() should be sealed
{
    Dispose(true);
    GC.SuppressFinalize(this);
}
}

public class Foo3 : IDisposable
{
public void Dispose()
{
    Dispose(true);
    GC.SuppressFinalize(this);
}

protected virtual void Dispose(bool disposing)
{
    // Cleanup
}

~Foo3() // Noncompliant - Modify Foo.~Foo() so that it calls Dispose(false) and then returns.
{
    // Cleanup
}
}{code}

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.

if (food is Pizza pizza)

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.

// file path: Gui/Screen.cs
namespace Green  // Noncompliant
{
class Screen
{
}
}

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

The default configuration is the one recommended by Microsoft:

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

public enum foo // Noncompliant
{
FooValue = 0
}

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 System;
using System.IO;
using System.Runtime.Serialization;
using System.Runtime.Serialization.Formatters.Binary;
using System.Security;
using System.Security.Permissions;

[assembly: AllowPartiallyTrustedCallersAttribute()]
namespace MyLibrary
{
[Serializable]
public class Foo : ISerializable
{
    private int n;

    [FileIOPermissionAttribute(SecurityAction.Demand, Unrestricted = true)]
    public Foo()
    {
       n = -1;
    }

    protected Foo(SerializationInfo info, StreamingContext context) // Noncompliant
    {
       n = (int)info.GetValue("n", typeof(int));
    }

    void ISerializable.GetObjectData(SerializationInfo info, StreamingContext context)
    {
       info.AddValue("n", n);
    }
}
}

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.

public Person GetBasePerson() 
{
return delegate () { base.GetThePerson();  } // Noncompliant
}

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.

public class Person
{
public int age;
public string name;

public override int GetHashCode()
{
int hash = 12;
hash += this.age.GetHashCode(); // Noncompliant
hash += this.name.GetHashCode(); // Noncompliant
return hash;
}

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.

public static class MyExtensions
{
public static void SomeExtension(this object obj) // Noncompliant
{
    // ...
}
}

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.

namespace myLib
{
public class Foo
{
static int i;
static string s;

static Foo() // Noncompliant
{
  i = 3;
  ResourceManager sm =  new ResourceManager("strings", Assembly.GetExecutingAssembly());
  s = sm.GetString("mystring");
}
}
}

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

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

[Flags]
enum Days
{
Monday = 1,    // 0b00000001
Tuesday = 2,   // 0b00000010
Wednesday = 4, // 0b00000100
Thursday = 8,  // 0b00001000
Friday = 16,   // 0b00010000
Saturday = 32, // 0b00100000
Sunday = 64    // 0b01000000
}

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.

var l = new List<int>() {}; // Noncompliant, {} can be removed
var o = new object() {}; // Noncompliant, {} can be removed

var ints = new int[] {1, 2, 3}; // Noncompliant, int can be omitted
ints = new int[3] {1, 2, 3}; // Noncompliant, the size specification can be removed

int? i = new int?(5); // Noncompliant new int? could be omitted, it can be inferred from the declaration, and there's implicit conversion from T to T?
var j = new int?(5);

Func<int, int> f1 = (int i) => 1; //Noncompliant, can be simplified

class Class
{
private event EventHandler MyEvent;

public Class()
{
    MyEvent += new EventHandler((a,b)=>{ }); // Noncompliant, needlessly verbose
}
}

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

public int doSomething() {...} // Noncompliant

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

public void DoSomething(List<string> strings) {
var sb = new StringBuilder();  // Noncompliant
sb.Append("Got: ");
foreach(var str in strings) {
sb.Append(str).Append(", ");
// ...
}
}

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.

if (condition1)                  // Compliant - depth = 1
{             
/* ... */
if (condition2)                // Compliant - depth = 2
{              
/* ... */
for (int i = 0; i < 10; i++)  // Compliant - depth = 3
{
  /* ... */
  if (condition4)            // Noncompliant - depth = 4, which exceeds the limit
  {            
    if (condition5)          // Depth = 5, exceeding the limit, but issues are only reported on depth = 4
    {          
      /* ... */
    }
    return;
  }
}
}
}

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.

public void Method(MyObject myObject)
{
if (myObject is null)
{
    new MyObject(); // Noncompliant
}

if (myObject.IsCorrupted)
{
    new ArgumentException($"{nameof(myObject)} is corrupted"); // Noncompliant
}

// ...
}

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

using System;

namespace MyLibrary
{
public enum Color
{ 
    None, 
    Red, 
    Orange, 
    Yellow,
    ReservedColor  // Noncompliant
}  
}

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

public Stream WriteToFile(string path, string text)
{
var fs = new FileStream(path, FileMode.Open); // Compliant: it is returned
var bytes = Encoding.UTF8.GetBytes(text);
fs.Write(bytes, 0, bytes.Length);
return fs;
}

public void ReadFromStream(Stream s)
{
var sr = new StreamReader(s); // Compliant: it would close the underlying stream.
// ...
}

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:

int NthFibonacciNumber(int n)
{
if (n <= 1)
{
    return 1;
}
else
{
    return NthFibonacciNumber(n - 1) + NthFibonacciNumber(n - 2);
}
}

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.

public struct S1
{
public int value;

public void SetValue()
{
    value = 10;
}
}

class Test
{
static readonly S1 first;
static S1 second;

static void Main()
{
    first.SetValue();  // Noncompliant
    second.SetValue();
    Console.WriteLine(first.value);  // Surprise! This writes 0
    Console.WriteLine(second.value); // This writes 10
}
}

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.

public class Fruit { }
public class Orange : Fruit { }
public class Apple : Fruit { }

class MyTest
{
public void Test()
{
var fruitBasket = new List<Fruit>();
fruitBasket.Add(new Orange());
fruitBasket.Add(new Orange());
fruitBasket.Add(new Apple());

foreach (Orange orange in fruitBasket) // Noncompliant
{
  //...
} 
}
}

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.

public class Base
{
public void SomeMethod(int count) { }
}
public class Derived : Base
{
private void SomeMethod(int count) { } // Noncompliant
}

class Program
{
public void DoWork()
{
    var derived = new Derived();
    derived.SomeMethod(42); // Base.SomeMethod is accessed here
}
}

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:

public class Parent 
{
public Parent() 
{
DoSomething();  // Noncompliant
}

public virtual void DoSomething() // can be overridden
{  
...
}
}

public class Child : Parent 
{
private string foo;

public Child(string foo) // leads to call DoSomething() in Parent constructor which triggers a NullReferenceException as foo has not yet been initialized
{
this.foo = foo;
}

public override void DoSomething() 
{
Console.WriteLine(this.foo.Length);
}
}

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.

void Sample(bool condition)
{
int? nullableValue = condition ? 42 : null;
Console.WriteLine(nullableValue.Value); // Noncompliant: InvalidOperationException is raised

int? nullableCast = condition ? 42 : null;
Console.WriteLine((int)nullableCast);   // Noncompliant: InvalidOperationException is raised
}

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.

public class Car 
{
private string _make;
public string Make // Noncompliant
{
get { return _make; }
set { _make = value; }
}
}

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.

public void DoSomething(object obj)
{
if (obj == null)
{
throw new NullReferenceException("obj");  // Noncompliant: This reserved exception should not be thrown manually
}
// ...
}

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

using System;

namespace MyLibrary
{
public class Foo // Noncompliant
{
private int left;
private int right;

public Foo(int l, int r)
{
  this.left = l;
  this.right = r;
}

public static Foo operator +(Foo a, Foo b)
{
  return new Foo(a.left + b.left, a.right + b.right);
}

public static Foo operator -(Foo a, Foo b)
{
  return new Foo(a.left - b.left, a.right - b.right);
}
}
}

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.

@for (var i = 1; i < 100; i++)
{
var buttonNumber = i;

<button @onclick="@(e => DoAction(e, buttonNumber))"> @* Noncompliant *@
    Button #@buttonNumber
</button>
}

@code {
private void DoAction(MouseEventArgs e, int button)
{
    // Do something here
}
}

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.

public interface IMyInterface
{
void MyMethod();
}

public class Foo : IMyInterface
{
void IMyInterface.MyMethod() // Noncompliant
{
    MyMethod();
}

void MyMethod()
{
    // Do something ...
}
}

public class Bar : Foo, IMyInterface
{
public void MyMethod()
{
    // Can't access base.MyMethod()
    // ((IMyInterface)this).MyMethod() would be a recursive call
}
}

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.

using System;
using System.IO;

namespace MyLibrary
{
public class Foo
{
public void ReadStream(FileStream stream) // Noncompliant: Uses only System.IO.Stream methods
{
  int a;
  while ((a = stream.ReadByte()) != -1)
  {
        // Do something.
  }
}
}
}

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.

class C
{
private int x;
private int y;
public int Y => x; // Noncompliant: The returned field should be 'y'
}

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

s = string.Format("[0}", arg0); // Noncompliant: square bracket '[' instead of curly bracket '{'
s = string.Format("{{0}", arg0); // Noncompliant: double starting curly brackets '{{'
s = string.Format("{0}}", arg0); // Noncompliant: double ending curly brackets '}}'
s = string.Format("{-1}", arg0); // Noncompliant: invalid index for the format item, must be >= 0
s = string.Format("{0} {1}", arg0); // Noncompliant: two format items in the string but only one argument provided

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.

public override void Method() // Noncompliant
{
base.Method(); 
}

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.

Environment.Exit(0);
Application.Exit();

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.

void DoTheThing(Toy toy) 
{
Ball ball = toy as Ball;
if (toy != null) // Noncompliant
{
//...
}
}

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.

using System;
using System.Collections.Generic;

namespace MyLibrary
{
public class Foo
{
public void DoSomething(ICollection<ICollection<int>> outerCollect) // Noncompliant
{
}
}
}

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.

<h3>Component</h3>

@code {
[Parameter]
[SupplyParameterFromQuery]  // Noncompliant
public bool Param { get; set; }
}

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

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

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.

public class TooManyCase
{
public int mapValues(char ch)
{
    switch(ch) {  // Noncompliant: 5 cases, "default" excluded, more than maximum = 4
        case 'a':
            return 1;
        case 'b':
        case 'c':
            return 2;
        case 'd':
            return 3;
        case 'e':
            return 4;
        case 'f':
        case 'g':
        case 'h':
            return 5;
        default:
            return 6;
    }
}
}

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.

public override string ToString()
{
if (string.IsNullOrEmpty(Name))
{
throw new ArgumentException(nameof(Name));  // Noncompliant
}
//...
}

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:

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:

data.All(x =>
{
x.Property = “foo”;
return true;
});

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.

data.Where(x => x > 5).Select(x => x * x); // Noncompliant
"this string".Equals("other string"); // Noncompliant

data.All(x =>  // Noncompliant
{
x.Property = "foo";
return true;
});

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.

var hashProvider1 = new MD5CryptoServiceProvider(); //Noncompliant
var hashProvider2 = (HashAlgorithm)CryptoConfig.CreateFromName("MD5"); //Noncompliant
var hashProvider3 = new SHA1Managed(); //Noncompliant
var hashProvider4 = HashAlgorithm.Create("SHA1"); //Noncompliant

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.

class A 
{
private List<string> names = new List<string>();

public ICollection<string> Names => names; // Noncompliant

public IEnumerable<string> GetNames()  // Noncompliant
{
    return names;
}

public void SetNames(List<string> strings) 
{
    this.names = strings;  // Noncompliant
}
}

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.

interface IPoint
{
int X { get; set; }
int Y { get; set; }
}

class PointManager<T1, T2> 
where T1 : IPoint
where T2 : IPoint
{
readonly T1 point1;  // this could be a struct
readonly T2 point2;  // this could be a struct

public PointManager(T1 point1, T2 point2)
{
    this.point1 = point1;
    this.point2 = point2;
}

public void MovePoints(int newX)
{
    point1.X = newX; //Noncompliant: if point is a struct, then nothing happened
    point2.X = newX; //Noncompliant: if point is a struct, then nothing happened
}
}

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.

int PickNumber()
{
int i = 0;
int j = 0;

i = i++;      // Noncompliant: i is still 0 
return j--;   // Noncompliant: returns 0 
}

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

using System;

namespace MyLibrary
{
public class Foo
{
public void Bar(ref object o1, ref object o2) // Noncompliant
{
}
}
}

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.

public class MyException : Exception // Noncompliant: several constructors are missing
{
public MyException()
{
}
}

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.

abstract class Fruit { }
class Apple : Fruit { }
class Orange : Fruit { }

class Program
{
static void Main(string[] args)
{
Fruit[] fruits = new Apple[1]; // Noncompliant - array covariance is used
FillWithOranges(fruits);
}

// Just looking at the code doesn't reveal anything suspicious
static void FillWithOranges(Fruit[] fruits)
{
for (int i = 0; i < fruits.Length; i++)
{
  fruits[i] = new Orange(); // Will throw an ArrayTypeMismatchException
}
}
}

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.

public string nameof(string s) { return s; }  // Noncompliant
...
public string Hello { get { return "World!"; } }
...
Console.WriteLine(nameof(Hello)); // prints "World!" instead of "Hello" as expected

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.

namespace Foo // Noncompliant, must be defined in the same assembly as the entity class that implements it
{
public interface ICounter<T> // Noncompliant, interfaces cannot contain generic parameters
{
    string Name { get; set; } // Noncompliant, interface must only define methods
    void Add(int amount, int secondParameter); // Noncompliant, methods must not have more than one parameter
    int Get(); // Noncompliant, methods must return void, Task, or Task<T>
}
}

namespace Bar
{
public class Counter : ICounter
{
    // do stuff
}

public static class AddToCounterFromQueue
{
    [FunctionName("AddToCounterFromQueue")]
    public static Task Run(
        [QueueTrigger("durable-function-trigger")] string input,
        [DurableClient] IDurableEntityClient client)
    {
        var entityId = new EntityId("Counter", "myCounter");
        int amount = int.Parse(input);
        return client.SignalEntityAsync<ICounter>(entityId, proxy => proxy.Add(amount, 10));
    }
}
}

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.

public interface IAggregate: IComparable, IFormattable { } // Compliant: Aggregates two interfaces

When you call Any(), it clearly communicates the code’s intention, which is to check if the collection is empty. Using `Count()

private static bool HasContent(IEnumerable<string> strings)
{
return strings.Count() > 0;  // Noncompliant
}

private static bool HasContent2(IEnumerable<string> strings)
{
return strings.Count() >= 1;  // Noncompliant
}

private static bool IsEmpty(IEnumerable<string> strings)
{
return strings.Count() == 0;  // Noncompliant
}

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.

private void DoSomething()
{
object local = new object();
// Code potentially modifying the local variable ...

lock (local) // Noncompliant
{
// ...
}
}

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.

namespace MyLibrary
{
public class Foo
{
    // ...
}

public static class MyExtensions
{
    public static void Bar(this Foo a) // Noncompliant
    {
        // ...
    }
}
}

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.

// Saturday = 0b00100000, Sunday = 0b01000000, weekend = 0b01100000
var weekend = Days.Saturday | Days.Sunday;  // Combining elements

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.

Another case is using set-like operations. For example, using Union between a list and itself will always return the same list. Conversely, using Except between a list and itself will always return an empty list.

var list = new List<int>();

list.AddRange(list);          // Noncompliant
list.Concat(list);            // Noncompliant

list.Union(list);             // Noncompliant: always returns list
list.Intersect(list);         // Noncompliant: always returns list
list.Except(list);            // Noncompliant: always returns empty
list.SequenceEqual(list);     // Noncompliant: always returns true

var set = new HashSet<int>();
set.UnionWith(set);           // Noncompliant: no changes
set.IntersectWith(set);       // Noncompliant: no changes
set.ExceptWith(set);          // Noncompliant: always returns empty
set.SymmetricExceptWith(set); // Noncompliant: always returns empty
set.IsProperSubsetOf(set);    // Noncompliant: always returns false
set.IsProperSupersetOf(set);  // Noncompliant: always returns false
set.IsSubsetOf(set);          // Noncompliant: always returns true
set.IsSupersetOf(set);        // Noncompliant: always returns true
set.Overlaps(set);            // Noncompliant: always returns true
set.SetEquals(set);           // Noncompliant: always returns true

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.

public class GarbageDisposal : IDisposable
{
protected virtual void Dispose(bool disposing)
{
//...
}
public void Dispose() 
{
Dispose(true);
GC.SuppressFinalize(this);
}
}

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.

using C;  // Noncompliant
using System.A.A;
using A;
using D;
using B;
using System;
using System.A;
using System.B;

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.

private int count;
public int Count
{
get { return count; }
set { count = 42; } // Noncompliant 
}

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.

int await = 42; // Noncompliant, but compiles
int async = 42; // Noncompliant, but compiles

Events that are not invoked anywhere are dead code, and there’s no good reason to keep them in the source.

class UninvokedEventSample
{
private event Action<object, EventArgs> Happened; //Noncompliant

public void RegisterEventHandler(Action<object, EventArgs> handler)
{
    Happened += handler; //we register some event handlers
}

public void RaiseEvent()
{
    if (Happened != null)
    {
        // Happened(this, null); // the event is never triggered, because this line is commented out.
    }
}
}

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.

public void DoSomething(string fileName) 
{
using (var stream = File.Open(fileName, FileMode.Open))
{
var result = new byte[stream.Length];
stream.Read(result, 0, (int)stream.Length); // Noncompliant
// ... do something with result
}
}

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.

public unsafe int SubarraySum(int[] array, int start, int end)  // Sensitive
{
var sum = 0;

// Skip array bound checks for extra performance
fixed (int* firstNumber = array)
{
    for (int i = start; i < end; i++)
        sum += *(firstNumber + i);
}

return sum;
}

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.

class MyClass
{
private int field; // Noncompliant, shouldn't it be initialized? This way the value is always default(int), 0.
private int Property { get; set; }  // Noncompliant
public void Print()
{
Console.WriteLine(field); //Will always print 0
Console.WriteLine(Property); //Will always print 0
}
}

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.

public interface IBase1
{
string SomeProperty { get; set; }
}

public interface IBase2
{
string SomeProperty { get; set; }
}

public interface IDerived : IBase1, IBase2 // Noncompliant, accessing IDerived.SomeProperty is ambiguous
{
}

public class MyClass : IDerived
{
// Implements both IBase1.SomeProperty and IBase2.SomeProperty
public string SomeProperty { get; set; } = "Hello";

public static void Main()
{
MyClass myClass = new MyClass();
Console.WriteLine(myClass.SomeProperty); // Writes "Hello" as expected
Console.WriteLine(((IBase1)myClass).SomeProperty); // Writes "Hello" as expected
Console.WriteLine(((IBase2)myClass).SomeProperty); // Writes "Hello" as expected
Console.WriteLine(((IDerived)myClass).SomeProperty); // Error CS0229 Ambiguity between 'IBase1.SomeProperty' and 'IBase2.SomeProperty'
}
}

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.

class MyClass
{
public void DoStuff([Optional]int i, int j)  // Noncompliant
{
// ...
}
}

Methods and properties that don’t access instance data can be static to prevent any misunderstanding about the contract of the method.

public class Utilities
{
public int MagicNum // Noncompliant
{
    get 
    {
        return 42;
    }
}

private static string magicWord = "please";
public string MagicWord  // Noncompliant
{
    get 
    {
        return magicWord;
    }
    set 
    {
        magicWord = value;
    }
}

public int Sum(int a, int b)  // Noncompliant
{
    return a + b;
}
}

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.

class Person 
{
int Age { get; }  // Noncompliant; will always be 0.

Person () {}
}

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

public void Foo(Bar a, int[] b)
{
throw new ArgumentException();                                        // Noncompliant
throw new ArgumentException("My error message", "c");                 // Noncompliant
throw new ArgumentException("My error message", "c", innerException); // Noncompliant

throw new ArgumentNullException("c");                     // Noncompliant
throw new ArgumentNullException(nameof(c));               // Noncompliant
throw new ArgumentNullException("My error message", "a"); // Noncompliant

throw new ArgumentOutOfRangeException("c");                           // Noncompliant
throw new ArgumentOutOfRangeException("c", "My error message");       // Noncompliant
throw new ArgumentOutOfRangeException("c", b, "My error message");    // Noncompliant

throw new DuplicateWaitObjectException("c", "My error message");      // Noncompliant
}

In C#, without constraints on a generic type parameter, both reference and value types can be passed. However, comparing this type parameter to null can be misleading as value types, like struct, can never be null.

bool IsDefault<T>(T value)
{
if (value == null) // Noncompliant
{
// ...
}
}

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.

public class MyClass
{
private MyOtherClass other;

public void Foo(MyOtherClass other)
{
    this.other = other.Clone(); // Noncompliant
}

protected void Bar(MyOtherClass other)
{
    this.other = other.Clone(); // Noncompliant
}
}

The default clause should take appropriate action. Having an empty default is a waste of keystrokes.

enum Fruit
{
Apple,
Orange,
Banana
}

void PrintName(Fruit fruit)
{
switch(fruit)
{
case Fruit.Apple:
  Console.WriteLine("apple");
  break;
default:  //Noncompliant
  break;
}
}

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:

public void Method(ILogger logger, bool parameter)
{
logger.DebugFormat($"The value of the parameter is: {parameter}.");
}

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.

if (x != null && x is MyClass) { ... }  // Noncompliant

if (x == null || !(x is MyClass)) { ... } // Noncompliant

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

public void MyMethod([DefaultParameterValue(5)] int j) //Noncompliant, useless
{
Console.WriteLine(j);
}

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.

string str = "some string";
foreach (var c in str.ToCharArray()) // Noncompliant
{
// ...    
}

ReadOnlySpan<byte> span = "some UTF-8 string literal"u8;
foreach (var c in span.ToArray()) // Noncompliant
{
// ...    
}

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.

public class Fruit 
{
public double GetCost() 
{
return 3.5;
} 
}

public class Raspberry : Fruit 
{
public new double GetCost()  // Noncompliant
{ 
return 7.5;
}
}

// ...
var r = new Raspberry();
var f = (Fruit) r;
Console.WriteLine(r.GetCost());  // prints 7.5
Console.WriteLine(f.GetCost());  // prints 3.5; there's only one instance but different code executes depending on cast

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.

AesManaged aes = new AesManaged
{
KeySize = 128,
BlockSize = 128,
Mode = CipherMode.OFB, // Noncompliant
Padding = PaddingMode.PKCS7
};

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

sealed class MyClass
{
public void Method()
{
...
GC.SuppressFinalize(this); //Noncompliant
}
}

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.

class RequestParser
{
[FunctionName(nameof(ParseRequest))]
public static async Task<IActionResult> ParseRequest([HttpTrigger(AuthorizationLevel.Function, "get", "post", Route = null)] HttpRequest req)
{
	// This can lead to thread exhaustion
	string requestBody = new StreamReader(req.Body).ReadToEndAsync().Result;
	// do stuff...
}
}

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.

public class ResourceHolder : IDisposable
{
private FileStream fs;  
public void OpenResource(string path)
{
this.fs = new FileStream(path, FileMode.Open);
}
public void CloseResource()
{
this.fs.Close();
}

public void CleanUp() 
{
this.fs.Dispose(); // Noncompliant; Dispose not called in class' Dispose method
}

public void Dispose() 
{
// method added to satisfy demands of interface
}
}

This rule allows you to track the usage of the SuppressMessage attributes and #pragma warning disable mechanism.

[SuppressMessage("", "S100")]
...

#pragma warning disable S100
...
#pragma warning restore S100

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

class MyClass 
{
private static int count = 0;

public void DoSomething() 
{
//...
count++;  // Noncompliant: make the enclosing instance property 'static' or remove this set on the 'static' field.
}
}

interface MyInterface
{
private static int count = 0;

public void DoSomething() 
{
//...
count++;  // Noncompliant: remove this set, which updates a 'static' field from an instance method.
}
}

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.

private int myVar;

public int MyProperty
{
get { return myVar; }
set { myVar = value; }
}

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.

public class HttpExample
{
    [FunctionName("HttpExample")]
    public async Task<IActionResult> Run([HttpTrigger(AuthorizationLevel.Anonymous, "get", Route = null)] HttpRequest request)
    {
        HttpClient httpClient = new HttpClient(); // Noncompliant

        var response = await httpClient.GetAsync("https://example.com");
        // rest of the function
    }
}

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

var lowered = someString.ToLower(); //Noncompliant

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.

[ServiceContract]
interface IMyService // Noncompliant
{
int MyServiceMethod();
}

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`

using System;
using System.Runtime.InteropServices;

namespace MyLibrary
{
public class Foo : IDisposable // Noncompliant: Doesn't have a finalizer
{
private IntPtr myResource;
private bool disposed = false;

protected virtual void Dispose(bool disposing) 
{
  if (!disposed) 
  {
    // Dispose of resources held by this instance.
    FreeResource(myResource);
    disposed = true;

    // Suppress finalization of this disposed instance.
    if (disposing)
    {
      GC.SuppressFinalize(this);
    }
  }
}

public void Dispose() {
  Dispose(true);
}
}
}

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.

using System;
using  System.Threading.Tasks;

namespace myLibrary
{

public class Foo
{
public Task Read(byte [] buffer, int offset, int count, // Noncompliant
                            CancellationToken cancellationToken)
}
}

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:

var pattern = "{0} {1} {2}";
var res = string.Format(pattern, 1, 2); // Incorrect, but the analyzer doesn't raise any warnings here

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.

public class Car
{

[Obsolete("Replaced by the automatic starter")]
public void CrankEngine(int turnsOfCrank) 
{ ... }
}

public class R2 : Car
{

public void CrankEngine(int turnsOfCrank)   // Noncompliant
{ ... }

...
}

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

string str = "";
for (int i = 0; i < arrayOfStrings.Length ; ++i) 
{
str = str + arrayOfStrings[i];
}

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.

public static bool operator ==(MyType x, MyType y) // Noncompliant: confusing for the caller
{
// custom implementation
}

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.

using System;

namespace MyLibrary
{
public class MyClass
{
public IntPtr myPointer;  // Noncompliant
protected UIntPtr myOtherPointer; // Noncompliant
}
}

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.

public int Transform(int value)
{
if (value <= 0)
{
    return value;
}
int number = int.MaxValue;
return number + value;  // Noncompliant
}

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.

using System;

namespace MyLibrary
{
public class Text   // Noncompliant: Collides with System.Text
{
}
}

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.

public void Method()
{
var zero_length = new int[0]; // Noncompliant
var empty_array = new string[] { }; // Noncompliant
}

Overriding methods automatically inherit the params behavior. To ease readability, this modifier should be explicitly used in the overriding method as well.

class Base
{
public virtual void Method(params int[] numbers)
{
...
}
}
class Derived : Base
{
public override void Method(int[] numbers) // Noncompliant, the params is missing.
{
...
}
}

To check the type of an object there are several options:

expr is SomeType or ++expr.GetType()

class Fruit { }
sealed class Apple : Fruit { }

class Program
{
static void Main()
{
var apple = new Apple();
var b = apple != null && apple.GetType() == typeof (Apple); // Noncompliant
b = typeof(Apple).IsInstanceOfType(apple); // Noncompliant
if (apple != null)
{
  b = typeof(Apple).IsAssignableFrom(apple.GetType()); // Noncompliant
}
var appleType = typeof (Apple);
if (apple != null)
{
  b = appleType.IsAssignableFrom(apple.GetType()); // Noncompliant
}

Fruit f = apple;
if (f as Apple != null) // Noncompliant
{
}
if (apple is Apple) // Noncompliant
{
}
}
}

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.

public Result[] GetResults()
{
return null; // Noncompliant
}

public IEnumerable<Result> GetResults(bool condition)
{
var results = GenerateResults();
return condition 
    ? results 
    : null; // Noncompliant
}

public IEnumerable<Result> GetResults() => null; // Noncompliant

public IEnumerable<Result> Results 
{
get
{
    return default(IEnumerable<Result>); // Noncompliant
}
}

public IEnumerable<Result> Results => default; // Noncompliant

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.

var X = 5;

var anon = new 
{
X = X, //Noncompliant, the new object would have the same property without the "X =" part.
Y = "my string"
};

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.

using System;

namespace MyLibrary
{
[Flags]
public enum Color // Noncompliant, Orange is neither a power of two, nor a combination of any of the defined values
{
    None    = 0,
    Red     = 1,
    Orange  = 3,
    Yellow  = 4
}
}

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.

int foo = 42;
switch (foo) // Noncompliant
{
case 0:
Console.WriteLine("foo = 0");
break;
case 42:
Console.WriteLine("foo = 42");
break;
}

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:

if (condition)  // Noncompliant
DoTheThing();
DoTheOtherThing(); // Was the intent to call this function unconditionally?

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.

int sum = 0;
for (int i = 0; i < 10; sum++) // Noncompliant: `i` is not updated in the increment section
{
// ...
i++;
}

NUnit TestFixtures may only have one [SetUp] method. Any more than that and the TestFixture will compile but not run.

namespace NUnit.Tests
{
using System;
using NUnit.Framework;

[TestFixture]
public class MyTests
{
[SetUp] public void Init()
{ /* ... */ }

[Setup] public void Prep()  // Noncompliant
{ /* ... */ }
}
}

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.

[TestMethod]
[Ignore]
public void Test_DoTheThing() 
{ 
// ...
}

The rule targets test methods that lack an assertion and consist solely of an action and, optionally, a setup.

[TestMethod]
public void Add_SingleNumber_ReturnsSameNumber()
{
var stringCalculator = new StringCalculator();
var actual = stringCalculator.Add("0");
}

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

using System;

class MyEventProcucer
{
public static event EventHandler eventFired;
}

public class MyEventSubscriber : IDisposable
{

public MyEventSubscriber()
{
    MyEventProcucer.eventFired += c_EventFired;  // Noncompliant.
}

static void c_EventFired(object sender, EventArgs e)
{}

public void Dispose()
{}
}

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.

partial class C
{
partial void M(); //Noncompliant

void OtherM()
{
M(); //Noncompliant. Will be removed.
}
}

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.

[Serializable]                                                                                 // 1.
public class SerializationOptIn_Attribute
{
}

public class SerializationOptIn_Interface : ISerializable                                      // 2.
{
}

public class SerializationOptIn_Constructor
{
protected SerializationOptIn_Constructor(SerializationInfo info, StreamingContext context) // 3.
{ 
}
}

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

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

public class Foo
{
public string HelloTime() =>
    $"Hello at {DateTime.UtcNow}";
}

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

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

class MyClass
{
private object obj = new object();

public void DoSomethingWithMonitor()
{
Monitor.Enter(obj); // Noncompliant: not all paths release the lock
if (IsInitialized())
{
  // ...
  Monitor.Exit(obj);
}
}
}

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

if ( !(a == 2)) { ...}  // Noncompliant
bool b = !(i < 10);  // Noncompliant

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.

void Regexes(string input)
{
var regex = new Regex("[A");                                                    // Noncompliant
var match = Regex.Match(input, "[A");                                           // Noncompliant
var negativeLookahead = new Regex("a(?!b)", RegexOptions.NonBacktracking);      // Noncompliant
var negativeLookbehind = new Regex("(?<!a)b", RegexOptions.NonBacktracking);    // Noncompliant
}

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`

var randomPath = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName());

// Creates a new file with write, non inheritable permissions which is deleted on close.
using var fileStream = new FileStream(randomPath, FileMode.CreateNew, FileAccess.Write, FileShare.None, 4096, FileOptions.DeleteOnClose);
using var writer = new StreamWriter(fileStream);

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

public class Monument
{
public static readonly List<Monument> ALL_MONUMENTS = new List<Monument>();
// ...

public Monument(string location, ...)
{
ALL_MONUMENTS.Add(this);  // Noncompliant; passed to a method of another object 

this.location = location;
// ...
}
}

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.

if(condition) 
{
doSomething();}

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.

[TestClass]
public class Foo // Noncompliant
{

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

Func<object, bool> item1 = o => { return true; }; // Compliant by exception
Func<object, bool> item1 = o => { var r = false; return r; }; // Noncompliant

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.

if (((condition1 && condition2) || (condition3 && condition4)) && condition5) { ... }

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.

public void ConfigureServices(IServiceCollection services)
{
// ...
services.AddControllersWithViews(options => options.Filters.Add(new IgnoreAntiforgeryTokenAttribute())); // 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.

Process p = new Process();
p.StartInfo.FileName = @"C:\Apps\binary.exe"; // Compliant

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.

String origin = Request.Headers["Origin"];
Response.Headers.Add("Access-Control-Allow-Origin", origin); // Sensitive

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.

public int Foo
{
get
{
    throw new Exception(); // Noncompliant
}
}

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 cases simply clutter the code, and should be removed.

switch(ch) 
{
case 'a' :
HandleA();
break;
case 'b' :
HandleB();
break;
case 'c' :  // Noncompliant
default:
HandleTheRest();
break;
}

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.

public long ComputeDurationInMilliseconds()
{
long duration = (((hours * 60) + minutes) * 60 + seconds ) * 1000 ;
return duration;
}

public void DoSomething()
{
ApplicationException myException = new ApplicationException();
throw myException;
}

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

[ServiceContract]
interface IMyService 
{
[OperationContract(IsOneWay = true)]
int SomethingHappened(int parameter); // Noncompliant
}

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.

string text = "";
try
{
text = File.ReadAllText(fileName);
} catch {  // Noncompliant
// ...
}

try
{
text = File.ReadAllText(fileName);
} catch (Exception exc) {  // Noncompliant
// ...
}

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.

using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Hosting;

namespace mvcApp
{
public class Startup2
{
    public void Configure(IApplicationBuilder app, IHostingEnvironment env)
    {
        // Those calls are Sensitive because it seems that they will run in production
        app.UseDeveloperExceptionPage(); // Sensitive
        app.UseDatabaseErrorPage(); // Sensitive
    }
}
}

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.

public class Person 
{
private static DateTime dateOfBirth;
private static int expectedFingers;

public Person(DateTime birthday) 
{
dateOfBirth = birthday;  // Noncompliant; now everyone has this birthday
expectedFingers = 10;  // Noncompliant
}
}

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.

public class MyClass
{
private void DoSomething()
{
    // ...
    Console.WriteLine("My Message"); // Noncompliant
    // ...
}
}

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.

public void DoSomething()
{
for (int i = 0; i < 4; i++)  // Noncompliant, 4 is a magic number
{
    ...
}
}

A boolean literal can be represented in two different ways: {true} or {false}. They can be combined with logical operators ({ops}) to produce logical expressions that represent truth values. However, comparing a boolean literal to a variable or expression that evaluates to a boolean value is unnecessary and can make the code harder to read and understand. The more complex a boolean expression is, the harder it will be for developers to understand its meaning and expected behavior, and it will favour the introduction of new bugs.

if (booleanMethod() == true) { /* ... */ }
if (booleanMethod() == false) { /* ... */ }
if (booleanMethod() || false) { /* ... */ }
doSomething(!false);
doSomething(booleanMethod() == true);

booleanVariable = booleanMethod() ? true : false;
booleanVariable = booleanMethod() ? true : exp;
booleanVariable = booleanMethod() ? false : exp;
booleanVariable = booleanMethod() ? exp : true;
booleanVariable = booleanMethod() ? exp : false;

for (var x = 0; true; x++)
{
...
}

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.

class Program
{
public int Foo  //Non-Compliant
{
    set
    {
        // ... some code ...
    }
}
}

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.

using System.Security.Cryptography;
...
var randomGenerator = RandomNumberGenerator.Create(); // Compliant for security-sensitive use cases
byte[] data = new byte[16];
randomGenerator.GetBytes(data);
return BitConverter.ToString(data);

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

public class BaseClass
{
public virtual void MyMethod(int i = 1)
{ 
    Console.WriteLine(i);
}
}

public class DerivedClass : BaseClass
{
public override void MyMethod(int i = 1)
{
    // ... 
    base.MyMethod(); // Noncompliant: caller's value is ignored
}

static int Main(string[] args) 
{
    DerivedClass dc = new DerivedClass();
    dc.MyMethod(12);  // prints 1
}
}

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.

public class Foo
{
public int MagicNumber = 42; 
}

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.

public class StringUtils // Noncompliant: implicit public constructor
{ 
public static string Concatenate(string s1, string s2) 
{
return s1 + s2;
}
}

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

int THRESHOLD_ENTRIES = 10000;        
int THRESHOLD_SIZE = 1000000000; // 1 GB
double THRESHOLD_RATIO = 10;         
int totalSizeArchive = 0;
int totalEntryArchive = 0;

using var zipToOpen = new FileStream(@"ZipBomb.zip", FileMode.Open);
using var archive = new ZipArchive(zipToOpen, ZipArchiveMode.Read);
foreach (ZipArchiveEntry entry in archive.Entries)
{
totalEntryArchive ++;

using (Stream st = entry.Open())
{
byte[] buffer = new byte[1024];
int totalSizeEntry = 0;
int numBytesRead = 0;

do
{
  numBytesRead = st.Read(buffer, 0, 1024);
  totalSizeEntry += numBytesRead;
  totalSizeArchive += numBytesRead;
  double compressionRatio = totalSizeEntry / entry.CompressedLength;          

  if(compressionRatio > THRESHOLD_RATIO) {
    // ratio between compressed and uncompressed data is highly suspicious, looks like a Zip Bomb Attack
    break;
  }
} 
while (numBytesRead > 0);
}

if(totalSizeArchive > THRESHOLD_SIZE) {
  // the uncompressed data size is too much for the application resource capacity
  break;
}

if(totalEntryArchive > THRESHOLD_ENTRIES) {
  // too much entries in this archive, can lead to inodes exhaustion of the system
  break;
} 
}

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.

if (a == b)
{
doTheThing(b);
}
if (a == b) // Noncompliant; is this really what was intended?
{
doTheThing(c);
}

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

public class Fruit
{
protected string plantingSeason;
//...
}

public class Raspberry : Fruit
{
protected string plantingseason;  // Noncompliant
// ...
}

Nested code - blocks of code inside blocks of code - is eventually necessary, but increases complexity. This is why keeping the code as flat as possible, by avoiding unnecessary nesting, is considered a good practice.

Merging if statements when possible will decrease the nesting of the code and improve its readability.

if (condition1)
{
if (condition2)           // Noncompliant
{        
    // ...
}
}

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.

public void ShouldNotBeEmpty() {  // Noncompliant - method is empty
}

public void NotImplementedYet() {  // Noncompliant - method is empty
}

public void WillNeverBeImplemented() {  // Noncompliant - method is empty
}

public void EmptyOnPurpose() {  // Noncompliant - method is empty
}

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.

for (int i = 0; i < maximum; i--)  // Noncompliant: runs until it underflows to int.MaxValue
{
// ...
}

for (int i = maximum; i >= maximum; i++)  // Noncompliant: runs until it overflows to int.MinValue
{
// ...
}

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:

var urlHttp = "http://example.com";                 // Noncompliant
var urlFtp = "ftp://anonymous@example.com";         // Noncompliant
var urlTelnet = "telnet://anonymous@example.com";   // Noncompliant

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.

HttpCookie myCookie = new HttpCookie("Sensitive cookie");
myCookie.HttpOnly = true; // Compliant: the sensitive cookie is protected against theft thanks to the HttpOnly property set to true (HttpOnly = true)

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.

public int Example(int i) 
{
return (int) (i + 42); // Noncompliant
}

public IEnumerable<int> ExampleCollection(IEnumerable<int> coll) 
{
return coll.Reverse().OfType<int>(); // Noncompliant
}

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.

public void CompliantExample() {
String cmd="/usr/bin/file.exe";
var startInfo = new ProcessStartInfo();
startInfo.FileName = cmd; // Compliant
}

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.

if (str == null && str.Length == 0)
{
Console.WriteLine("String is empty");
}

if (str != null || str.Length > 0)
{
Console.WriteLine("String is not empty");
}

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.

using Microsoft.AspNetCore.Mvc;

public class MyController : Controller
{
[HttpPost]
[DisableRequestSizeLimit] // Sensitive: No size  limit
[RequestSizeLimit(10485760)] // Sensitive: 10485760 B = 10240 KB = 10 MB is more than the recommended limit of 8MB
public IActionResult PostRequest(Model model)
{
// ...
}

[HttpPost]
[RequestFormLimits(MultipartBodyLengthLimit = 10485760)] // Sensitive: 10485760 B = 10240 KB = 10 MB is more than the recommended limit of 8MB
public IActionResult MultipartFormRequest(Model model)
{
// ...
}
}

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.

var result = Foo(() =>
{
int x = 100; // dead store, but ignored
x = 200;
return x;
}

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

public void Foo(DbContext context, string query, string param)
{
context.Database.ExecuteSqlCommand("SELECT * FROM mytable WHERE mycol=@p0", param); // Compliant, it's a parametrized safe query
}

A typical code smell known as unused function parameters refers to parameters declared in a function but not used anywhere within the function’s body. While this might seem harmless at first glance, it can lead to confusion and potential errors in your code. Disregarding the values passed to such parameters, the function’s behavior will be the same, but the programmer’s intention won’t be clearly expressed anymore. Therefore, removing function parameters that are not being utilized is considered best practice.

private void DoSomething(int a, int b) // Noncompliant, "b" is unused
{
Compute(a);
}

private void DoSomething2(int a) // Noncompliant, the value of "a" is unused
{
a = 10;
Compute(a);
}

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.

public class Foo
{
public int InstanceData = 32; // Noncompliant
public int AnotherInstanceData = 32; // Noncompliant

}

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.

using System;

namespace MyLibrary  // Noncompliant
{
}

break; is an unstructured control flow statement which makes code harder to read.

Ideally, every loop should have a single termination condition.

int i = 0;
while (true)
{
if (i == 10)
{
break;      // Non-Compliant
}

Console.WriteLine(i);
i++;
}

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.

[Serializable]
public class InternalUrl : ISerializable
{
private string url;

public InternalUrl(string tmpUrl)
{
    if(!tmpUrl.StartsWith("http://localhost/")) // there is some input validation
    {
        url= "http://localhost/default";
    }
    else 
    {
        url= tmpUrl;
    }
}

// special constructor used during deserialization
protected InternalUrl(SerializationInfo info, StreamingContext context)
{
   string tmpUrl= (string) info.GetValue("url", typeof(string));

   if(!tmpUrl.StartsWith("http://localhost/") { // Compliant
      url= "http://localhost/default";
   }
   else {
      url= tmpUrl;
   }
 }

void ISerializable.GetObjectData(SerializationInfo info, StreamingContext context)
{
    info.AddValue("url", url);
}
}

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.

using System.Diagnostics;
Process p = new Process();
p.StartInfo.FileName = "/usr/bin/find";
if (allowed.Contains(input)) {
p.StartInfo.ArgumentList.Add(input);
}

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.

public class Foo {
public List<User> ListUsers() {
string userListPath = "/home/mylogin/Dev/users.txt"; // Noncompliant
return ParseUsers(userListPath);
}
}

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.

public void DoTheThing(string str, int i, List<string> strings)
{
str = i.ToString(i);  // Noncompliant

foreach (var s in strings)
{
s = "hello world";  // Noncompliant
}
}

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.

if (x == 0) 
{
DoSomething();
} 
else if (x == 1) 
{
DoSomethingElse();
}

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.

for (int i = 1; i <= 5; i++) 
{
Console.WriteLine(i);
if (condition) 
{
    i = 20;
}
}

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

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

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

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

[Serializable]
public class Foo
{
[OnSerializing]
public void OnSerializing(StreamingContext context) {} // Noncompliant: should be private

[OnSerialized]
int OnSerialized(StreamingContext context) {} // Noncompliant: should return void

[OnDeserializing]
void OnDeserializing() {} // Noncompliant: should have a single parameter of type StreamingContext

[OnSerializing]
public void OnSerializing2<T>(StreamingContext context) {} // Noncompliant: should have no type parameters

[OnDeserialized]
void OnDeserialized(StreamingContext context, string str) {} // Noncompliant: should have a single parameter of type StreamingContext
}

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.

using System;

[assembly:CLSCompliant(true)]
namespace MyLibrary
{
}

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

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

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

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

public class Foo
{
public const double Version = 1.0;           // Noncompliant
}

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.

string value = Request.QueryString["value"];
Response.AddHeader("X-Header", value); // Noncompliant

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.

HttpCookie myCookie = new HttpCookie("Sensitive cookie");
myCookie.Secure = true; // Compliant

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.

public class Foo    // Noncompliant - Foo depends on too many classes: T1, T2, T3, T4, T5, T6 and T7
{
private T1 a1;    // Foo is coupled to T1
private T2 a2;    // Foo is coupled to T2
private T3 a3;    // Foo is coupled to T3

public T4 Compute(T5 a, T6 b)    // Foo is coupled to T4, T5 and T6
{
T7 result = a.Process(b);    // Foo is coupled to T7
return result;
}

public static class Bar    // Compliant - Bar depends on 2 classes: T8 and T9
{
public T8 a8;
public T9 a9;
}
}

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

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

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

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

public string GetReadableStatus(Job j)
{
return j.IsRunning ? "Running" : j.HasErrors ? "Failed" : "Succeeded";  // Noncompliant
}

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

public class FruitException // Noncompliant - this has nothing to do with Exception
{
private Fruit expected;
private string unusualCharacteristics;
private bool appropriateForCommercialExploitation;
// ...
}

public class CarException // Noncompliant - does not derive from any Exception-based class
{
public CarException(string message, Exception inner) 
{
 // ...
}
}

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

interface IMyInterface 
{
int DoTheThing(); // Noncompliant - overloaded method declarations are not grouped together
string DoTheOtherThing();
int DoTheThing(string s);
}

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.

if (condition) // Noncompliant
ExecuteSomething();
CheckSomething();

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

public class Foo
{
private string name = "foobar"; // Noncompliant

public string DefaultName { get; } = "foobar"; // Noncompliant

public Foo(string value = "foobar") // Noncompliant
{
    var something = value ?? "foobar"; // Noncompliant
}
}

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

public class Foo // Noncompliant
{
}

public struct Bar // Noncompliant
{
}

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.

using System.Reflection;

Type dynClass = Type.GetType("MyInternalClass");
// Noncompliant. Using BindingFlags.NonPublic will return non-public members
BindingFlags bindingAttr = BindingFlags.NonPublic | BindingFlags.Static;
MethodInfo dynMethod = dynClass.GetMethod("mymethod", bindingAttr);
object result = dynMethod.Invoke(dynClass, null);

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.

public class Foo
{
private int singularField;

public void DoSomething(int x)
{
singularField = x + 5;

if (singularField == 0) { /* ... */ }
}
}

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.

[DllImport("ole32.dll")]
static extern int CoSetProxyBlanket([MarshalAs(UnmanagedType.IUnknown)]object pProxy, uint dwAuthnSvc, uint dwAuthzSvc,
[MarshalAs(UnmanagedType.LPWStr)] string pServerPrincName, uint dwAuthnLevel, uint dwImpLevel, IntPtr pAuthInfo,
uint dwCapabilities);

public enum RpcAuthnLevel
{
Default = 0,
None = 1,
Connect = 2,
Call = 3,
Pkt = 4,
PktIntegrity = 5,
PktPrivacy = 6
}

public enum RpcImpLevel
{
Default = 0,
Anonymous = 1,
Identify = 2,
Impersonate = 3,
Delegate = 4
}

public enum EoAuthnCap
{
None = 0x00,
MutualAuth = 0x01,
StaticCloaking = 0x20,
DynamicCloaking = 0x40,
AnyAuthority = 0x80,
MakeFullSIC = 0x100,
Default = 0x800,
SecureRefs = 0x02,
AccessControl = 0x04,
AppID = 0x08,
Dynamic = 0x10,
RequireFullSIC = 0x200,
AutoImpersonate = 0x400,
NoCustomMarshal = 0x2000,
DisableAAA = 0x1000
}

[DllImport("ole32.dll")]
public static extern int CoInitializeSecurity(IntPtr pVoid, int cAuthSvc, IntPtr asAuthSvc, IntPtr pReserved1,
RpcAuthnLevel level, RpcImpLevel impers, IntPtr pAuthList, EoAuthnCap dwCapabilities, IntPtr pReserved3);

static void Main(string[] args)
{
var hres1 = CoSetProxyBlanket(null, 0, 0, null, 0, 0, IntPtr.Zero, 0); // Noncompliant

var hres2 = CoInitializeSecurity(IntPtr.Zero, -1, IntPtr.Zero, IntPtr.Zero, RpcAuthnLevel.None,
	RpcImpLevel.Impersonate, IntPtr.Zero, EoAuthnCap.None, IntPtr.Zero); // Noncompliant
}

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

Ask Yourself Whether

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

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

Recommended Secure Coding Practices

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

public void RegexPattern(string input)
{
var emailPattern = new Regex(".+@.+", RegexOptions.None);
var isNumber = Regex.IsMatch(input, "[0-9]+");
var isLetterA = Regex.IsMatch(input, "(a+)+");
}

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.

public bool IsOdd(int x) 
{
return x % 2 == 1;  // Noncompliant; if x is an odd negative, x % 2 == -1
}

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.

using (var tripleDES = new TripleDESCryptoServiceProvider()) //Noncompliant
{
//...
}

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

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

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

[ValidateInput(true)]
public ActionResult Welcome(string name)
{
...
}

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?

void Notify(string company, string office = "QJZ") // Noncompliant
{
}

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.

using System.ComponentModel.Composition;

[PartCreationPolicy(CreationPolicy.Any)] // Noncompliant
public class FooBar : IFooBar { }

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

public class BaseClass
{
public BaseClass(int param1)
{
    // ...
}
}

public class DerivedClass : BaseClass
{
public DerivedClass(int param1, int param2, int param3, string param4, long param5) : base(param1) // Compliant by exception
{
    // ...
}
}

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.

switch (myVariable) 
{
case 0: // Noncompliant: 9 statements in the case
    methodCall1("");
    methodCall2("");
    methodCall3("");
    methodCall4("");
    methodCall5("");
    methodCall6("");
    methodCall7("");
    methodCall8("");
    methodCall9("");
    break;
case 1:
    ...
}

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

private const string CODE = "secret";
private int callCount = 0;

public string GetCode() 
{
callCount++;
return CODE;
}

public string GetName() // Noncompliant: duplicates GetCode
{
callCount++;
return CODE;
}

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.

var ip = ConfigurationManager.AppSettings["myapplication.ip"];
var address = IPAddress.Parse(ip);

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

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

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

public void Foo()
{
var g1 = new Guid();    // Noncompliant - what's the intent?
Guid g2 = new();        // Noncompliant
var g3 = default(Guid); // Noncompliant
Guid g4 = default;      // Noncompliant
}

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.

private void DoSomething() 
{
// TODO
}

A message template must conform to the specification. The rule raises an issue if the template string violates the template string grammar.

logger.LogError("Login failed for {User", user);       // Noncompliant: Syntactically incorrect
logger.LogError("Login failed for {}", user);          // Noncompliant: Empty placeholder
logger.LogError("Login failed for {User-Name}", user); // Noncompliant: Only letters, numbers, and underscore are allowed for placeholders
logger.LogDebug("Retry attempt {Cnt,r}", cnt);         // Noncompliant: The alignment specifier must be numeric
logger.LogDebug("Retry attempt {Cnt:}", cnt);          // Noncompliant: Empty format specifier is not allowed

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.

if (b == 0)    //no issue, this could have been done on purpose to make the code more readable
{
DoSomething();
} 
else if (b == 1)
{
DoSomething();
}

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.

using System.Web.Mvc;

public class HomeController : Controller
{
public ActionResult Article()
{
    ViewBag.Title = "Title"; // Noncompliant, model should be used
    ViewData["Text"] = "Text"; // Noncompliant, model should be used
    return View();
}
}

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.

public void Evaluate()
{
/* ... */
{     // Noncompliant - nested code block '{' ... '}'
      int a = stack.pop();
      int b = stack.pop();
      int result = a + b;
      stack.push(result);
}
/* ... */
}

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

public struct Coordinates
{
public int X { get; }
public int Y { get; }

[ExcludeFromCodeCoverage] // Noncompliant
public override bool Equals(object obj) => obj is Coordinates coordinates && X == coordinates.X && Y == coordinates.Y;

[ExcludeFromCodeCoverage] // Noncompliant
public override int GetHashCode()
{
    var hashCode = 1861411795;
    hashCode = hashCode * -1521134295 + X.GetHashCode();
    hashCode = hashCode * -1521134295 + Y.GetHashCode();
    return hashCode;
}
}

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.

var bananaAndStrawberry = FruitType.Banana | FruitType.Strawberry; 
Console.WriteLine(bananaAndStrawberry.ToString());  // Will display only "Strawberry"

[Flags]
enum FruitType    // Noncompliant
{
None,
Banana,
Orange,
Strawberry
}

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.

class MyClass {
private int my_field;
}

This rule raises an issue when a {visibility} {operationName} is never referenced in the code.

public class Foo
{
private void UnusedPrivateMethod(){...} // Noncompliant, this private method is unused and can be removed.

private class UnusedClass {...} // Noncompliant, unused private class that can be removed.
}

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

public class Empty // Noncompliant
{
}

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.

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

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

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.

int GetBestNumber()
{
return 12;  // Noncompliant
}

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

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

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

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

if (true) 
{  
DoSomething(); 
}
...
if (false) 
{
DoSomethingElse(); 
}

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.

void DoSomething() 
{
; // Noncompliant - was used as a kind of TODO marker
}

void DoSomethingElse() 
{
Console.WriteLine("Hello, world!");;  // Noncompliant - double ;
// ...
// Rarely, they are used on purpose as the body of a loop. It is a bad practice to 
// have side-effects outside of the loop:
for (int i = 0; i < 3; Console.WriteLine(i), i++); // Noncompliant
// ...
}

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

public class Fruit 
{ 
protected Season ripe; 
protected Color flesh; 

// ... 
} 

public class Raspberry : Fruit 
{ 
private bool ripe; // Noncompliant 
private static Color FLESH; // Noncompliant 
}

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.

public bool CanVote(Person person) 
{
return person.GetAge() > 18 ? true : true; // Noncompliant; is this what was intended?
}

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.

global using System.Net.Sockets; // Compliant by exception

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

var hashProvider1 = new SHA512Managed(); // Compliant
var hashProvider2 = (HashAlgorithm)CryptoConfig.CreateFromName("SHA512Managed"); // Compliant
var hashProvider3 = HashAlgorithm.Create("SHA512Managed"); // Compliant

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

class MyException: Exception
{
public void MyException()
{
     if (bad_thing) 
     {
         throw new Exception("A bad thing happened");  // Noncompliant
      }
}
}

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

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

[Serializable]
public class Foo
{
[OptionalField(VersionAdded = 2)]
int optionalField = 5;    
}

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.

using System.IO;
using System.IO.Compression;

public class ZipHelper
{
public void Extract(ZipFile zipFile, string destinationDirectory)
{
    foreach (var entry in zipFile.Entries)
    {
        var destinationFileName = Path.GetFullPath(Path.Combine(destinationDirectory, entry.FullName));
        entry.ExtractToFile(destinationFileName); // entry.FullName could contain parent directory references (..) and make the
                                                  // file to be extracted in an arbitrary directory, outside of destinationDirectory
    }
}
}

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.

SHA256 mySHA256 = SHA256.Create()

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

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

string url2 = "http://guest:guest@domain.com"; // Compliant
const string Password_Property = "custom.password"; // Compliant

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

if (Compare(point.X, point.X) != 0) // Noncompliant 
{
  //... 
} 

if (DoSomething(GetNextValue(), GetNextValue()))  // Noncompliant 
{
  // ... 
}

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

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

class Person
{
private int age;

[Pure] // Noncompliant: The method makes a state change
void ConfigureAge(int age) =>
this.age = age;
}

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

This rule detects MSTest and NUnit ExpectedException attribute.

[TestMethod]
[ExpectedException(typeof(ArgumentNullException))]  // Noncompliant
public void TestNullArg()
{
//...
}

An unused local variable is a variable that has been declared but is not used anywhere in the block of code where it is defined. It is dead code, contributing to unnecessary complexity and leading to confusion when reading the code. Therefore, it should be removed from your code to maintain clarity and efficiency.

using(var t = new TestTimer()) // t never used, but compliant.
{
//...
}

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.

public class ExampleController : Controller
{
public IActionResult Compute(int data)
{
    for (int i = 0; i < data; i++) // Noncompliant
    {
        Console.WriteLine("Hello");
    }

    Enumerable
        .Range(1, data) // Noncompliant
        .ToList()
        .ForEach(i => Console.WriteLine("World"));

    return Ok();
}
}

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.

var foo = new Disposable();
foo.Dispose();
foo.Dispose(); // Noncompliant

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

try 
{
// Some work which end up throwing an exception
throw new ArgumentException();
}
finally 
{
// Cleanup
throw new InvalidOperationException(); // Noncompliant: will mask the ArgumentException
}

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.

int Foo(int y)
{
int x = 100; // Noncompliant: dead store
x = 150;     // Noncompliant: dead store  
x = 200;
return x + y;
}

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.

using System;

namespace MyLibrary
{
public class MyExtension : MarkupExtension
{
public MyExtension() { }

public MyExtension(object value1)
{
  Value1 = value1;
}

[ConstructorArgument("value2")]   // Noncompliant
public object Value1 { get; set; }
}
}

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.

decimal CalculateFinalPrice(User user, Cart cart)
{
decimal total = CalculateTotal(cart);
if (user.HasMembership()               // +1 (if)
    && user.OrdersCount > 10           // +1 (more than one condition)
    && user.AccountActive
    && !user.HasDiscount
    || user.OrdersCount == 1)          // +1 (change of operator in condition)
{
    
    total = ApplyDiscount(user, total);
}
return total;
}

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.

using System.Reflection;

[assembly: AssemblyTitle("MyAssembly")] // Noncompliant
namespace MyLibrary
{
// ...
}

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.

public class Rectangle
{
private readonly int length;
private readonly int width;  // Noncompliant: width is written but never read

public Rectangle(int length, int width)
{
this.length = length;
this.width = width;
}

public int Surface
{
get
{
  return length * width;
}
}
}

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.

public class Person
{
private int _birthYear; // Noncompliant

Person(int birthYear)
{
    _birthYear = birthYear;
}
}

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

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

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

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

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

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

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

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

[Export(typeof(ISomeType))]
public class SomeType // Noncompliant: doesn't implement 'ISomeType'.
{
}

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.

public bool Seek(int[] input)
{
var target = 32;  // Noncompliant
foreach (int i in input)
{
if (i == target) 
{
  return true;
}
}
return false;
}

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.

string d = null;
var v1 = d ?? "value";      // Noncompliant

Local variables should not shadow class fields or properties

class Foo
{
public int myField;
public int MyProperty { get; set; }

public void DoSomething()
{
int myField = 0;    // Noncompliant
int MyProperty = 0; // Noncompliant
}
}

Azure Functions should be stateless

public static class HttpExample
{
    private static readonly int port = 2000;
    private static int numOfRequests = 1;

    [FunctionName("HttpExample")]
    public static async Task<IActionResult> Run( [HttpTrigger(AuthorizationLevel.Anonymous, "get", Route = null)] HttpRequest request, ILogger log)
    {
        numOfRequests += 1; // Noncompliant
        log.LogInformation($"Number of POST requests is {numOfRequests}.");

        string responseMessage = $"HttpRequest was made on port {port}."; // Compliant, state is only read.

        return new OkObjectResult(responseMessage);
    }
}

Azure Functions should use Structured Error Handling

[FunctionName("HttpExample")]
public static async Task<IActionResult> Run(
[HttpTrigger(AuthorizationLevel.Function, "get", "post", Route = null)] HttpRequest req)
{
// Noncompliant
string requestBody = await new StreamReader(req.Body).ReadToEndAsync();
dynamic data = JsonConvert.DeserializeObject(requestBody);
// do stuff
}

Azure Functions should log all failures

[FunctionName("Foo")]
public static async Task<IActionResult> Run(
[HttpTrigger(AuthorizationLevel.Anonymous, "get", "post", Route = null)] HttpRequest req,
ILogger log)
{
try
{
	// do stuff that can fail
}
catch (Exception ex)
{
	// the failure is not logged at all OR is logged at DEBUG/TRACE level
}
}

Integral numbers should not be shifted by zero or more than their number of bits-1

var number = 14;         // ...01110 (14)
var left = number << 1;  // ...11100 (28) 
var right = number >> 1; // ...00111 (7)

Get Started

Ask Yourself Whether

Recommended Secure Coding Practices