compare_array implementation is confusing

[a. bellenir]

the existing implementation works but it can be written much more concisely.

because it uses localeCompare, the suggested new implementation will only work on arrays of strings. it can be modified to work on arrays of numbers too if needed.

feel free to mark this bug invalid or wontfix if this is not considered an improvement.

[a. bellenir (abellenir)]

here's a branch with a suggested implementation.

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/abellenir/lp1821032-compare_array-implmenetation-is-confusing

[Dan Wells (dbw2)]

I am big fan of code refactoring where it is needed, so please don't let the following discourage you!

While the proposed change is clever, in this case, I actually find the original code quite a bit easier to read and understand. It also completely self-contained, so any refactoring will not have ripple benefits into the bigger picture. My biggest concern, though, is using length subtraction to return the sort value when the lengths are different. If the arrays are one apart, it will return 1 or -1, but it can also return 2 or -2, 3, or -3, etc. This will *probably* work like the expected values in many cases, but it feels like a recipe for trouble.

Just my two cents, and I am definitely not marking this as 'invalid' in case it tickles someone else's fancy.

[a. bellenir (abellenir)]

readability is highly subjective and i don't mind if y'all prefer the original implementation.

> My biggest concern, though, is [unexpected behavior if the comparison function returns values other than -1, 0, or 1]

fear not. javascript doesn't care what the value really is, just if it's bigger than, less than, or equal to zero.
the spec says: "If comparefn is not undefined, it should be a function that accepts two arguments x and y and returns a negative value if x < y, zero if x = y, or a positive value if x > y."
https://www.ecma-international.org/ecma-262/5.1/#sec-15.4.4.11

MDN says "If compareFunction(a, b) is less than 0, ..." and "If compareFunction(a, b) is greater than 0, ..."
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort

floating point math *can* get weird if you're returning values *very* close to zero and your numbers lose precision (but we're not doing this): 10**-1000 === 0 //=> true

[Jason Boyer (jboyer)]

Concision is not necessarily a goal in itself, though it can sometimes help reign in a meandering function. Most of the confusion around how compare_array is currently implemented can probably be cleared up with a comment explaining the fall-through that finally results in hitting the 'return 0;'
It may be that using localeCompare is a route worth exploring since it is currently called on a list of strings, but that discussion is better had in context of the Angular client than the AngularJS client.