FxCop Rules, Causes and Fixes - Part 2

Continuing the previous article on FxCop rules. I have added some more rules and their reasons.

Rule: StaticHolderTypesShouldNotHaveConstructors

Cause: A Public type declares only static members and has a public or default constructor.

Fix: Remove the public constructor and replace that with a private constructor.

Rule: StaticHolderTypesShouldBeSealed

Cause: A Public type declares only static members is not declared with a sealed modifier

Fix: Mark the type as sealed.

Rule: IdentifiersShouldHaveCorrectSuffix

Cause: Types that extend certain base types or that implement certain interfaces, or types derived from these types, have a suffix that is associated with the base type or interface.

The following table lists the base types and interfaces that have associated suffixes.

Base type/Interface	Suffix
System.Attribute	Attribute
System.EventArgs	EventArgs
System.Exception	Exception
System.Collections.ICollection	Collection
System.Collections.IDictionary	Dictionary
System.Collections.IEnumerable	Collection
System.Collections.Queue	Collection or Queue
System.Collections.Stack	Collection or Stack
System.Collections.Generic.ICollection	Collection
System.Collections.Generic.IDictionary	Dictionary
System.Data.DataSet	DataSet
System.Data.DataTable	Collection or DataTable
System.IO.Stream	Stream
System.Security.IPermission	Permission
System.Security.Policy.IMembershipCondition	Condition
An event-handler delegate.	EventHandler

Fix: Rename the type so that it is suffixed with the correct term.

Rule: IdentifiersShouldNotContainUnderscores

Cause: identifier contains the underscore (_) character.

Fix: Remove the underscore.

Rule: UriPropertiesShouldNotBeStrings, UriParametersShouldNotBeStrings, UriReturnValuesShouldNotBeStrings

Cause: A Property, Parameter or a method that contains Uri, Url etc in the name is declared as a String. A string representation of a URI is prone to parsing and encoding errors, and can lead to security vulnerabilities. The System.Uri class provides these services in a safe and secure manner.

Fix: Change the return type to Uri.

Rule: DoNotDeclareVisibleInstanceFields

Cause: An externally visible type with modifiers (Public, Protected/ Protected Internal) has an externally visible instance field.

Fix: Make the externally visible field private or internal and expose it using an externally visible property.

Rule: DoNotInitializeUnnecessarily

Cause: A field is initialized to its default value. The common language runtime initializes all fields to their default values before running the constructor. In most cases, initializing a field to its default value in a constructor is redundant, which degrades performance and adds to maintenance costs.

Fix: Remove the unnecessary initializations.

Rule: IdentifiersShouldBeCasedCorrectly

Cause: The name of an identifier is not cased correctly. By convention, parameter names use camel casing; namespace, type, and member names use Pascal casing. In a camel-cased name, the first letter is lower case, and the first letter of any remaining words in the name is in uppercase. In a Pascal-cased name, the first letter is upper case, and the first letter of any remaining words in the name is in uppercase.

Fix: Change the name so that it is cased correctly.

Rule: CompoundWordsShouldBeCasedCorrectly

Cause: The name of an identifier contains multiple words and at least one of the words appears to be a compound word that is not cased correctly.

Fix: Change the name so that it is cased correctly.

Rule: IdentifiersShouldNotMatchKeywords

Cause: A namespace name or a type name matches a reserved keyword in a case-insensitive comparison. Using a reserved keyword as the name of a virtual/interface member makes it harder for consumers in other languages to override/implement the member.

Fix: Change the name to something that does not appear in the list of keywords.

Rule: PropertyNamesShouldNotMatchGetMethods

Cause: The name of a public or protected member starts with 'Get' and otherwise matches the name of a public or protected property.

Fix: Change the name so that it does not match the name of a method prefixed with 'Get'.

FxCop Rules, Causes and Fixes - Part 1

Using FxCop as a code analysis tool provides a lot of benefits in fixings the errors / coding standards in the application. I will try to list out the common errors and fixes for those errors.

Rule: MarkAssembliesWithClsCompliant

Cause: Assemblies should explicitly indicate CLS compliance with CLSCompliantAttribute. If the attribute is not present on an assembly, the assembly is not CLS compliant. Assembly does not have the System.CLSCompliantAttribute attribute applied to it.

Fix: Add the reference to the namespace System in the AssemblyInfo.cs file and add the attribute [assembly: CLSCompliant(true)] to the file

Rule: AvoidNamespacesWithFewTypes

Cause: A namespace other than the global namespace contains fewer than five types.

Fix: Combine namespaces that contain a small number of types (Less than 5) into a single namespace.

Rule: AssembliesShouldHaveValidStrongNames

Cause: An assembly is not signed with a strong name, the strong name could not be verified, or the strong name would not be valid without the current registry settings of the computer. You have delay signed the assembly. You have used the strong name tool to skip verification for the assembly.

Fix: Create a strong name file using the sn tool and sign the assembly.

Rule: LongAcronymsShouldBePascalCased

Cause: The name of an identifier contains an acronym of three or more uppercase letters

Fix: Change any three-letter acronyms in the name to Pascal case, and change any two-letter acronyms to uppercase

Rule: ShortAcronymsShouldBeUppercase

Cause: The name of an identifier contains a two-letter acronym and the second letter is lowercase.

Fix: Change the second letter of acronym to upper case.

Rule: UsePropertiesWhereAppropriate

Cause: A public method that does not take a parameter starts with a Get and does not return a collection.

Fix: Change the method to a property.

Rule: AvoidUninstantiatedInternalClasses

Cause: Internal classes that are not instantiated in the assembly. The class may have only static members.

Fix: Create a private constructor to prevent the compiler creating an instance of the class.

DoNotExposeGenericLists error and Using Collection<T> instead of List<T>

If you have ever exposed a List<T> from a method / Property in a .NET assembly code, while running FxCop you are supposed to get the ‘DoNotExposeGenericLists’ error as follow.

Do not expose List<T> in object models. Use Collection<T>, ReadOnlyCollection<T> or KeyedCollection instead. List<T> is meant to be used from implementation, not in object model API. List<T> is optimized for performance at the cost of long term versioning. For example, if you return List<T> to the client code, you will not ever be able to receive notifications when client code modifies the collection.

Reason:

· List<T> is not designed for extensibility. It does not support members that can be overridden. An object returning a List<T> won’t be able to get notified when the collection is changed.
· List<T> is intended to be a class used in internal implementation. By exposing a List<T> in the interface of your class, you are breaking encapsulation by giving client code access to an implementation detail of your class--especially if your property provides direct access to the internal List<T> .
· Collection<T> is designed for extensibility and helps you to notify changes in the collection by overriding the methods exposed by the Collection<T>

David Kean from MS Code Analysis team has a very good article explaining the rule. I will try to use Dave’s example to explain the advantages of using Collection<T> over List<T> while exposing the type for public API.

The following example shows how to derive a collection class from a constructed type of Collection<T> generic class, and how to override the InsertItem, ClearItem, SetItem and RemoveItem to provide notification about the collection changes.


public class Person
{
private AddressCollection _addresses;

public Collection<Address> Addresses
{
get
{
if (_addresses == null)
_addresses = new AddressCollection(this);
return _addresses;
}
}

public event EventHandler<addresschangedeventargs> AddressChanged;

protected virtual void OnAddressInserted(Address item)
{
EventHandler<addresschangedeventargs> temp = AddressChanged;

if (temp != null)
temp(this, new AddressChangedEventArgs(ChangeType.Added, item.Country, null));
}

private void RaiseAddressInserted(Address item)
{
OnAddressInserted(item);
}



private class AddressCollection : Collection<address>
{
private Person _owner;

public AddressCollection(Person _person)
{
_owner = _person;
}


protected override void InsertItem(int index, Address item)
{
base.InsertItem(index, item);
_owner.RaiseAddressInserted(item);
}

}
}


From the above code you can see how to get notified by exposing the Collection<T> when changes occur in the collection. Similarly you can implement the RemoveItem, ClearItem and SetItem methods and raise events that notify the respective changes.


You can also encapsulate the List<T> and expose the methods that are relevant to the application by calling the underlying List<T> implementation. I will show how to expose the Find(), Sort() and FindAll() methods of List<T> while using Collection<T>

For example I have made some changes to the AddressCollection class for exposing the List<T> methods


private class AddressCollection : Collection<Address>
{
private Person _owner;
public AddressCollection(Person _person) : base(new List<Address>())
{
_owner = _person;
}


. . . .
. . . .

protected internal Address Find(Predicate<Address> match)
{
List<Address> items = (List<Address> )Items;
return items.Find(match);
}

protected internal void Sort(Comparison<Address> match)
{
List<Address> items = (List<Address> )Items;
items.Sort(match);
}

protected internal IEnumerable<Address> FindAll(Predicate<Address> match)
{
List<Address> items = (List<Address> )Items;
return (IEnumerable<Address> )items.FindAll(match);
}
}


Later in your Person class you can have the methods to get the desired functionality


public Address FindAddressByStreet(String street)
{
return _addresses.Find(delegate(Address a)
{
return a.Street.StartsWith(street);
});
}


public IEnumerable<Address> FindAllAddressByState(String state)
{
return _addresses.FindAll(delegate(Address a)
{
return a.State == state;
});
}


public void SortAddressByState()
{
_addresses.Sort(delegate(Address a1, Address a2)
{
return a1.State.CompareTo(a2.State);
});
}