Generic equality checker

Question

I use this method to check if two reference types are equal

 public static bool AreEquals<T>(T source, object obj) where T : class
 {
 if (ReferenceEquals(source, obj))
 return true;
 var convertedTarget = obj as T;
 if (ReferenceEquals(convertedTarget, null))
 return false;
 List<object> equalityMembers = typeof (T).GetProperties(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance).Where(p => p.GetCustomAttribute<EqualityMemberAttribute>() != null).ToList<object>();
 equalityMembers.AddRange(typeof(T).GetFields(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance).Where(p => p.GetCustomAttribute<EqualityMemberAttribute>() != null).ToList<object>());
 if (equalityMembers.Count == 0)
 return false; //The references were not equals and there is nothing to compare..
 var enumerator = equalityMembers.GetEnumerator();
 bool areEquals = true;
 while (enumerator.MoveNext() && areEquals)
 {
 var current = enumerator.Current;
 //On sait que FieldInfo et PropertyInfo ont la méthode GetValue
 var methodInfo = current.GetType().GetMethod("GetValue", new [] {typeof (object)});
 object valueSource = methodInfo.Invoke(current, new object[] {source});
 object valueObj = methodInfo.Invoke(current, new object[] {convertedTarget});
 areEquals = valueSource.Equals(valueObj);
 }
 return areEquals;
 }

I use attributes on properties/fields to find which of them are used to decide if the two objects are equals ex :

public class MyClass
{
 [EqualityMember]
 public int ID{get;set;}
 public override Equals(object obj)
 {
 return MyStaticClass.AreEquals(this,obj);
 }
}

It works very well but I am bothered with how I get the properties and fields in a list and use reflection to invoke the GetValue method, would you have any other alternatives? Or is there anything that seems flawed in my method?

• svick: Why don't you override Equals() and GetHashCode() (and possibly implement IEquatable<T>)? That's the usual way to do it, since it means you can use your types as keys in dictionaries and LINQ GroupBy queries and so on.
• IEatBagels: Oh ehm, I should add an example to my code, this is what I do. I'll add the code when I'm on my computer, on a mobile editing is horrible
• svick: You can't override Equals() without also overriding GetHashCode(). How do you deal with that?
• IEatBagels: Well, I usually write it by hand, but it wouldn't be complicated to write a similar helper to do it!

Answer 1

List<object> equalityMembers = typeof (T).GetProperties(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance).Where(p => p.GetCustomAttribute<EqualityMemberAttribute>() != null).ToList<object>();
equalityMembers.AddRange(typeof(T).GetFields(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance).Where(p => p.GetCustomAttribute<EqualityMemberAttribute>() != null).ToList<object>());

I would consider caching these values. Usually, the custom attributes do not change during runtime, and having the complete list cached after the first run should improve performance of subsequent comparisons a lot.

while (enumerator.MoveNext() && areEquals)

Since areEquals is initialized as true, you could use while (areEquals && enumerator.MoveNext()) instead, thus avoiding an additional call to MoveNext if the process has already determined that the objects are not equal. However, I do not know, why you use the IEnumerator manually, because would be intrigued to write the whole loop as a simple foreach loop and just return false on the first value that is not equal in both instances.

areEquals = valueSource.Equals(valueObj);

This throws a NullReferenceException if valueSource is null.

Honestly, instead of using GetValue via reflection, I would just save PropertyInfo and FieldInfo in separate lists and compare them in separate loops. For example

public static class EqualityComparer<T> where T : class
{
 private static readonly BindingFlags flags = BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance;
 private static readonly IReadOnlyCollection<PropertyInfo> propertiesForEquality = typeof(T).GetProperties(flags).Where(p => p.GetCustomAttribute<EqualityMemberAttribute>() != null).ToList();
 private static readonly IReadOnlyCollection<FieldInfo> fieldsForEquality = typeof(T).GetFields(flags).Where(p => p.GetCustomAttribute<EqualityMemberAttribute>() != null).ToList();
 private static readonly bool hasEqualityMembers = propertiesForEquality.Any() || fieldsForEquality.Any();
 public static bool AreEquals(T source, object obj)
 {
 if (ReferenceEquals(source, obj))
 return true;
 var convertedTarget = obj as T;
 if (ReferenceEquals(convertedTarget, null))
 return false;
 if (!hasEqualityMembers)
 return false; //The references were not equals and there is nothing to compare..
 foreach (var propertyInfo in propertiesForEquality)
 {
 var valueSource = propertyInfo.GetValue(source);
 var valueObj = propertyInfo.GetValue(obj);
 if (valueSource == null && valueObj != null || !valueSource.Equals(valueObj))
 {
 return false;
 }
 }
 foreach (var fieldInfo in fieldsForEquality)
 {
 var valueSource = fieldInfo.GetValue(source);
 var valueObj = fieldInfo.GetValue(obj);
 if (valueSource == null && valueObj != null || !valueSource.Equals(valueObj))
 {
 return false;
 }
 }
 return true;
 }
}

In a quick benchmark (really simple class with one field and one property used for equality comparison), my version was a lot faster. You may want to benchmark that with your concrete classes. :)

If you do not want to have multiple foreach loops, you can also build a small wrapper around PropertyInfo and FieldInfo. In my simple test, the following did not lead to any noticable performance degradation.

public interface IValueGetter
{
 object GetValue(object obj);
}
public class FieldInfoValueGetter : IValueGetter
{
 private readonly FieldInfo fieldInfo;
 public FieldInfoValueGetter(FieldInfo fieldInfo)
 {
 if (fieldInfo == null)
 {
 throw new ArgumentNullException("fieldInfo");
 }
 this.fieldInfo = fieldInfo;
 }
 public object GetValue(object obj)
 {
 return this.fieldInfo.GetValue(obj);
 }
}
public class PropertyInfoValueGetter : IValueGetter
{
 private readonly PropertyInfo propertyInfo;
 public PropertyInfoValueGetter(PropertyInfo propertyInfo)
 {
 if (propertyInfo == null)
 {
 throw new ArgumentNullException("propertyInfo");
 }
 this.propertyInfo = propertyInfo;
 }
 public object GetValue(object obj)
 {
 return this.propertyInfo.GetValue(obj);
 }
}
public static class InterfacedEqualityComparer<T> where T : class
{
 private static readonly BindingFlags flags = BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance;
 private static readonly IReadOnlyCollection<IValueGetter> valueGetters = typeof(T).GetProperties(flags).Where(p => p.GetCustomAttribute<EqualityMemberAttribute>() != null).Select(p => (IValueGetter)new PropertyInfoValueGetter(p))
 .Union(typeof(T).GetFields(flags).Where(p => p.GetCustomAttribute<EqualityMemberAttribute>() != null).Select(p => new FieldInfoValueGetter(p)))
 .ToList();
 private static readonly bool hasEqualityMembers = valueGetters.Any();
 public static bool AreEquals(T source, object obj)
 {
 if (ReferenceEquals(source, obj))
 return true;
 var convertedTarget = obj as T;
 if (ReferenceEquals(convertedTarget, null))
 return false;
 if (!hasEqualityMembers)
 return false; //The references were not equals and there is nothing to compare..
 foreach (var valueGetter in valueGetters)
 {
 var valueSource = valueGetter.GetValue(source);
 var valueObj = valueGetter.GetValue(obj);
 if (valueSource == null && valueObj != null || !valueSource.Equals(valueObj))
 {
 return false;
 }
 }
 return true;
 }
}

I know that I changed the method signature, but having the method be not generic but embedding it in a generic class is probably the easiest way to cache loaded property and field info data, but that can be changed to a generic method in a non-generic class.

public class EqualityComparer
{
 private static readonly BindingFlags flags = BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance;
 private static readonly IDictionary<Type, object> valueGetterCache = new Dictionary<Type, object>();
 public static bool AreEquals<T>(T source, object obj) where T : class
 {
 if (ReferenceEquals(source, obj))
 return true;
 var convertedTarget = obj as T;
 if (ReferenceEquals(convertedTarget, null))
 return false;
 var valueGetters = GetValueGettersForType<T>();
 if (!valueGetters.Any())
 return false; //The references were not equals and there is nothing to compare..
 foreach (var valueGetter in valueGetters)
 {
 var valueSource = valueGetter.GetValue(source);
 var valueObj = valueGetter.GetValue(obj);
 if (valueSource == null && valueObj != null || !valueSource.Equals(valueObj))
 {
 return false;
 }
 }
 return true;
 }
 private static IReadOnlyCollection<IValueGetter> GetValueGettersForType<T>() where T : class
 {
 var type = typeof(T);
 object dictionaryValue;
 IReadOnlyCollection<IValueGetter> valueGetters;
 if (!valueGetterCache.TryGetValue(type, out dictionaryValue) || (valueGetters = dictionaryValue as IReadOnlyCollection<IValueGetter>) == null)
 {
 valueGetters = type.GetProperties(flags).Where(p => p.GetCustomAttribute<EqualityMemberAttribute>() != null).Select(p => (IValueGetter)new PropertyInfoValueGetter(p))
 .Union(type.GetFields(flags).Where(p => p.GetCustomAttribute<EqualityMemberAttribute>() != null).Select(p => new FieldInfoValueGetter(p)))
 .ToList();
 valueGetterCache.Add(type, valueGetters);
 }
 return valueGetters;
 }
}

Note that the main advantage seems to be the caching - if you remove it, this performs about as fast as your original approach. :)

• IEatBagels: I found your answer to be very constructive, especially concerning the caching of the attributes which I didn't think about. Thank you!

Answer 2

I took a shot at this as well. I think @Hangy had a few good ideas but ended up with a rather complex solution. I haven't added any caching so you might want to add that yourself afterwards.

What I changed:

• Different check for null which is clearer in my opinion.
• Braces for one-line statements.
• Extracted member parsing to a separate method.
• Instead of holding all data of PropertyInfo and FieldInfo, I simply use a Dictionary<string, object> which translates to <FieldName, FieldValue>.
• Unused properties and fields data is eligible for GC as soon as you extracted the needed information.
• Got rid of the Enumerator and added some juicy LINQ.

The end result:

public static class EqualityAttribute
{
 public static bool AreEquals<T>(T source, object obj) where T : class
 {
 if (ReferenceEquals(source, obj))
 {
 return true;
 }
 var convertedTarget = obj as T;
 if (convertedTarget == null)
 {
 return false;
 }
 var obj1Members = GetFieldEqualityMembers(source);
 var obj2Members = GetFieldEqualityMembers(obj);
 var noMembersToCompare = obj1Members.Values.Count == 0 && obj2Members.Values.Count == 0;
 if (noMembersToCompare)
 {
 return false;
 }
 if (obj1Members.Keys.Any(key => !obj1Members[key].Equals(obj2Members[key])))
 {
 return false;
 }
 return true;
 }
 private static Dictionary<string, object> GetFieldEqualityMembers<T>(T obj)
 {
 var properties = obj.GetType().GetProperties(BindingFlags.Public |
 BindingFlags.NonPublic |
 BindingFlags.Instance)
 .Where(x => x.GetCustomAttribute<EqualityMemberAttribute>() != null)
 .ToDictionary(x => x.Name, x => x.GetValue(obj));
 var fields = obj.GetType().GetFields(BindingFlags.Public |
 BindingFlags.NonPublic |
 BindingFlags.Instance)
 .Where(x => x.GetCustomAttribute<EqualityMemberAttribute>() != null)
 .ToDictionary(x => x.Name, x => x.GetValue(obj));
 properties.ToList().ForEach(x => fields.Add(x.Key, x.Value));
 return fields;
 }
}

• IEatBagels: I like the way you implemented the dictionary! I will certainly look into that. I'm not a big fan of braces for one liners though, but that's just a matter of opinion. Also, for the null check, I used ReferenceEquals in fear that someone might override the == operator of the object I'm checking.
• hangy: Interesting approach. I don't think my version is too complex - if you go without the optional IValueGetter approach and just use two loops, it is pretty close to the original. I just left everything in, so that it's obvious how I got there. ;)
• hangy: Caching reflected propinfo/fieldinfo seems to be crucial for performance, so they cannot be GC'd directly, if a cache is added. This version has the disadvantage, that it will definitely get the value of every field and property. The other approaches could perform better, because they will return as soon as a first mismatch is found, instead of getting more attribute values via reflection calls. I imagine this might have some impact on larger object graphs that use this method, but it could be as well be premature optimization on my side.
• svick: Why do you use List.ForEach() instead of foreach? foreach is simpler and more idiomatic.
• Jeroen Vannevel: No reason in particular, it just kept it condensed while still being easily readable.