HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Custom EqualityComparer using IEqualityComparer<> interface

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
interfacecustomusingequalitycompareriequalitycomparer

Problem

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 : IEqualityComparer where T : class
{
private List properties;
private List 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();

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

public bool Equals(T x, T y, List 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)
{

Solution

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), 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>.Namespace)



  • The comparer doesn't handle Dictionary, since you're only looking for IList; if you started looking for IEnumerable (and added a special case for KeyValuePair) 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.

Code Snippets

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

Context

StackExchange Code Review Q#3088, answer score: 7

Revisions (0)

No revisions yet.