Require Code Review of a Universal Number Comparer

Question

I wrote some code which compares numbers of all datatypes with each other. The standard Comparer which is implemented by every number Type only compares numbers of the same type.

I cast to decimal because this is the largest possible number type and should cover everything.

What do you think about the code? Room for improvement? How to handle NaN the best way? Do you think BigInt and Real numbers should be a part of it?

    internal class UniversalNumberComparer : IComparer
    {
        public int Compare(object x, object y)
        {
            if(!IsNumber(x) || !IsNumber(y))
                throw new InvalidOperationException();

            return x == null
               ? (y == null ? 0 : -1)
               : (y == null ? 1 : Decimal.Compare(Convert.ToDecimal(x), Convert.ToDecimal(y)));
        }

        private static readonly TypeCode[] _TypeCode ={TypeCode.Byte,
                                                TypeCode.Decimal,
                                                TypeCode.Double,
                                                TypeCode.Int16,
                                                TypeCode.Int32,
                                                TypeCode.Int64,
                                                TypeCode.SByte,
                                                TypeCode.Single,
                                                TypeCode.UInt16,
                                                TypeCode.UInt32,
                                                TypeCode.UInt64};

        private bool IsNumber(object value)
        {
            return (value == null) || (Array.IndexOf(_TypeCode, Type.GetTypeCode(value.GetType())) != -1);
        }
    }

Update:

Based on your Feedback i refined the UniversalNumberComparer.

Answer 1

Some thoughts:

• There's no point in testing whether x or y is null, because when null is passed, IsNumber() returns false.
• If you do want to keep that code, perhaps by modifying IsNumber() to accept null, then you should organize that if/else tree better. Standard structure is to either return from every branch of the tree, not return from the tree at all, or (for purposes of error checking) not have an else clause at all. You're using an if/else pair, with a nested ternary operator (? :) and another nested if. It's just a confusing jumble.
• Exhaustive lists like in IsNumber() are generally a bad idea, but in this case I think it's acceptable. I have no opinion between lots of individual test/return pairs vs a single giant test. However, there's no point to testing the nullable cases - once you've converted to an object ("boxing"), a null value looses the type it had had, and anything else is simply the type of its value. You would have to explicitly cast it back to a nullable in order to set it to null.

Answer 2

As for your question about NaN: this is also a situation where conversion to decimal is likely to be problematic. Assuming that you resolve that problem, the question then is what to do about it in the comparison.

It depends on the goals of your comparison. The semantics of NaN are "this thing isn't even a number, so asking which one is bigger is a nonsensical question", and therefore any operation on NaN is false. That means that NaN is not greater than any number, not smaller than any number and not equal to any number, including itself. If that's acceptable for your comparison purposes then that's the semantics you should implement.

Those semantics are not acceptable for sorting. If the comparison is being used in a comparison sort then the comparison is required to produce a total consistent order. It must be reflexive -- every number must equal itself. It must be antisymmetric -- if a < b is true then b < a must be false. And it must be transitive -- if a < b is true and b < c is true then a < c must be true. The usual way to deal with NaNs is to say that all NaNs are equal to each other -- achieving reflexivity, and that all NaNs are smaller than every non-NaN -- achieving antisymmetry and transitivity.

And don't forget that you'll need to deal with positive and negative infinities.

Answer 3

I cast to decimal because this is the largest possible number type and should cover everything.

Since this statement uses fallacious reasoning, and the correctness of the code depends on this statement being sound, the code is unlikely to be correct. Just because decimal takes up the most bytes doesn't mean that every value of every other numeric type can be represented in decimal.

Presumably you would like your numeric comparer to have the property that if, say, 10²⁰⁰ and 10²¹⁰ are passed in as doubles, that the one which is ten billion times larger would be considered larger. What actually happens?

And presumably you would like to have the property that if say, 10^-200 and 10^-210 are passed in as doubles, that the one which is ten billion times smaller would be considered smaller. What actually happens?

Answer 4

Based on the comment of "radarbob", you could redefine your IsNumber() to:

private static bool IsNumber(object value)
{
    return TypeCodes.Contains(Type.GetTypeCode(value.GetType()));
}

private static readonly List<TypeCode> TypeCodes = new List<TypeCode>
{
    TypeCode.Byte,
    TypeCode.Decimal,
    TypeCode.Double,
    TypeCode.Int16,
    TypeCode.Int32,
    TypeCode.Int64,
    TypeCode.SByte,
    TypeCode.Single,
    TypeCode.UInt16,
    TypeCode.UInt32,
    TypeCode.UInt64,
};

And as "Bobson" mentioned, mixing the ternary operator and nested if/else structures is messy. Keep it with an if/else structure or you could try this:

public int Compare(object x, object y)
{
    if(!IsNumber(x) || !IsNumber(y))
        throw new InvalidOperationException();

    return x == null
        ? (y == null ? 0 : -1)
        : (y == null ? 1 : Decimal.Compare(Convert.ToDecimal(x), Convert.ToDecimal(y)));
}