Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
    }
}
share|improve this question
    
Have you even tried your code? It won't work unless both values are decimal. Also, what about BigInteger? –  svick Mar 4 '13 at 16:03
5  
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. –  Dan Lyons Mar 4 '13 at 18:50
1  
Determine if type is numeric should be very helpful –  radarbob Mar 4 '13 at 20:38
1  
@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 –  rudimenter Mar 5 '13 at 13:54
1  
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. –  loki2302 Mar 7 '13 at 19:14

4 Answers 4

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.

share|improve this answer
    
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. –  rudimenter Mar 14 '13 at 14:29

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?

share|improve this answer

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.
share|improve this answer
    
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. –  svick Mar 4 '13 at 19:28
    
@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. –  Bobson Mar 4 '13 at 20:56

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)));
}
share|improve this answer
1  
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. –  Bobson Mar 4 '13 at 23:01

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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