views:

70

answers:

3

Why is the FxCop rule CA1061 a bad idea?

The docs state that this rule should not be suppressed. If I have class like so:

public class Set<T>
{    List<T> m_backingList;

     public bool Contains(T value)
     {
         return m_backingList.Contains(value);
     }
}  

then I add a specific implementation like this:

public class CaseInsensitiveSet : Set<String>
{
    public bool Contains(object value)
    {
         string stringValue  = value as string;
         if (stringValue == null)
             return false;
         return base.Contains(stringValue);
    }
}

the FxCop complains, but I'm not certain why this is such a bad idea. Is there some problem I don't see with this implementation?

A: 

http://msdn.microsoft.com/en-us/library/ms182143(VS.80).aspx

A method in a base type is hidden by an identically named method in a derived type when the parameter signature of the derived method differs only by types that are more weakly derived than the corresponding types in the parameter signature of the base method.

EDIT

Basically you're hiding the base method (public bool Contains in Set), which will never now be run in preference to the derived method. But the derived method is more weakly defined than the base method so there are situations when the base method is the preferable method.

amelvin
thanks. I had read the page that I linked to in the question.
Sam Holder
@Sam Sorry if that seemed a bit cheap, but I had to unexpectedly do some work in between posting the MSDN answer and offering some additional comments.
amelvin
+4  A: 

The rule states why you're getting the message:

A method in a base type is hidden by an identically named method in a derived type when the parameter signature of the derived method differs only by types that are more weakly derived than the corresponding types in the parameter signature of the base method.

In your child class, the Contains method takes an object which is more weakly typed than string and therefore hides the parent.

The reason you're getting the warning from FxCop is that this might not be an intentional design choice (since you're not overriding anything or using the new keyword).

Even if it is an intentional design choice, I would argue that it's not necessarily a good one. If you already know that the collection is going to contain strings and nothing else, why would you provide a Contains method that takes anything other than a string? It may appear that you're adding flexibility into the design but, in the end, you're really only going to confuse other developers.

There are also other naming options instead of calling the method Contains which wouldn't hide (intentionally or not) the base Contains method.

Justin Niessner
+1. Yes, I agree
RichardOD
Thanks. Even if I use the new keyword (which it lets me do) it still complains. So in the above example (with or without the new) would I be ok to ignore the warning or will I get suspect behaviour because of this somewhere down the line?
Sam Holder
A: 

Ask yourself: do I want the users to be able to call the base class method on an instance of the derived class. If the answer is yes: don't hide the base method, as this will make it more cumbersome to use it. If the answer is no: don't derive from this class, or else they can still access the base method by casting the object to the base class.

Henrik