5
\$\begingroup\$

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.

  1. What do you think about the code?
  2. Is there any room for improvement?
  3. How can I handle NaN the best way?
  4. Do you think BigInt and Real numbers should be a part of it?

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

        if(x == null) 
        {
            return y == null
                ? 0
                : -1;
        }
        else
        {
            if(y == null)
                return 1;
        }

        return Decimal.Compare(Convert.ToDecimal(x), Convert.ToDecimal(y));
    }

    private bool IsNumber(object value)
    {
        if(value is sbyte || value is sbyte?)
            return true;
        if(value is byte || value is byte?)
            return true;
        if(value is short || value is short?)
            return true;
        if(value is ushort || value is ushort?)
            return true;
        if(value is int || value is int?)
            return true;
        if(value is uint || value is uint?)
            return true;
        if(value is long || value is long?)
            return true;
        if(value is ulong || value is ulong?)
            return true;
        if(value is float || value is float?)
            return true;
        if(value is double || value is double?)
            return true;
        if(value is decimal || value is decimal?)
            return true;
        return false;
    }
}
\$\endgroup\$
9
  • \$\begingroup\$ Have you even tried your code? It won't work unless both values are decimal. Also, what about BigInteger? \$\endgroup\$
    – svick
    Commented Mar 4, 2013 at 16:03
  • 5
    \$\begingroup\$ Where do you see yourself needing this object? Generally, you should be comparing two numbers of the same type. Exceptions to this rule should be few enough that you should handle (and comment) it where needed rather than coming up with a general-purpose class. Needing to create a general-purpose class to compare separate number types, particularly in a type-unsafe manner reeks of code smells elsewhere. \$\endgroup\$
    – Dan Lyons
    Commented Mar 4, 2013 at 18:50
  • 1
    \$\begingroup\$ Determine if type is numeric should be very helpful \$\endgroup\$
    – radarbob
    Commented Mar 4, 2013 at 20:38
  • 1
    \$\begingroup\$ @DanLyons The point is to have a comparer for a generic list<object> which orders the content by the values of the numbers. And... in an Assert Helper class wich test if a number value is in between 2 other values without knowing the type and without writing tons of overloads and generics make no much sense here either \$\endgroup\$
    – rudimenter
    Commented Mar 5, 2013 at 13:54
  • 1
    \$\begingroup\$ Whenever you write code, there should be a reason to do it. Treating the code as good or bad, or considering possible improvements only makes sense if there's context of problem you're trying to solve. If there's not problem your code solves, it's just useless. \$\endgroup\$ Commented Mar 7, 2013 at 19:14

4 Answers 4

6
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks Eric. I already felt that casting to decimal is not the way to go. The Comparer as i posted does it for me at the moment and my project. BUT i really want to make it work considering all possible cases (Negatives, NAN, nullables,...). To be honest, i expected this task as much easier then it turned out to be. \$\endgroup\$
    – rudimenter
    Commented Mar 14, 2013 at 14:29
5
\$\begingroup\$

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, 10200 and 10210 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?

\$\endgroup\$
3
\$\begingroup\$

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.
\$\endgroup\$
2
  • \$\begingroup\$ I think the point about is int? could use some elaboration: There is no such thing as boxed nullable value. Boxing (i.e. converting to object) of nullable value results either in null or in the underlying type (e.g. int) boxed. \$\endgroup\$
    – svick
    Commented Mar 4, 2013 at 19:28
  • \$\begingroup\$ @svick - Good to know. I knew that it behaved that way, but I had no idea why. I'll edit in a bit more about that. \$\endgroup\$
    – Bobson
    Commented Mar 4, 2013 at 20:56
0
\$\begingroup\$

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)));
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ I don't find that use of ternary particularly clear, but it's still better than the mess of conventions in the OP. It's easy enough to read through it and see what it does, but it isn't intuitively obvious. \$\endgroup\$
    – Bobson
    Commented Mar 4, 2013 at 23:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.