Make "hybrid" sort algorithm more robust. #360
Conversation
Add bounds checking so that we don't make so many assumptions about how the comparator behaves. Make the "median of three" algorithm actually take the median of three.
| private static int median(int n1, int n2, int n3) | ||
| /* | ||
| Return the index of the smallest of three elements in the specified array range -- the | ||
| first, the last, and the one in the middle. |
There was a problem hiding this comment.
Is this correct? From my reading median() returns the index of the median of three elements (start, end and in the middle), not the smallest in range.
| return 0; | ||
| }); | ||
|
|
||
| // Torture tests devised by @sainen and @bensummers |
There was a problem hiding this comment.
Thanks, but I'd say that attribution isn't in necessary/useful here. It's just a test, plus comparator itself comes from underscore. (And it's sainaen. 🙂 )
| if (a < b || b === void 0) return -1; | ||
| } | ||
| return left.index < right.index ? -1 : 1; | ||
| }); |
There was a problem hiding this comment.
Since both these ‘torture’ cases are basically the same, how about replacing one of them with something different that also throws currently?
Like,
// should work ok, comparator is similar to the underscore's one
sortAndCompare([1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 1, 1, 1, 1, 1, 1, 1, 1, 1], function (a, b) { return a >= b ? 1 : -1; });
// weird comparator, array may not be sorted, but shouldn't fail/throw
assertDoesNotThrow(function () {
[1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 1, 1, 1, 1, 1, 1, 1, 1, 1].sort(function (a, b) { return -1; });
});
// yet another weird comparator, again shouldn't fail/throw
assertDoesNotThrow(function () {
[1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 1, 1, 1, 1, 1, 1, 1, 1, 1].sort(function (a, b) { return 1; });
});
|
Thanks for the review and suggestions! I kind of think that if someone sends us code and they're not going to be part of the Git commit then I should credit them ;-) I'll merge this soon. |
Add bounds checking so that we don't make so many assumptions about
how the comparator behaves.
Make the "median of three" algorithm actually take the median
of three.