views:

207

answers:

4

So I am utilizing CollectionBase as an inherited class for custom collections. I am utilizing CollectionBase through an abstract class so that I don't repeated knowledge (following the DRY principle). The abstract class is defined as a generic class also. Here is how I am implementing my class:

 public abstract class GenericCollectionBase<T,C> : CollectionBase
 {
      //Indexders, virtual methods for Add, Contains, IndexOf, etc
 }

I utilize this so I don't have to implement these base methods in 10+ classes.

My question is am I taking this too far when I override the Equals method like this:

    public override bool Equals(object obj)
    {
        if (obj is C)
        {
            GenericCollectionBase<T, C> collB = 
                obj as GenericCollectionBase<T, C>;

            if (this.Count == collB.Count)
            {
                for (int i = 0; i < this.Count; ++i)
                {
                    if (!this[i].Equals(collB[i]))
                        return false;
                }
                return true;
            }
        }
        return false;
    }

Am I trying to accomplish too much with my abstract, or doing this the correct way?

EDIT: This is written for .Net 2.0 and do not have access to 3.5 to utilize things like LINQ

A: 

Making an extension method against IDictionary would be far more useful. There's also methods like Intersect from LINQ that may be useful.

ermau
+3  A: 

I don't believe you are trying to accomplish too much. If an abstract class was meant to not have any implementation at all, or other methods which define functionality, then they would be interfaces.

The only thing I would change is to use EqualityComparer<T> instead of equals for the comparison of this[i] and collB[i].

casperOne
I am getting this error when using "if(!EqualityComparer<T>.Equals(this[i], collB[i]))" An object reference is required for the nonstatic field, method, or property 'System.Collections.Generic.EqualityComparer<T>.Equals(T, T)'
phsr
Equals(T, T) is not a static method; EqualityComparer<T>.Default.Equals(this[i], collB[i]) is what you want.
Vojislav Stojkovic
Awesome, thanks!
phsr
+1  A: 

Well, first, this is weird :

    if (obj is C)
    {
        GenericCollectionBase<T, C> collB = obj as GenericCollectionBase<T, C>;

I'll assume you meant that :

    GenericCollectionBase<T, C> collB = obj as GenericCollectionBase<T, C>;
    if (collB != null)
    {
        ...

I think you're over-thinking this, except if you really, really need two different collections with the same content to be considered as equal. I'd put this logic in another method to be called explicitly or in an equality comparer.

ybo
A: 

I don't know if you're trying to accomplish too much, but I think you're trying to accomplish the wrong thing. There are cases where you might want that type of equality for collections, but it should be opt-in and obvious from the name of the type. I've created a ListValue<> with the type of equality you're using, but then it's always been immutable as well.

Also, if you're going to do this type of equality check, an initial test using object.ReferenceEquals can save you from having to iterate over a large collection when your comparing an object to itself.

Freed