Combined Min Heap and Max Heap classes#1494
Combined Min Heap and Max Heap classes#1494raklaptudirm merged 4 commits intoTheAlgorithms:masterfrom
Conversation
appgurueu
left a comment
There was a problem hiding this comment.
Looks mostly fine. Some points:
- The tests are pretty much duplicated. IMO it would be fine to test just min or max heap, then test that the comparator is being used at all with a small example of the opposite ordering. But I'm not terribly opposed to this; it needs to be tested one way or another.
- You got rid of explanatory comments. Please readd some. At the very least, please add JSDoc comments to public methods, and mark "private" methods (like
bubbleUp,sinkDownandswap) as such using the#prefix.
|
Yeah!You are right.I will work on that and get back to you. |
I assume the @Class tag is for classes implemented via constructor functions, not using ES6 class syntax
appgurueu
left a comment
There was a problem hiding this comment.
Though you have some redundant tags there (e.g. there's no need for @private for #methods, no need to document the heap member (which btw should maybe also be private)).
|
I did this under hactober fest will it be counted? |
Yes, it should be. From the Hacktoberfest website:
The last point is what I effectively just did :) |
Thanks a lot @appgurueu |
np, thank you for your contribution :) it's important that every now and then someone takes the time to refactor something |
|
@appgurueu are there any more issues that you would suggest me to work on? |
No, but if you see other places where code is duplicated (or that otherwise have low code quality and warrant a refactor - plenty of such cases in this repo), or want to add an interesting algorithm, feel free to open a PR :) |
Deduplicated the code of Minheap and Maxheap and used a single binary heap class.
Describe your change:
Checklist:
**Example:**UserProfile.js is allowed but userprofile.js,Userprofile.js,user-Profile.js,userProfile.js are not