9
\$\begingroup\$

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;
    }
}

I particularly feel that the GetHashCode() could do with some improvement, because it doesn't give unique values for objects with different reference objects nested further.

\$\endgroup\$
0
7
\$\begingroup\$

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.

\$\endgroup\$
5
  • \$\begingroup\$ one question I have for you @carlosfigueira is in regards to your suggestion to handle dictionaries: How would you know the <T> part of the IEnumerable you are checking against? The problem is (and this is why I chose the IList in the first place) that at compile time I don't know the value of T. \$\endgroup\$ – Martin Jun 24 '11 at 10:59
  • \$\begingroup\$ I have made some modifications, and am curious as to what the best strategy for showing them would be? replace the original codeblock, or add underneath? \$\endgroup\$ – Martin Jun 24 '11 at 13:31
  • \$\begingroup\$ @Martin, I believe adding modified code below the original one is ok. Otherwise it won't be clear what our current answers were about. Just make sure that it will be displayed separately from the original code. \$\endgroup\$ – Snowbear Jun 24 '11 at 15:40
  • \$\begingroup\$ @Martin, IDictionary<K,V> is-a IEnumerable<KeyValuePair<K,V>>, so if (typeof(T).IsGenericType && typeof(T).GetGenericTypeDefinition() == typeof(KeyValuePair<,>)), then you can apply a comparison logic for lists of KVP. Another option is to first check for IDictionary, then IEnumerable. \$\endgroup\$ – carlosfigueira Jun 24 '11 at 16:48
  • \$\begingroup\$ @carlosfigueira made some changes as suggested. looks pretty good to me now. \$\endgroup\$ – Martin Jun 28 '11 at 14:40
3
\$\begingroup\$

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?

\$\endgroup\$
2
  • \$\begingroup\$ If I'm not mistaken, the GetHashCode() function normally takes the object's memory address into it's calculation. I'll have to do some more reading up on that. \$\endgroup\$ – Martin Jun 28 '11 at 12:32
  • \$\begingroup\$ @Martin, well, it depends, because GetHashCode in your classes used as properties may be overridden as well \$\endgroup\$ – Snowbear Jun 28 '11 at 14:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.