23

I am getting strange behaviour using the built-in C# List.Sort function with a custom comparer.

For some reason it sometimes calls the comparer class's Compare method with a null object as one of the parameters. But if I check the list with the debugger there are no null objects in the collection.

My comparer class looks like this:

public class DelegateToComparer<T> : IComparer<T>
{
    private readonly Func<T,T,int> _comparer;

    public int Compare(T x, T y)
    {
        return _comparer(x, y);
    }

    public DelegateToComparer(Func<T, T, int> comparer)
    {
        _comparer = comparer;
    }
}

This allows a delegate to be passed to the List.Sort method, like this:

mylist.Sort(new DelegateToComparer<MyClass>(
    (x, y) => { 
         return x.SomeProp.CompareTo(y.SomeProp); 
     });

So the above delegate will throw a null reference exception for the x parameter, even though no elements of mylist are null.

UPDATE: Yes I am absolutely sure that it is parameter x throwing the null reference exception!

UPDATE: Instead of using the framework's List.Sort method, I tried a custom sort method (i.e. new BubbleSort().Sort(mylist)) and the problem went away. As I suspected, the List.Sort method passes null to the comparer for some reason.

cbp
  • 25,252
  • 29
  • 125
  • 205
  • 2
    Re the edit - I don't suppose you have anything reproducible we can look at? (btw, if it was you - was a downvote really warranted?) – Marc Gravell Jun 23 '09 at 06:57
  • 1
    Agreed - a short but complete program reproducing the problem would be very handy. I very much doubt that this is a bug in List.Sort. – Jon Skeet Jun 23 '09 at 10:00

7 Answers7

28

This problem will occur when the comparison function is not consistent, such that x < y does not always imply y < x. In your example, you should check how two instances of the type of SomeProp are being compared.

Here's an example that reproduces the problem. Here, it's caused by the pathological compare function "compareStrings". It's dependent on the initial state of the list: if you change the initial order to "C","B","A", then there is no exception.

I wouldn't call this a bug in the Sort function - it's simply a requirement that the comparison function is consistent.

using System.Collections.Generic;

class Program
{
    static void Main()
    {
        var letters = new List<string>{"B","C","A"};

        letters.Sort(CompareStrings);
    }

    private static int CompareStrings(string l, string r)
    {
        if (l == "B")
            return -1;

        return l.CompareTo(r);
    }
}
Lee
  • 1,576
  • 2
  • 15
  • 21
  • 1
    In VB.NET this doesn't throw an error. How weird is that? – Fabian Bigler Sep 25 '14 at 07:11
  • 2
    not a bug, but would be nice if the exception would be "InconsistentComparisionMethodException" instead of standard null pointer ex. when there are no null values in the array... very confusing – serine Feb 18 '15 at 15:04
5

Are you sure the problem isn't that SomeProp is null?

In particular, with strings or Nullable<T> values.

With strings, it would be better to use:

list.Sort((x, y) => string.Compare(x.SomeProp, y.SomeProp));

(edit)

For a null-safe wrapper, you can use Comparer<T>.Default - for example, to sort a list by a property:

using System;
using System.Collections.Generic;
public static class ListExt {
    public static void Sort<TSource, TValue>(
            this List<TSource> list,
            Func<TSource, TValue> selector) {
        if (list == null) throw new ArgumentNullException("list");
        if (selector == null) throw new ArgumentNullException("selector");
        var comparer = Comparer<TValue>.Default;
        list.Sort((x,y) => comparer.Compare(selector(x), selector(y)));
    }
}
class SomeType {
    public override string ToString() { return SomeProp; }
    public string SomeProp { get; set; }
    static void Main() {
        var list = new List<SomeType> {
            new SomeType { SomeProp = "def"},
            new SomeType { SomeProp = null},
            new SomeType { SomeProp = "abc"},
            new SomeType { SomeProp = "ghi"},
        };
        list.Sort(x => x.SomeProp);
        list.ForEach(Console.WriteLine);
    }
}
Marc Gravell
  • 1,026,079
  • 266
  • 2,566
  • 2,900
  • 1
    Sorry it is definitely parameter x that is null, not its property. I do not want this to be null safe - it should not be null. – cbp Jun 23 '09 at 00:32
5

I too have come across this problem (null reference being passed to my custom IComparer implementation) and finally found out that the problem was due to using inconsistent comparison function.

This was my initial IComparer implementation:

public class NumericStringComparer : IComparer<String>
{
    public int Compare(string x, string y)
    {
        float xNumber, yNumber;
        if (!float.TryParse(x, out xNumber))
        {
            return -1;
        }
        if (!float.TryParse(y, out yNumber))
        {
            return -1;
        }
        if (xNumber == yNumber)
        {
            return 0;
        }
        else
        {
            return (xNumber > yNumber) ? 1 : -1;
        }
    }
}

The mistake in this code was that Compare would return -1 whenever one of the values could not be parsed properly (in my case it was due to wrongly formatted string representations of numeric values so TryParse always failed).

Notice that in case both x and y were formatted incorrectly (and thus TryParse failed on both of them), calling Compare(x, y) and Compare(y, x) would yield the same result: -1. This I think was the main problem. When debugging, Compare() would be passed null string pointer as one of its arguments at some point even though the collection being sorted did not cotain a null string.

As soon as I had fixed the TryParse issue and ensured consistency of my implementation the problem went away and Compare wasn't being passed null pointers anymore.

sirobee
  • 51
  • 1
  • 1
0

Marc's answer is useful. I agree with him that the NullReference is due to calling CompareTo on a null property. Without needing an extension class, you can do:

mylist.Sort((x, y) => 
      (Comparer<SomePropType>.Default.Compare(x.SomeProp, y.SomeProp)));

where SomePropType is the type of SomeProp

Matthew Flaschen
  • 278,309
  • 50
  • 514
  • 539
0

For debugging purposes, you want your method to be null-safe. (or at least, catch the null-ref. exception, and handle it in some hard-coded way). Then, use the debugger to watch what other values get compared, in what order, and which calls succeed or fail.

Then you will find your answer, and you can then remove the null-safety.

abelenky
  • 63,815
  • 23
  • 109
  • 159
0

Can you run this code ...

mylst.Sort((i, j) =>
              {
                  Debug.Assert(i.SomeProp != null && j.SomeProp != null);
                  return i.SomeProp.CompareTo(j.SomeProp);
              }
         );
JP Alioto
  • 44,864
  • 6
  • 88
  • 112
0

I stumbled across this issue myself, and found that it was related to a NaN property in my input. Here's a minimal test case that should produce the exception:

public class C {

    double v;

    public static void Main() {
        var test =
            new List<C> { new C { v = 0d },
                          new C { v = Double.NaN },
                          new C { v = 1d } };
        test.Sort((d1, d2) => (int)(d1.v - d2.v));
    }

}
Derek Slager
  • 13,619
  • 3
  • 34
  • 34