(How)
C^# You Are - 15 June 2008

These questions revolve around some of the code analysis errors you might run across when developing code with code analysis enabled.  Note that the code used in these questions are for demonstrating the problem and should not be considered good, proper coding styles.

1. This code generates the error Collection properties should be read only.  What is wrong with this?  Answer
   
   
   public class Department
   {
      public EmployeeCollection Employees
      {
         get { return m_Employees; }
         set { m_Employees = value; }
      }
   
      private EmployeeCollection m_Employees = new EmployeeCollection();
   }

   CA2227: The problem is that you are gaining very little over having a read-only property.  Items in the collection can still be modified, added or removed.  Making a collection property writable allows someone to replace your existing collection with a brand new one and this is rarely a good idea.  It also limits maintainability since, at a future date, you might chose to use a specialized, or even new, collection type.  Making the collection property writable would prevent this.

   This rule should be excluded only in extremely rare circumstances.  In all other cases redesign your code to eliminate it.

2. This code generates the error Do not cast unnecessarily Why?  Answer
   
   
   public class CueBanne   public void SetCueBanner ( Control control, string text )
      {
         if (control is ComboBox)
         {
             ComboBox cb = (ComboBox)control;
             ...
         } else if (control is TextBox)
         {
             TextBox tb = (TextBox)control;
             ...
         };
      }
   }

   CA1800: The is operator is useful but only in limited circumstances.  In general you should use the as operator instead.  The problem is a performance issue.  If you use the is operator and then use either an as operator or an explicit cast to convert to the type you just checked then you will be paying for the runtime typechecking twice.  Whenever you do an explicit cast or use the as operator the CLR must verify the type is correct.  The only difference between the two is that one will throw an exception and one won't.  Since it is rare that you ever just want to check the type of an object you should rarely use is.

   This rule should never be excluded.  The size of the written code has no relation to the runtime size or performance so use the right operator instead.

3. This code generates the error Enums should have zero value.  Why?  Answer
   
   
   [Flags]
   public enum OperationOptions
   {
       IncludeEmpty = 0x1,
       ExcludeLarge  = 0x2,
       AddPrefix       = 0x3,
   }

   CA1008: Enumerations are glorified integral values, even in .NET.  The default value of an enumeration is zero.  If you do not define an enum value for zero then the enumeration will still get a zero value but it will not map to anything.  Contrary to popular belief it is perfectly legal to assign an integral value to an enumeration variable.  The CLR will not complain.  However if the numeric value does not map to a valid combination then displaying the value will just display the number.

   In the MSDN documentation you might have noticed that any member that accepts an enumeration as a parameter often will specify an exception that occurs when the parameter is not a valid valid.  It is a safety measure.  CA1008 is designed to help you find such issues.

   The only time you might want to remotely consider excluding this rule is if the enumeration is used for mapping values to unmanaged code.  Even then it might not hurt.

4. This code generates the error Mark members as static.  Why?  Answer
   
   
   public class SimpleClass
   {
      protected string FormatObject ( object value, string format )
      {
         return value.ToString(format);
      }
   }

   CA1822: This error is raised when you use a non-static, non-virtual method inside a class and the method does not reference any non-static members of the class.  CA1822 is a performance rule.  It is pointing out that instance methods have more overhead than static methods.  Since the method in question does not have any instance data it would perform better if you were to make it a static member.

   This rule can be excluded when it does not make sense to mark the member as static.  A protected member might be a good example.

5. This code generates the error Interface methods should be callable by child types.  Why?  Answer
   
   
   public class EmployeeCollection : ICollection<T>
   {
      public Employee[] ToArray ( ) { ... }
   
      void ICollection<T>.CopyTo ( T[] array, int startingIndex )
      { ... }
   }

   CA1033: This error is generated when you have a non-sealed class that implements an interface member in a non-virtual member.  In this case the error is telling you that, since the class is not sealed, derived classes will not be able to override or change the underlying interface implementation because the base class does not expose a virtual method.

   This rule can be ignored if this is the behavior you want.  Sometimes you do not want a derived class to override an interface implementation.  You should examine each occurrence of this error and determine if a derived class might ever need to override the base implementation.  If so then you should make the member virtual or call a virtual method.  Otherwise you can safely exclude the error.

6. This code generates the error Specify StringComparison.  Why?  Answer
   
   
   public static string Trim ( string trimString, string value )
   {
      int len = trimString.Length;
   
      while (value.StartsWith(trimString))
         value = value.Substring(len);
   
      while (value.EndsWith(trimString))
         value = value.Substring(0, value.Length - len - 1);
   
      return value;
   }

   CA1307:  This rule is for globalization.  Whenever you are dealing with comparing strings or formatting certain types like numbers, dates or currency then globalization becomes a concern.  In any of these cases you should consider how to handle non-English languages (or whatever your default language is).  For string comparisons you should expose an overload that accepts a StringComparison parameter.  This allows users to handle the comparison based upon the current language or in a language neutral manner. Furthermore case sensitivity now becomes an issue for the caller to solve.  Note that you can (should) still expose overloads that do not require any additional parameters.  These overloads can use a standard set of options instead.

   This rule should never be excluded.  Even if you are not planning for world domination today you might be in the future.  Additionally case sensitivity is always an issue so it is better to pass the buck onto the caller rather than having to deal with it yourself.

7. This code generates the error Do not ignore method results.  Why?  Answer
   
   
   public object GetKeyValue ( string key )
   {
      object value;
      dictionary.TryGetValue(key, out value);
   
      return value;
   }

   CA1806: The error is in regards to ignoring the return value of a method.  This practice is one of the reasons why .NET moved away from error codes for functions. 

   In the code above the return value indicates whether the value was successfully retrieved.  In this particular case the returned value will always be set by the called method irrelevant of whether it was successful or not.  Therefore in this particular case it might be OK to ignore the return value.  It depends upon what the method should do when the key is missing.  If the method is going to throw an exception if the key cannot be found then it probably is not necessary to call the TryGetValue.  It would make more sense to simply index the dictionary and let the dictionary throw the exception.

   Each occurrence of this rule should be evaluated.  In most cases the method return value should be used.  In the rare case where it is meaningless then you can exclude this rule.

8. This code generates the error Instantiate argument exceptions correctly.  Why?  Answer
   
   
   public void InitializeObject ( string value )
   {
      if (value == null)
         throw new ArgumentNullException("value", "Value is null.");
      if (value.Length < 10)
         throw new ArgumentException("value", "Value must be at least 10 characters.");
   
      ...
   }

   CA2208:  This is a good one.  Evidently the developer who wrote ArgumentException did not talk to the developer who wrote ArgumentNullException (and some others).  In the former case the parameter ordering is message, name.  In the latter it is name, message.  They are backwards.  Fortunately we have Intellisense (usually) to help us out.

   This error gets tripped when it detects your usage of one of these exceptions and the name parameter does not match the name of one of the parameters to the method in question.  This indicates a problem.

   Except for one circumstance this error should never be excluded.  The exception is if you use helper methods to build exception objects.  In this case the parameter names are probably passed to the helper method rather than being the helper method names.  Code analysis is not smart enough to realize what you are trying to do.  You can therefore exclude the error in this case.

9. This code generates the error Do not expose generic lists.  Why?  Answer
   
   
   public class Department
   {
      ...
      public List<Employee> Employees
      {
         get { return m_Employees; }
      } 
   }

   CA1002:  This rule gets violated all the time.  To be fair it is rarely the fault of the developer but rather the confusion that .NET has caused.  Lists and collections are different beasts even though they do similar things.  Making this even worse is the fact that the interfaces are what truly differentiates them but the standard implementation classes implement both the interfaces.  This causes major confusion.

   You should almost never expose List/List<T> directly in code.  These classes are designed to be used as implementation details and everybody knows not to expose implementation details.  Collections are more flexible (from a caller's standpoint) and should be used instead.

   Each occurrence of this rule should be evaluated.  In most cases the error should be fixed.  The only exception is if you are implementing support classes for use inside a library (in which case they should probably be internal) or if you are implementing collection support classes.  Even in this particular case you should evaluate whether a collection would work better.

10. This code generates the error Override equals and operator equals on value types.  Why?  Answer
    
    
    public struct Address
    {
       public string Street { get { get } }
       public string City { get { ... } }
       public string State { get { ... } }
       public string ZipCode { get { ... } }
    
       private string m_Street, m_City, m_State, m_Zip;
    }

    CA1815: Value types have a default equality operator defined for them automatically.  This is great.  The not so great part is that it uses reflection to compare the field values.  It is always faster to explicitly define the equality/not equal operators and do it yourself.

    This error should rarely be excluded. The only viable time it could be is for nested types or in interop structures where equality is never used.