Further improve sorting algorithm to put undefined values at end of list #376
Conversation
| @@ -44,7 +44,7 @@ module('Integration | Helper | {{sort-by}}', function(hooks) { | |||
| {{~/each~}} | |||
| `); | |||
|
|
|||
| assert.equal(find('*').textContent.trim(), 'aAAabccb', 'sorts multiletter words'); | |||
| assert.equal(find('*').textContent.trim(), 'AaaAbccB', 'sorts multiletter words'); | |||
There was a problem hiding this comment.
We aren't splitting the strings up by individual char, so I actually think this is now "more" right.
| return false; | ||
| } else if (val < 0) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
We shouldn't swap if val === 0, thus the need for this refactor
There was a problem hiding this comment.
Ah! Yes! This is what I could NOT seem to get my head around and fix. Yay!
|
@jrjohnson Can I get your careful eye on this PR? Thank you!! |
| return false; | ||
| } else if (val < 0) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Ah! Yes! This is what I could NOT seem to get my head around and fix. Yay!
| @@ -17,6 +21,15 @@ function sortDesc(key, a, b) { | |||
| const aValue = get(a, key); | |||
| const bValue = get(b, key); | |||
|
|
|||
| if (typeof bValue == 'undefined') { | |||
There was a problem hiding this comment.
This makes more sense to me than my fix.
addon/helpers/sort-by.js
Outdated
| // put aValue last | ||
| return 1; | ||
| } | ||
|
|
||
| if (aValue && bValue && aValue.toLowerCase && bValue.toLowerCase) { |
There was a problem hiding this comment.
aValue && bValue && can now be removed here as it was just to catch undefined. I think everything else can have toLowerCase check called on it.
addon/helpers/sort-by.js
Outdated
| // put aValue last | ||
| return 1; | ||
| } | ||
|
|
||
| if (aValue && bValue && aValue.toLowerCase && bValue.toLowerCase) { |
There was a problem hiding this comment.
Same, aValue && bValue && can go away now that undfined check is above.
|
Thank you for the awesome review!! 4.2.1 released!! |
close #374