What am I doing wrong?
You are doing at least seven things wrong.
First, you are missing two return
statements.
Second, since you did not notice that you are missing two return
statements, odds are good that you are missing test cases that would exercise those code paths.
Third, DRY out your code. Don't Repeat Yourself. Rewrite this code so that you call int.TryParse
twice, not eight times.
Fourth, never use ArrayList
in code that was written after, let's say 2005. Use List<string>
.
Fifth, the return 0
is unreachable. Unreachable code is a bad practice. Rewrite the method so that every line is reachable.
Sixth, ArrayList.Sort
doesn't take an IComparer<string>
Seventh, comparers are required to handle nulls. It's a good idea to handle thoses cases explicitly so that you don't dereference them accidentally. Traditionally the way to go is to say that nulls are smaller than everything else.
Eighth, though it is not wrong to return any old number, it is my practice when writing a comparer to always return 0, 1 or -1.
Ninth, it is a good practice to take an "early out" in the case where the strings are reference equals. That case is extremely fast so if you can take it, you should.
I would be inclined to write this code like this:
static int? MyParse(string s)
{
int parsed;
bool isValid = int.TryParse(s, out parsed);
return isValid ? (int?)parsed : (int?) null;
}
public int Compare(string x, string y)
{
if (ReferenceEquals(x, y)) return 0;
if (ReferenceEquals(x, null)) return -1;
if (ReferenceEquals(y, null)) return 1;
// We now know that neither is null.
int? intX = MyParse(x);
int? intY = MyParse(y);
if (!intX.HasValue && intY.HasValue) return -1;
if (intX.HasValue && !intY.HasValue) return 1;
// We now know that intX.HasValue == intY.HasValue
int result = intX.HasValue ? intX.Value.CompareTo(intY.Value) : x.CompareTo(y);
if (result < 0) return -1;
if (result > 0) return 1;
return 0;
}