compare_array implementation is confusing

Bug #1821032 reported by a. bellenir on 2019-03-20
This bug affects 1 person
Affects Status Importance Assigned to Milestone

Bug Description

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.

Revision history for this message
a. bellenir (abellenir) wrote :
tags: added: pullrequest webstaffclient
Revision history for this message
Dan Wells (dbw2) wrote :

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.

Revision history for this message
a. bellenir (abellenir) wrote :

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."

MDN says "If compareFunction(a, b) is less than 0, ..." and "If compareFunction(a, b) is greater than 0, ..."

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

Andrea Neiman (aneiman) on 2021-03-18
tags: removed: webstaffclient
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers