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

Issue popping when using custom comparator #31

Closed
bjrnt opened this issue Apr 27, 2020 · 3 comments
Closed

Issue popping when using custom comparator #31

bjrnt opened this issue Apr 27, 2020 · 3 comments

Comments

@bjrnt
Copy link

bjrnt commented Apr 27, 2020

Hi,

Thanks for the library! I've had great use of it in a lot of online coding interviews. :)

Here's a quick reproduction of a bug I found when using custom comparators:

const heap = new Heap((a, b) => a[1] - b[1]);
heap.push([1, 2]);
heap.push([6, 0]);
heap.push([2, 3]);
heap.pop();
// => TypeError: Cannot read property '1' of undefined

After some digging, I found that inside getPotentialParent sometimes calls this.compare with an undefined value. Adding a quick typeof this.heapArray[j] !== "undefined" && to the function solves the issue but I am not sure if this is the best way to handle it. I'd be happy to send a PR with tests if you think this is a good solution to the problem.

@ignlg
Copy link
Owner

ignlg commented Apr 28, 2020

Hello @bjrnt, thanks for your feedback. I will try it within the next branch to be sure that it is not already fixed. Also I will add your example as a test :)

ignlg added a commit that referenced this issue Apr 28, 2020
Adds tests for custom heaps and comparators
@ignlg
Copy link
Owner

ignlg commented Apr 28, 2020

It should be fixed now.

To allow undefined as a valid value inside the heap, it checks that the index is not beyond length before any comparison occurs.

I have added tests for custom heaps with custom comparators, and improved the existing ones.

@bjrnt Could you try the next branch and let me know if it fixed the issue for you?

Thanks :)

@bjrnt
Copy link
Author

bjrnt commented Apr 29, 2020

Yep, working great now! Thanks for the quick fix! :)

@bjrnt bjrnt closed this as completed Apr 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants