Method having high number of lines of code(LOC)/statements are harder to understand and may violate single responsibility principle. Consider refactoring the method to modular methods.
The default threshold for maximum allowed LOC/statements for a method is 50. When the LOC/statements for a method exceeds the threshold limit, the violation is notified.
Non-complaint code: ///
/// Loads the module definition into a project content.
///
/// Unresolved type system representing the assembly
[CLSCompliant(false)]
public IUnresolvedAssembly LoadModule(ModuleDefinition moduleDefinition)
{
if (moduleDefinition == null)
throw new ArgumentNullException("moduleDefinition");
this.currentModule = moduleDefinition;
// Read assembly and module attributes
IList assemblyAttributes = new List();
IList moduleAttributes = new List();
AssemblyDefinition assemblyDefinition = moduleDefinition.Assembly;
if (assemblyDefinition != null) {
AddAttributes(assemblyDefinition, assemblyAttributes);
}
AddAttributes(moduleDefinition, moduleAttributes);
// Register type forwarders:
foreach (ExportedType type in moduleDefinition.ExportedTypes) {
if (type.IsForwarder) {
int typeParameterCount;
string ns = type.Namespace;
string name = ReflectionHelper.SplitTypeParameterCountFromReflectionName(type.Name, out typeParameterCount);
ns = interningProvider.Intern(ns);
name = interningProvider.Intern(name);
var typeRef = new GetClassTypeReference(GetAssemblyReference(type.Scope), ns, name, typeParameterCount);
typeRef = interningProvider.Intern(typeRef);
var key = new TopLevelTypeName(ns, name, typeParameterCount);
currentAssembly.AddTypeForwarder(key, typeRef);
}
}
// Create and register all types:
CecilLoader cecilLoaderCloneForLazyLoading = LazyLoad ? new CecilLoader(this) : null;
List cecilTypeDefs = new List();
List typeDefs = new List();
foreach (TypeDefinition td in moduleDefinition.Types) {
this.CancellationToken.ThrowIfCancellationRequested();
if (this.IncludeInternalMembers || (td.Attributes & TypeAttributes.VisibilityMask) == TypeAttributes.Public) {
string name = td.Name;
if (name.Length == 0)
continue;
if (this.LazyLoad) {
var t = new LazyCecilTypeDefinition(cecilLoaderCloneForLazyLoading, td);
currentAssembly.AddTypeDefinition(t);
RegisterCecilObject(t, td);
} else {
var t = CreateTopLevelTypeDefinition(td);
cecilTypeDefs.Add(td);
typeDefs.Add(t);
currentAssembly.AddTypeDefinition(t);
// The registration will happen after the members are initialized
}
}
}
// Initialize the type's members:
for (int i = 0; i < typeDefs.Count; i++) {
InitTypeDefinition(cecilTypeDefs[i], typeDefs[i]);
}
AddToTypeSystemTranslationTable(this.currentAssembly, assemblyDefinition);
// Freezing the assembly here is important:
// otherwise it will be frozen when a compilation is first created
// from it. But freezing has the effect of changing some collection instances
// (to ReadOnlyCollection). This hidden mutation was causing a crash
// when the FastSerializer was saving the assembly at the same time as
// the first compilation was created from it.
// By freezing the assembly now, we ensure it is usable on multiple
// threads without issues.
currentAssembly.Freeze();
var result = this.currentAssembly;
this.currentAssembly = null;
this.currentModule = null;
return result;
}
CyclomaticComplexityForMethodVoilationRule
Cyclomatic Complexity (CC) is a measure of the program's complexity achieved by measuring the number of linearly independent paths through a program's source code. The number of linearly independent paths also means the minimum number of paths that should be tested. The more paths, the higher the number of test cases that need to be implemented. Consider splitting this method into smaller methods.
The default threshold for the maximum allowed Cyclomatic Complexity for a method is 15. When the Cyclomatic Complexity for a method exceeds the threshold limit, the violation is notified.
Non-compliant code: static bool Compare(object val1, object val2, Type type)
{
if (val1 == val2)
return true;
if (val1 == null || val2 == null)
return false;
if (type == typeof(ResolveResult)) {
return IsEqualResolveResult((ResolveResult)val1, (ResolveResult)val2);
} else if (type == typeof(IVariable) || type == typeof(IParameter)) {
return IsEqualVariable((IVariable)val1, (IVariable)val2);
} else if (type == typeof(MethodListWithDeclaringType)) {
var l1 = (MethodListWithDeclaringType)val1;
var l2 = (MethodListWithDeclaringType)val2;
return object.Equals(l1.DeclaringType, l2.DeclaringType)
&& Compare(l1, l2, type.BaseType);
} else if (type.IsArray || type.IsGenericType && (type.GetGenericTypeDefinition() == typeof(List<>) || type.GetGenericTypeDefinition() == typeof(ReadOnlyCollection<>) || type.GetGenericTypeDefinition() == typeof(IList<>) || type.GetGenericTypeDefinition() == typeof(ICollection<>) || type.GetGenericTypeDefinition() == typeof(IEnumerable<>))) {
Type elementType = type.IsArray ? type.GetElementType() : type.GetGenericArguments()[0];
object[] arr1 = ((IEnumerable)val1).Cast
InvalidLoggingClassNameRule
Consider creating the logger name in context to the current class name.
Weak encryption algorithms provide very little security and insufficient protection for sensitive data hence it is recommended to use a more secure encryption algorithm, such as AES.
Non-compliant code :
using (var tripleDES = new TripleDESCryptoServiceProvider()) //Noncompliant
{
//...
}
Disposable types should not be publicly exposed as properties or fields.
Non-compliant code: public IDisposable Test { get { return null; } }
DoNotChangeLoopVariablesRule
Do not change a loop variable inside the loop. Changing variable inside the loop may cause inconsistent behavior and a possible runtime exception.
Non-compliant code void TestForLoopVariableChange()
{
for (int i = 0; i < 20; i++)
{
i = 14;
}
}
AsyncMethodShouldNotHaveThreadSleepRule
An asynchronous method should not use a synchronous thread sleep mechanism. Thread.Sleep blocks the current thread; instead, use Task.Delay. Task.Delay pauses execution but will free the thread for other execution.
Non-compliant code
Task MyMethod()
{
Thread.Sleep(1000);
}
Compliant code:
Task MyMethod1()
{
await Task.Delay(1000);
}
CatchBlockShouldThrowExceptionRule
A catch block should throw exceptions instead of eating the exception. Swallowing an exception without re-throwing it's details, tampers the origin of exception and makes debugging difficult.
Throwing an exception from a finally block may hide an exception thrown from the try or catch block.
Non-compliant code
Something()
{
var value = 1;
try
{
// do something
}
catch (System.ArgumentException)
{
// do something
}
finally
{
Increment(ref value);
throw new System.Exception($"Unbecoming exception No {value}");
}
}
IneffectiveExceptionHandlingRule
While handling exceptions, the 'Message' property does not contain any useful information which can assist in debugging the cause for the exception. Useful information can be extracted from the 'InnerException' property and should be used while handling and logging exceptions.
Non-compliant code: public static void Foo()
{
try
{
new object();
}
catch (Exception e)
{
Console.WriteLine(e.Message);
}
}
UsingMoreSpecificExceptionTypeRule
Throw statements should not directly use types such as Exception, SystemException, and ApplicationException because they are too vague and don't have information about the failure reason. Use more specific types of exceptions or create your own that represents a given exceptional situation. Having a specific type for all types of failures makes logs filtering and aggregating much easier. Custom exception types allow also to convey in semantic form more contextual information inside the exception object.
Non-compliant code :
try
{
// do something
}
catch (ArgumentException)
{
throw new ArgumentException($"Hiding the original exception {value}");
}
Compliant code :
try
{
//some code here
}
catch (ArgumentException ex)
{
throw new CustomException("", ex);
}
MultiThreading_AcquireLockInsideTryRule
When manually managing scope of locking by directly calling methods responsible for acquiring and releasing the lock , ensure that the lock is acquired inside try clause.
Non-compliant code :
class MyClass
{
private readonly object myLockObj = new object();
public void MyMethod()
{
Monitor.Enter(myLockObj);
}
}
Compliant code :
class MyClass
{
private readonly object myLockObj = new object();
public void MyMethod()
{
var lockAcquired = false;
try
{
lockAcquired = Monitor.TryEnter(myLockObj);
}
finally
{
if (lockAcquired)
Monitor.Exit(myLockObj);
}
}
}
MultiThreading_MethodLevelSynchronizationRule
Do not lock on the publicly accessible member as it can result in a deadlock.Type encapsulating access to a resource that requires synchronization should ensure that resource is not public.
Non-compliant code:
[MethodImpl(MethodImplOptions.Synchronized)]
public void DoFirstThing()
{
//some code here
}
MultiThreading_NoLockOnPublicMembersRule
Do not lock on the publicly accessible member as it can result in a deadlock. Type encapsulating access to a resource that requires synchronization; it should be ensured that synchronization should be encapsulated.
Non-compliant code :
public class SomeObject
{
public void SomeOperation()
{
lock(this)
{
//Access instance variables
}
}
}
MultiThreading_ReleaseLockInsideFinallyRule
When manually managing the scope of locking by directly calling methods responsible for acquiring and releasing, ensure that the lock is released inside the finally clause to ensure that the lock is manually released in case of any exception.
Non-compliant code :
class MyClass
{
private readonly object myLockObj = new object();
public void MyMethod()
{
try
{
Monitor.Enter(myLockObj);
}
catch(Exception ex)
{
...
...
}
Monitor.Exit(myLockObj);
}
}
Compliant code :
class MyClass
{
private readonly object myLockObj = new object();
public void MyMethod()
{
try
{
Monitor.Enter(myLockObj);
}
catch(Exception ex)
{
...
...
}
finally
{
Monitor.Exit(myLockObj);
}
}
}
DontCallVirtualMethodsInConstructorRule
When a virtual method is called, the actual type that executes the method is not selected until run time. When a constructor calls a virtual method, it's possible that the type that invokes the method has not been initialized yet.
class DontCallVirtualMethodsInConstructorRule_Test
{
DontCallVirtualMethodsInConstructorRule_Test()
{
var test = new DontCallVirtualMethodsInConstructorRule_Test();
test.A();
}
public virtual void A() { }
}
DateTimeNowUsageRule
Storing date/time values in UTC makes the data transportable, as the data is no longer tied to a particular time zone. To get the current date and time in UTC, it is recommended to use DateTime.UtcNow instead of DateTime.Now
A method returning values representing strings or collections or task should never be null. The consumer of the APIs returning NULL would break if not handled.
Non-compliant code :
public IEnumerable GetProductIds(int brandId)
{
var products = productService.GetProductsForBrand(brandId);
if (products == null || !products.Any())
{
return null;
}
return products.Select(p => p.Id);
} Compliant code :
public IEnumerable GetProductIds(int brandId)
{
var products = productService.GetProductsForBrand(brandId);
if (products == null)
{
return Enumerable.Empty();
}
return products.Select(p => p.Id);
}
UseArrayEmptyAPIRule
While initializing an array, avoid using 'new T[0]'. Instead use 'Array.Empty' & '()' to avoid unnecessary allocation.
Non-compliant code :
public void UseEmptyArrayForInit_Test()
{
var variable1 = new int[0];
}
Compliant code :
public void UseEmptyArrayForInit_Test()
{
var variable1 = Array.Empty();
}
MakeMethodStaticRule
If a method is not referencing any instance variable and if you are not creating a virtual/abstract/new or partial method and if it is not a method override, your instance method may be changed to a static method. Therefore we recommend declaring the method as static.
Non-compliant code: public void Foo()
{
Console.WriteLine("A::Foo()");
}
Compliant code: public static void Foo()
{
Console.WriteLine("A::Foo()");
}
ClassImplementsICloneableCheckRule
ICloneable does not define which kind of cloning is done - deep or shallow copies. Therefore we recommend defining your Clone() or Copy() methods and document whether they perform deep or shallow copying instead of using ICloneable interface.
Non-compliant code public class Person : ICloneable
{
public int Age;
public string Name;
public Person(int age, string name)
{
Age = age;
Name = name;
}
public string ToString()
{
return "(" + age + "," + name + ")";
}
public virtual object Cloning()
{
return new Person(age, name);
}
object ICloneable.Clone()
{
return Cloning();
}
}
Compliant code :
public class IdInfo
{
public int IdNumber;
public IdInfo(int IdNumber)
{
this.IdNumber = IdNumber;
}
}
public class Person
{
public int Age;
public string Name;
public IdInfo IdInfo;
public Person ShallowCopy()
{
return (Person) this.MemberwiseClone();
}
public Person DeepCopy()
{
Person other = (Person) this.MemberwiseClone();
other.IdInfo = new IdInfo(IdInfo.IdNumber);
other.Name = String.Copy(Name);
return other;
}
}
public class Example
{
public static void Main()
{
Person p1 = new Person();
p1.Age = 35;
p1.Name = "John";
p1.IdInfo = new IdInfo(10076);
Person p2 = p1.ShallowCopy();
Console.WriteLine("Original values of p1 and p2:");
Console.WriteLine("p1 instance values:");
DisplayValues(p1);
}
public static void DisplayValues(Person p)
{
Console.WriteLine("Name: {0:s}, Age: {1:d}", p.Name, p.Age);
Console.WriteLine("Value: {0:d}", p.IdInfo.IdNumber);
}
}
SwitchWithoutDefaultRule
The switch statement should include a default clause.
Non-compliant code :
switch (value)
{
case 1:
Console.WriteLine(1);
break;
case 2:
Console.WriteLine(2);
break;
}
Compliant code :
switch (value)
{
case 1:
Console.WriteLine(1);
break;
case 2:
Console.WriteLine(2);
break;
default:
Console.WriteLine(0);
break;
}
DoNotHideInheritedMembersRule
On hiding an inherited member using the "new" modifier, the derived version of the member replaces the base version.
Non-compliant code :
BaseClass
{
public string BaseMethod()
{
return "is Base";
}
}
class Derviedclass : BaseClass
{
//Hides inherited member
public new string BaseMethod()
{
return "is Derived";
}
}
Compliant code variant 1:
class BaseClass
{
public virtual string BaseMethod()
{
return "is Base";
}
}
class Derviedclass : BaseClass
{
public override string BaseMethod()
{
return "is Derived";
}
}
Compliant code variant 2:
class BaseClass
{
public string BaseMethod()
{
return "is Base";
}
}
class Derviedclass : BaseClass
{
public string DerivedMethod()
{
return "is Derived";
}
}
UsingContextWhileThrowingExceptionRule
While throwing an exception, add contextual information that can be used for constructing an appropriate error message. The rule ensures using context-aware constructors in throw statements.
Non-compliant code :
try
{
// do something
}
catch (Exception ex)
{
// do something
throw;
}
Compliant code :
``` try { // do something } catch (Exception ex) { // do something throw new Exception ("Put more context here", ex) }
UsingInnerExceptionInNewExceptionThrownRule
Catch block could throw another exception that would be more meaningful for the clients. When throwing a new exception from the catch clause, always pass caught exception as an innerException parameter for the new exception.
Non-compliant code :
public void Test()
{
try
{
//some code here
}
catch (Exception ex)
{
throw new CustomException("");
}
}
Compliant code :
public void Test()
{
try
{
//some code here
}
catch (Exception ex)
{
throw new CustomException("", ex);
}
}
DisposableFieldNotDisposedRule
The class having disposable field should call the dispose method of the field.
Non-compliant code :
public class TypeA : IDisposable
{
protected virtual void Dispose(bool disposing)
{
if (disposing)
{
// Dispose managed resources
}
}
// Disposable types implement a finalizer.
}
public class TypeB : IDisposable
{
// Assume this type has some unmanaged resources.
TypeA aFieldOfADisposableType = new TypeA();
private bool disposed = false;
protected virtual void Dispose(bool disposing)
{
if (!disposed)
{
disposed = true;
if (disposing)
{
GC.SuppressFinalize(this);
}
}
}
// Disposable types implement a finalizer.
}
Compliant code :
protected virtual void Dispose(bool disposing)
{
if (!disposed)
{
// Dispose of resources held by this instance.
aFieldOfADisposableType.Dispose();
disposed = true;
// Suppress finalization of this disposed instance.
if (disposing)
{
GC.SuppressFinalize(this);
}
}
}
DisposablesShouldCallSuppressFinalizeRule
Classes implementing IDisposable should call the GC.SuppressFinalize method in their finalize method to avoid any finalizer from being called. This rule should be followed even if the class does not have a finalizer as a derived class could have one.
Non-compliant code :
public class MyType : System.IDisposable
{
void IDisposable.Dispose()
{
Dispose(true);
}
protected virtual void Dispose(bool disposing)
{
}
}
A class should be marked as sealed class if there is no inheritor from the class.
Non-compliant code :
class Calculations
{
public int Add(int a, int b)
{
return a + b;
}
}
class Program
{
static void Main(string[] args)
{
Calculations calc = new Calculations();
int total = calc.Add(6, 4);
Console.WriteLine("Total = " + total.ToString());
}
} Compliant code :
sealed class Calculations
{
public int Add(int a, int b)
{
return a + b;
}
}
class Program
{
static void Main(string[] args)
{
Calculations calc = new Calculations();
int total = calc.Add(6, 4);
Console.WriteLine("Total = " + total.ToString());
}
}
