alalonde / angular-scrollable-table

A fixed header table directive for AngularJS
MIT License
68 stars 48 forks source link

Default comparison used for sorting does not handle undefined values #65

Closed piotr-dobrogost closed 5 years ago

piotr-dobrogost commented 5 years ago

Array.prototype.sort() has the following guarantee:

If compareFunction is not supplied, (…) All undefined elements are sorted to the end of the array.

For example:

> ['c', null, null, 'a', undefined, 'b'].sort()
< (6) ["a", "b", "c", null, null, undefined]

However, default comparison function used by the library – defaultCompare – does not make this guarantee:

> cmp = function (a, b) {
    if (a === b) return 0;
    return a > b ? 1 : -1;
}
> ['c', null, null, 'a', undefined, 'b'].sort(cmp)
< (6) ["a", "b", null, null, "c", undefined]

which is unfortunate as it makes default sorting broken in case of any undefined values. The fix should be easy: change current

    if (x === y) return 0;
    return x > y ? 1 : -1;

to

    if (x === y) return 0;
    if ((x == null) && (y != null)) return 1;
    if ((x != null) && (y == null)) return -1;
    return x > y ? 1 : -1;

which gives:

> ['c', null, null, 'a', undefined, 'b'].sort(cmp)
< (6) ["a", "b", "c", null, null, undefined]
alalonde commented 5 years ago

See my comment in #60 .

piotr-dobrogost commented 5 years ago

@alalonde

I can not image that anyone would be negatively surprised that sorting ['c', null, null, 'a', undefined, 'b'] he would now get sorted sequence ["a", "b", "c", null, null, undefined] instead of random mess ["a", "b", null, null, "c", undefined] they have been getting earlier. All who wanted to handle undefined values must have already used custom comparator and for all others the behavior would be clearly better. Could you please reconsider merging this fix?