I would appreciate your feedback on my custom EqualityComparer using IEqualityComparer<> interface

Question

I have a custom PropertiesByValueComparer and am fairly happy how it behaves for simple classes. I haven't included comparing by fields yet. Is there anything that is blatantly fail about this, or do you have other recommendations?

public class PropertiesByValueComparer<T> : IEqualityComparer<T> where T : class
{
    private List<PropertyInfo> properties;
    private List<FieldInfo> fieldInfos;

    public PropertiesByValueComparer()
    {
        Type t = typeof(T);

        this.properties = t.GetProperties(BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance).ToList();
        this.fieldInfos = t.GetFields(BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance).ToList();
    }

    public bool Equals(T x, T y)
    {
        var pool = new List<object>();

        return this.Equals(x, y, pool);
    }

    public bool Equals(T x, T y, List<object> pool)
    {
        if(pool.Contains(x) && pool.Contains(y))
        {
            return true;
        }

        if ((x == null && y == null) || ReferenceEquals(x, y))
        {
            pool.Add(x);
            pool.Add(y);

            return true;
        }

        if (x == null || y == null)
        {
            return false;
        }

        var xList = (x as IList);
        var yList = (y as IList);

        if(xList != null && yList != null)
        {
            var result = CompareCollectionIgnoreOrder(xList, yList, pool);
            if(result)
            {
                pool.Add(xList);
                pool.Add(yList);
                return true;
            }
        }

        var valueProperties = GetValueProperties();

        foreach (var property in valueProperties)
        {
            if (!Equals(property.GetValue(x, null), property.GetValue(y, null)))
            {
                return false;
            }
        }

        var classProperties = properties.Where(p => p.PropertyType.IsClass && p.PropertyType != typeof(String));

        foreach (var classProperty in classProperties)
        {
            Type valueComparerType = typeof(PropertiesByValueComparer<>);

            Type typeArg = classProperty.PropertyType;

            Type constructed = valueComparerType.MakeGenericType(typeArg);

            if (classProperty.PropertyType.Namespace != null && classProperty.PropertyType.Namespace.Equals("System.Collections.Generic"))
            {
                var collectionX = classProperty.GetValue(x, null);
                var collectionY = classProperty.GetValue(y, null);

                if (collectionX == null && collectionY == null)
                {
                    continue;
                }

                var arrayX = (collectionX as IList);
                var arrayY = (collectionY as IList);

                if(!CompareCollectionIgnoreOrder(arrayX, arrayY, pool))
                {
                    return false;
                }

                continue;
            }
            else
            {
                object[] args = {classProperty.GetValue(x, null), classProperty.GetValue(y, null), pool};

                object o = Activator.CreateInstance(constructed);

                if (!(bool)constructed.InvokeMember("Equals", BindingFlags.Public | BindingFlags.Instance | BindingFlags.InvokeMethod, null,o, args))
                {
                    return false;
                }
            }
        }

        pool.Add(x);
        pool.Add(y);
        return true;
    }

    private bool CompareCollectionIgnoreOrder(IList arrayX, IList arrayY, List<object> pool )
    {
        if ((arrayX == null && arrayY != null) || (arrayY == null && arrayX != null))
        {
            return false;
        }

        if (arrayX == null && arrayY == null)
        {
            return true;
        }

        if (arrayX.Count == 0 && arrayY.Count == 0)
        {
            return true;
        }

        if (arrayX.Count != arrayY.Count)
        {
            return false;
        }

        foreach (var itemX in arrayX)
        {
            foreach (var itemY in arrayY)
            {
                Type valueComparerType = typeof (PropertiesByValueComparer<>);

                Type typeX = itemX.GetType();

                if(typeX.IsValueType || typeX == typeof(String))
                {
                    if(Equals(itemX, itemY))
                    {
                        arrayY.Remove(itemY);
                        break;
                    }

                    continue;
                }

                Type innerConstructed = valueComparerType.MakeGenericType(typeX);

                object iO = Activator.CreateInstance(innerConstructed);

                var iArgs = new object[] { itemX, itemY, pool };

                if ((bool)innerConstructed.InvokeMember("Equals", BindingFlags.Public | BindingFlags.Instance | BindingFlags.InvokeMethod, null, iO, iArgs))
                {
                    arrayY.Remove(itemY);
                    break;
                }
                return false;
            }
        }

        if (arrayY.Count > 0)
        {
            return false;
        }

        return true;
    }

    public int GetHashCode(T obj)
    {
        var valueProperties = GetValueProperties();

        unchecked
        {
            int result = 0;

            foreach (var property in valueProperties)
            {
                var value = property.GetValue(obj, null);
                result = (result * 397) ^ (value != null ? value.GetHashCode() : 0);
            }

            return result;
        }
    }

    private IEnumerable<PropertyInfo> GetValueProperties()
    {
        var valueProperties = properties.Where(p => p.PropertyType.IsValueType || p.PropertyType == typeof(String));

        return valueProperties;
    }
}

Ok, so here is one more iteration, to take care of dictionaries as well.

Let me know if you find any more issues. Quite a nice little project this :-)

public class PropertiesByValueComparer<T> : IEqualityComparer<T> where T : class
{
    private static List<PropertyInfo> properties;
    private static List<FieldInfo> fieldInfos;

    public PropertiesByValueComparer()
    {
        Type t = typeof(T);

        properties = t.GetProperties(BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance).ToList();
        fieldInfos = t.GetFields(BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance).ToList();
    }

    public bool Equals(T x, T y)
    {
        var pool = new Dictionary<object, object>();

        return this.Equals(x, y, pool);
    }

    public bool Equals(T x, T y, Dictionary<object, object> pool)
    {
        if ((x == null && y == null))
        {
            return true;
        }

        if (x == null || y == null)
        {
            return false;
        }

        if (Object.ReferenceEquals(x, y))
        {
            return true;
        }

        if (x.GetType().IsValueType || x is String)
        {
            return Object.Equals(x, y);
        }

        if ((pool.ContainsKey(x) && pool[x] == y) || pool.ContainsKey(y) && pool[y] == x)
        {
            return true;
        }


        var xEnumerable = (x as IEnumerable);
        var yEnumerable = (y as IEnumerable);

        if (xEnumerable != null && yEnumerable != null)
        {
            var result = CompareCollectionIgnoreOrder(xEnumerable, yEnumerable, pool);

            if (result)
            {
                pool[x] = y;

                return true;
            }

            return false;

        }

        var valueProperties = GetValueProperties();

        foreach (var property in valueProperties)
        {
            if (!Equals(property.GetValue(x, null), property.GetValue(y, null)))
            {
                return false;
            }
        }

        var classProperties = GetClassProperties();

        foreach (var classProperty in classProperties)
        {

            Type typeArg = classProperty.PropertyType;

            var itemX = classProperty.GetValue(x, null);
            var itemY = classProperty.GetValue(y, null);

            if (!ReflectedCompare(typeArg, itemX, itemY, pool))
            {
                return false;
            }

        }

        pool[x] = y;

        return true;
    }

    private bool CompareCollectionIgnoreOrder(IEnumerable enumerableX, IEnumerable enumerableY, Dictionary<object, object> pool)
    {
        if ((enumerableX == null && enumerableY != null) || (enumerableY == null && enumerableX != null))
        {
            return false;
        }

        if (enumerableX == null)
        {
            return true;
        }

        var listX = (enumerableX as IList);
        var listY = (enumerableY as IList);

        if (listX != null && listY != null)
        {
            return CompareListIgnoreOrder(listX, listY, pool);
        }

        var dictX = (enumerableX as IDictionary);
        var dictY = (enumerableY as IDictionary);

        if (dictX != null && dictY != null)
        {

            return CompareDictionaryIgnoreOrder(dictX, dictY, pool);
        }

        return false;
    }

    private bool CompareDictionaryIgnoreOrder(IDictionary dictX, IDictionary dictY, Dictionary<object, object> pool)
    {
        var dictXKeys = dictX.Keys;
        var dictYKeys = dictY.Keys;

        if (dictXKeys.Count != dictYKeys.Count)
        {
            return false;
        }

        foreach (var dictXKey in dictXKeys)
        {
            Type keyXType = dictXKey.GetType();

            var enumerator = dictYKeys.GetEnumerator();

            for (var i = 0; i < dictYKeys.Count; i++ )
            {
                enumerator.MoveNext();

                var dictYKey = enumerator.Current;

                if (ReflectedCompare(keyXType, dictXKey, dictYKey, pool))
                {
                    var dictXValue = dictX[dictXKey];
                    var dictYValue = dictY[dictYKey];

                    if (dictXValue != null && dictYValue != null)
                    {
                        if (dictXValue.GetType() == dictYValue.GetType())
                        {
                            if (ReflectedCompare(dictXValue.GetType(), dictXValue, dictYValue, pool))
                            {
                                break;
                            }
                        }
                        else
                        {
                            return false;
                        }
                    }
                }

                if(i == (dictYKeys.Count - 1))
                {
                    return false;
                }
            }
        }

        return true;
    }


    private bool CompareListIgnoreOrder(IList listX, IList listY, Dictionary<object, object> pool)
    {
        if (listX.Count == 0 && listY.Count == 0)
        {
            return true;
        }

        if (listX.Count != listY.Count)
        {
            return false;
        }

        foreach (var itemX in listX)
        {
            foreach (var itemY in listY)
            {


                Type typeX = itemX.GetType();

                if (typeX.IsValueType || typeX == typeof(String))
                {
                    if (Object.Equals(itemX, itemY))
                    {
                        listY.Remove(itemY);
                        break;
                    }

                    continue;
                }



                if (ReflectedCompare(typeX, itemX, itemY, pool))
                {
                    listY.Remove(itemY);

                    break;

                }

                return false;
            }
        }

        if (listY.Count > 0)
        {
            return false;
        }

        return true;
    }

    private bool ReflectedCompare(Type genericType, object itemX, object itemY, Dictionary<object, object> pool)
    {
        var iArgs = new object[] { itemX, itemY, pool };

        Type valueComparerType = typeof(PropertiesByValueComparer<>);

        Type innerConstructed = valueComparerType.MakeGenericType(genericType);

        object iO = Activator.CreateInstance(innerConstructed);

        return (bool)innerConstructed.InvokeMember("Equals", BindingFlags.Public | BindingFlags.Instance | BindingFlags.InvokeMethod, null, iO, iArgs);
    }

    public int GetHashCode(T obj)
    {
        var valueProperties = GetValueProperties();

        unchecked
        {
            int result = 0;

            foreach (var property in valueProperties)
            {
                var value = property.GetValue(obj, null);
                result = (result * 397) ^ (value != null ? value.GetHashCode() : 0);
            }

            return result;
        }
    }

    private IEnumerable<PropertyInfo> GetValueProperties()
    {
        var valueProperties = properties.Where(p => p.PropertyType.IsValueType || p.PropertyType == typeof(String));

        return valueProperties;
    }

    private IEnumerable<PropertyInfo> GetClassProperties()
    {
        return properties.Where(p => p.PropertyType.IsClass && p.PropertyType != typeof(String));
    }
}

Cheers,

Martin

Answer 1

Some comments / suggestions:

• Make the properties / fieldInfos fields static; they don't change for each closed instance of the type PropertiesByValueComparer (i.e., for each T passed to it), so you don't need to initialize them for every new instance of the comparer
• On Equals(T, T, List<object>), there's no need to add both x and y if ReferenceEquals returns true - they're the same object, and you're only using the pool to check for pre-searched objects
• GetValueProperties is implemented as a (single-line) method; to fetch the "class properties" you use the lambda expression inline; the code should be consistent (either do both Where expressions inline, or both as helper methods)
• The calls to ReferenceEquals and Equals should be prefixed by Object. and base. respectively, so that we know without looking at the rest of the class that those are the methods from Object, not a helper method in the class
• [minor] Instead of using classProperty.PropertyType.Namespace.Equals("System.Collections.Generic"), I'd remove the string and use something like classProperty.PropertyType.Namespace.Equals(typeof<IList<object>>.Namespace)
• The comparer doesn't handle Dictionary<K,V>, since you're only looking for IList; if you started looking for IEnumerable<T> (and added a special case for KeyValuePair<K,V>) it would handle dictionaries as well
• I think the pool logic might be broken; you're adding objects which you see to the pool, and if the objects are on the pool then they're considered the same. It will fail if you have two objects of type A with three properties as shown below:

Objects:

object 1 { prop1 = B, prop2 = C, prop3 = B }
object 2 { prop1 = B, prop2 = C, prop3 = C }

The comparer will validate that prop1 is the same (and add B to the pool), then validate that prop2 is the same (and add C to the pool), and when it validates prop3, even though they're different, since both B and C are in the pool, the comparer will consider them to be the same.

Answer 2

One small addition to carlosfigueira's answer. I would also propose extracting ValueProperties nad ClassProperties in the constructor, there is no point to store properties in this case at all and you can avoid executing reflection and linq again and again for each GetValueProperties call.

Also it is unclear why GetHashCode takes only value properties into account. Even though it will definitely work but looks a little bit strange. Maybe you should add a comment why class properties are ignored?