Add proper tests for binary search #987
Conversation
There was a problem hiding this comment.
How does this compare to the current implementation? From what I can see it's merely a less readable iterative version. If it showed better traits than the current one, you may replace it. But I don't see the merit of two largely similar iterative implementations.
|
@appgurueu since it is being added as a separate implementation, I think we could add it. It is using some more js features, which could be wanted. What do you think? |
There was a problem hiding this comment.
This PR may indeed be using more JS features (primarily ternaries) but in a wrong manner; they only make the code less readable (except probably for the last ternary). Currently I see only two very tiny advantages over the other impl., the simplification of the term for mid and moving the check if found out of the loop; I don't see much of a benefit from the exclusive low & high indices, which is a mere convention. On the other hand however there is rather unreadable code, bad comments and syntactic sugar where it doesn't belong.
The tiny improvements should just be changes to the current implementation.
appgurueu
added
feature
|
At this point it's similar enough that you should just tidy up the existing implementation (adding proper jest tests) instead. |
|
I did this |
There was a problem hiding this comment.
From a quick glance the tests look fine to me. Definitely a significant improvement. But please deduplicate the code; you can do something like funcs = {iterative: binarySearchIterative, recursive: binarySearchRecursive}; for (let name in funcs) runTests(name, funcs[name]) where runTests is a parameterized function that runs the tests (which are the same for both implementations).
|
Nice vision, i will make this! |
|
lgtm now, what you think? |
appgurueu
changed the title
Welcome to the JavaScript community
Describe your change:
Checklist:
Example:
UserProfile.jsis allowed butuserprofile.js,Userprofile.js,user-Profile.js,userProfile.jsare not