Coding standards

fxCop

We use FxCop to check wether DelftShell sources follow correct coding standards such as

• correct CapITalizaTION
• exception throwing
• naming

The reports are currently generated for core functionality only.

An fxcop report can be generated manually by running the fxcop project on Teamcity. To inspect the results we can use FxCopGraph and FxCopReport.

Common mistakes

Often I find people use System.Exception instead of a more specific one like ArgumentException or ReadOnlyException etc.
Assemblies are often missing the ClsCompliant attribute and SecurityPermission Attribute.

Example

[assembly: CLSCompliant(true)] //fxcop prerequisite
[assembly: SecurityPermission(SecurityAction.RequestMinimum, Execution = true)] //fxcop prerequisite

When using a format statement or a ToString() function, the CultureInfo is usually missing. At least specify CultureInfo.InvariantCulture so the conversion always yields the same result when intended:

String comparison:

• String comparison for equality – Specify StringComparison.Ordinal or OrdinalIgnoreCase. This is true when comparing against string resources, and particularly important when comparing file names.
• String comparison for sorting - Specify StringComparison.CurrentCulture when sorting lists
• String.ToUpper - Use ToUpper rather than ToLower, and specify InvariantCulture in order to pick up OS casing rules
• Determining BiDi languages, for example to load correct icons - Continue to specify CurrentUICulture for these cases.
• String formatting including numbers, and int.ToString - Specify CurrentCulture
• String formatting of strings only (no numbers) - the Culture arg here is a no-op. Use CurrentCulture.

examples:

myInt.ToString(CultureInfo.InvariantCulture)
string.Format(CultureInfo.InvariantCulture, "Tomorrow I will be {0} years old", 37)

Implementation of IDisposable is often incomplete. For classes that are not sealed you have to implement the Dispose Pattern. A codeproject article provides some details on how to implement IDisposable.

The public void Dispose() method should be left final (in other words, don't make this a virtual method) and should always look like this

public void Dispose()
{
    Dispose(true);
    GC.SupressFinalize(this);
}

All of your resource cleanup should be contained in the Dispose(bool disposing) method. If necessary, you should protect the cleanup by testing the disposing parameter. This should happen for both managed and unmanaged resources. The Dispose(bool disposing) runs in two distinct scenarios:

• If disposing equals true, the method has been called directly or indirectly by a user's code. Managed and unmanaged resources can be disposed.
• If disposing equals false, the method has been called by the runtime from inside the finalizer and you should not reference other objects. Only unmanaged resources can be disposed.

protected virtual void Dispose(bool disposing)
{
    if (!disposed)
    {
        if (disposing)
        {
            // Dispose managed resources.
        }

        // There are no unmanaged resources to release, but
        // if we add them, they need to be released here.
    }
    disposed = true;

    // If it is available, make the call to the
    // base class's Dispose(Boolean) method
    base.Dispose(disposing);
}

Often people cast an object multiple times, leading to small performance degradation

if (myobject is IDataItem) //first cast
{
   node.Tag = ((IDataItem)myObject).Value; // second cast.
}

This can be done a bit more efficient in the following way:

IDataItem item
if ((item = myObject as IDataItem) != null) //first cast.
{
   node.Tag = item.Value;
}

Links

I found the following links on internet about coding standards while fixing some issues:
document on coding standards
Exception throwing
Exception handling
FxCop, locale installatie
Using CultureInfo.InvariantCulture
Writing your own fxcop rules
When and how to dispose and finalize in c#
Why StringComparison.Ordinal is usually the right choice
New Recommendations for Using Strings in Microsoft .NET 2.0