Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update bubble-sort and radix-sort solutions #7

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

nielsboecker
Copy link

Hi Brian, brilliant course as always, thanks ♡

Bubble sort

Changed index to go only to length - 1. It worked to go up to length, but only implicitly because 10 > undefined === false

    Compare 7 and 8
    Compare 8 and 9
    Compare 9 and 10
    Compare 10 and undefined

Radix sort

The second test in solution works, but only because the input array and the expected output are in fact references to the same array. array.sort() actually performs string-based alphabetic sorting, so this is not what we would want (even though the algorithm is correct).

[1, 10, 100, 1001, 11, 22, 2].sort()
// [1, 10, 100, 1001, 11, 2, 22]

@marr
Copy link

marr commented May 9, 2022

This PR appears to be doing more than described in the description. Maybe its worth a clean up?
I also discovered the radix sort issue, and came to make a PR for that.

@btholt I see are other possible PRs that address that radix sort but do so without copying the nums array. I feel those are insufficient since radix sort is destructive and thus sort the input array. @nielsboecker solution does copy the array before sorting which I believe is correct.

Other PRs that are testing on an already sorted array (and thus insufficient):
#9
#12
#13
#15

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants