0
\$\begingroup\$

This question is NOT about passing a Comparison to a Sort.

The question is how to pass a Comparison to an object for object to object (1:1) Comparison. Some users must be able to pass in a Comparison. They want 2 as want be able to tell when they give different comparison.

The way this solution approaches it is to pass a Comparison delegate to the constructor.

static int CompareMotorPrice(Motorcycle motorcycle1, Motorcycle motorcycle2)
{
    return motorcycle1.Price.CompareTo(motorcycle2.Price);
}
static int CompareMotorSpeed(Motorcycle motorcycle1, Motorcycle motorcycle2)
{
    return motorcycle1.Speed.CompareTo(motorcycle2.Speed);
}
public class Motorcycle : IComparable
{
    public string Name { get; }
    public int Speed { get; set; }
    public int Price { get; set; }
    public Comparison<Motorcycle> CompareDelegate { get; set; }
    public Comparison<Motorcycle> CompareDefault { get; set; }
    public int CompareViaDelegate(Motorcycle motorcycle2)
    {
        if (CompareDelegate == null || motorcycle2 == null)
        {
            throw new ArgumentNullException();
        }
        return (CompareDelegate(this, motorcycle2));
    }
    public int CompareTo(object obj)
    {
        if (obj == null) return 1;

        Motorcycle otherMotorcycle = obj as Motorcycle;
        if (otherMotorcycle != null)
        {
            if (CompareDefault == null)
            {
                return (this.Name.CompareTo(otherMotorcycle.Name));
            }
            else
            {
                return CompareDefault(this, otherMotorcycle);
            }
        }
        else
        {
            throw new ArgumentException("Object is not a otherMotorcycle");
        }
    }
    public Motorcycle(string name, int price, int speed, Comparison<Motorcycle> compareDefault = null, Comparison<Motorcycle> compareDelegate = null)
    {
        Name = name;
        Price = price;
        Speed = speed;
        CompareDelegate = compareDelegate;
        CompareDefault = compareDefault;
    }
}

test:

Motorcycle motorcycle1 = new Motorcycle("harley", 1000, 120, CompareMotorSpeed, CompareMotorPrice);
Motorcycle motorcycle2 = new Motorcycle("yamaha", 1900, 130, CompareMotorSpeed, CompareMotorPrice);
Motorcycle motorcycle3 = new Motorcycle("susuki", 1800, 150, CompareMotorSpeed, CompareMotorPrice);
Debug.WriteLine(motorcycle1.CompareViaDelegate(motorcycle2));
List<Motorcycle> Motorcycles = new List<Motorcycle>() { motorcycle1, motorcycle2, motorcycle3 };
foreach (Motorcycle m in Motorcycles)
{
    Debug.WriteLine($"name {m.Name}   price {m.Price}   speed {m.Speed}");
}
Motorcycles.Sort(CompareMotorPrice);
foreach (Motorcycle m in Motorcycles)
{
    Debug.WriteLine($"name {m.Name}   price {m.Price}   speed {m.Speed}");
}
Motorcycles.Sort();
foreach (Motorcycle m in Motorcycles)
{
    Debug.WriteLine($"name {m.Name}   price {m.Price}   speed {m.Speed}");
}
\$\endgroup\$
7
  • 2
    \$\begingroup\$ This is not very useful because that's exactly what the IEqualityComparer<> and IComparable<> interfaces are for. You are reinventing the wheel in a non-.net-compatible way. \$\endgroup\$ – t3chb0t Jun 4 '18 at 6:40
  • \$\begingroup\$ @t3chb0t It does implement IComparable. I am sorry you do not see the value. \$\endgroup\$ – paparazzo Jun 4 '18 at 15:27
  • \$\begingroup\$ @paparazzo, t3chb0t is talking about generic interfaces (IComparable<Motorcycle>). You do not implement those. \$\endgroup\$ – Nikita B Jun 4 '18 at 15:56
  • \$\begingroup\$ @NikitaB Not following. msdn.microsoft.com/en-us/library/… \$\endgroup\$ – paparazzo Jun 4 '18 at 16:02
  • 2
    \$\begingroup\$ @paparazzo Type<> is sometimes used for referring to a type with a single type parameter (as per typeof(Thing<>)), Type<,> having 2 type params, Type<,,> having 3, etc. etc. \$\endgroup\$ – VisualMelon Jun 4 '18 at 17:12
3
\$\begingroup\$

1)

    if (CompareDelegate == null || motorcycle2 == null)
    {
        throw new ArgumentNullException();
    }

This exception is misleading. First, it does not specify which parameter is null. Second, strictly speaking CompareDelegate is not a parameter.

2) I would set the default value for CompareDefault:

public Comparison<Motorcycle> CompareDefault { get; set; } = 
    other => Name.CompareTo(other.Name);

this way you can probably remove some of the null checks and simplify CompareTo implementation.

3) Overall, I don't like this approach.

  • it scales poorly.
  • it hides comparison logic: there is no way to learn how two objects are going to be compared just by reading the code, the comparison logic is hidden inside some delegate.
  • most importantly, you break asymmetric property of comparison. If a.CompareTo(b) > 0 then I expect that b.CompareTo(a) < 0. But it is not the case, since nothing in your code prevents two objects from using different delegates for comparison. This alone looks like a huge source of problems to me.

IMHO, just write a bunch of regular IComparer<T> implementations and be done with it. It has none of the problems I mentioned above:

//custom comparers are implemented as stand alone classes, 
//they are not part of Motorcycle class.
public class Motorcycle
{
    public string Name { get; }
    public int Speed { get; set; }
    public int Price { get; set; }

    public Motorcycle(string name, int price, int speed)
    {
        Name = name;
        Price = price;
        Speed = speed;
    }
}

//It also guarantees asymmetry:
//if SpeedComparer.Compare(a,b) < 0 then SpeedComparer.Compare(b,a) > 0 is also true
class SpeedComparer : IComparer<Motorcycle>
{
    public int Compare(Motorcycle first, Motorcycle second)
    {
        //add null checks

        return first.Price.CompareTo(second.Price);
    }
}

class NameComparer : IComparer<Motorcycle>
{
    public int Compare(Motorcycle first, Motorcycle second)
    {
        //add null checks

        return first.Name.CompareTo(other.Name);
    }
}

Usage:

public void Test()
{
    var motorcycle1 = new Motorcycle("harley", 1000, 120);
    var motorcycle2 = new Motorcycle("yamaha", 1900, 130);
    var motorcycle3 = new Motorcycle("susuki", 1800, 150);
    var motorcycles = new List<Motorcycle>() { motorcycle1, motorcycle2, motorcycle3 };

    IComparer<Motorcycle> selectedComparer = new NameComparer();
    //compares by name
    TestComparison();

    selectedComparer = new SpeedComparer();
    //selectedComparer is changed, so now speeds are compared
    TestComparison();

    void TestComparison()
    {
        Debug.WriteLine($"Using {selectedComparer.GetType().Name}...");
        Debug.WriteLine(selectedComparer.Compare(motorcycle1, motorcycle2));
        motorcycles.Sort(selectedComparer);
        motorcycles.ForEach(m => Debug.WriteLine($"name {m.Name}   price {m.Price}   speed {m.Speed}"));
    }
}  
\$\endgroup\$
7
  • \$\begingroup\$ Thanks. MotorcCycle not know Comparison is what I need to do. I need to change out Comparison external. Users want to able to change out Comparison on the fly. Think designing a scoring criteria. And they also want to know when two Comparison do not give the same results. \$\endgroup\$ – paparazzo Jun 4 '18 at 16:47
  • \$\begingroup\$ I get not symmetric is a potential problem. Here is my question. How do you pass an IComparer to Compare. msdn.microsoft.com/en-us/library/8ehhxeaf(v=vs.110).aspx I am not seeing any overloaded method to pass an IComparer. In the documentation it says this interface is used with the List<T>.Sort and List<T>.BinarySearch methods. msdn.microsoft.com/en-us/library/8ehhxeaf(v=vs.110).aspx \$\endgroup\$ – paparazzo Jun 4 '18 at 18:10
  • \$\begingroup\$ @paparazzo Users want to able to change out Comparison on the fly - this scenario is usually handled by swapping IComparer<T> implementation. You don't have to pass comparer to Compare method, you should callIComparer<T>.Compare() directly. Basically, instead of a.CompareTo(b) and list.Sort(), you should call userSelectedComparer.Compare(a, b) and list.Sort(userSelectedComparer) respectively (where userSelectedComparer is IComparer<T> field, that user can change via UI somehow). \$\endgroup\$ – Nikita B Jun 4 '18 at 18:20
  • \$\begingroup\$ Not sure I follow but I will look into it. Thanks \$\endgroup\$ – paparazzo Jun 4 '18 at 19:03
  • 2
    \$\begingroup\$ @paparazzo It seems like you have misunderstood how comparers should be used, so I've added a usage example. \$\endgroup\$ – Nikita B Jun 5 '18 at 7:38
0
\$\begingroup\$

As pointed out by Nikita the problem is a.CompareTo(b) might not by symmetric with b.CompareTo(a). Some times users ask for stuff that just does not make sense. A reality is often it is just easier to do it with limitations.

I think I have something that will work:

public delegate int DelegateDeclaration(Car car1, Car car2);
// implementation
public static int DelegateImplementation1(Car car1, Car car2)
{
    return car1.HP.CompareTo(car2.HP);
}
public class Car : IComparer<Car>
{
    public IComparer<Car> Comparer { get; set; }
    public int Compare(Car car1, Car car2)
    {
        if (car1 == null)
        {
            throw new ArgumentNullException("car1");
        }
        if (car1 == null)
        {
            throw new ArgumentNullException("car1");
        }
        if (Comparer == null)
        {
            return car1.Name.CompareTo(car2.Name);
        }
        return Comparer.Compare(car1, car2);  //dangerous as they might not have the same Comparer
    }
    public int ComareTo(Car other)
    {
        return Compare(this, other);
    }
    public bool CompareSame(Car other)
    {
        return Compare(this, other) == -Compare(other, this);
    }
    public int ComareTo(Car other, DelegateDeclaration dgate)
    {
        return dgate(this, other);
    }
    public bool ComareSame(Car other, DelegateDeclaration dgate1, DelegateDeclaration dgate2)
    {
        return dgate1(this, other) == dgate2(this, other);
    }
    public String Name { get; }
    public int HP { get; set; }
    public Car(string name, int hp, IComparer<Car> comparer = null)
    {
        Name = name;
        HP = hp;
    }
}
\$\endgroup\$

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.