-
-
Notifications
You must be signed in to change notification settings - Fork 250
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
feat: add binary insertion sort algorithm #71
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have a binary search implementation, so use that instead of making your own.
Hi @raklaptudirm, the existing binary search will return -1 if we don't find the element. But my modified version find the position of the element that is to be inserted in the array. So I cant reuse it. |
Ah, makes sense. |
@appgurueu Can you please review this PR |
You could consider updating the existing binary search. |
@appgurueu See the actuall Binary Search should return -1 if the key is not found. But in my function, I will be returning the insertion position of the key element. |
No. Our current implementation behaves this way because it is lazily written. More advanced implementations return the insertion or occurrence index; it is then trivial to check whether the element was found ( |
@appgurueu yeah, correct. How to proceed mate ? Do i need to close this PR and create a new one for Binary Search or Include the fix for binary search in this PR itself. ? |
I'd be fine with including the extension/feature (not a fix, the current implementation is correct) in this PR. |
@appgurueu I have created a PR for Binary Search Improvement |
@appgurueu What about this PR? I think this can be closed |
It can be closed, yes, but why should it be closed? It would add something if it were merged. It's just that it currently depends on #72. The author should be able pick this up at any time. |
An sorry, I understood this wrongly. I thought this PR was canceled |
fb7b311
to
d4d9abb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The binary search is still duplicated locally.
No description provided.