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

Removed array Insertion and added numbers Fixes: #1341 #1372

Closed
wants to merge 1 commit into from
Closed

Removed array Insertion and added numbers Fixes: #1341 #1372

wants to merge 1 commit into from

Conversation

ROHAN13498
Copy link
Contributor

Open in Gitpod know more

Fixes: #1341

Describe your change:

  • Add an algorithm?
  • Fix a bug or typo in an existing algorithm?
  • Documentation change?

Checklist:

  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized.
  • I know that pull requests will not be merged if they fail the automated tests.
  • This PR only changes one algorithm file. To ease review, please open separate PRs for separate algorithms.
  • All new JavaScript files are placed inside an existing directory.
  • All filenames should use the UpperCamelCase (PascalCase) style. There should be no spaces in filenames.
    Example:UserProfile.js is allowed but userprofile.js,Userprofile.js,user-Profile.js,userProfile.js are not
  • All new algorithms have a URL in their comments that points to Wikipedia or another similar explanation.
  • If this pull request resolves one or more open issues then the commit message contains Fixes: #{$ISSUE_NO}.

Copy link
Collaborator

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That comment should probably be removed or turned into a test case. Either way this change is an improvement already though.

@raklaptudirm
Copy link
Member

@ROHAN13498 are you interested in implementing the test cases? Cause I see no point in improving upon something that is explicitly disallowed by the contribution guidelines.

@ROHAN13498
Copy link
Contributor Author

Done. I will work on it.

@ROHAN13498
Copy link
Contributor Author

@ROHAN13498 are you interested in implementing the test cases? Cause I see no point in improving upon something that is explicitly disallowed by the contribution guidelines.

Hey,There is no testcases file for maxheap should I create a newfile and work on it?

image

@raklaptudirm
Copy link
Member

@ROHAN13498 are you interested in implementing the test cases? Cause I see no point in improving upon something that is explicitly disallowed by the contribution guidelines.

Hey,There is no testcases file for maxheap should I create a newfile and work on it?

image

Yes you should.

@appgurueu
Copy link
Collaborator

The current situation is a bit awkward; we have a max heap, a min heap, and a priority queue, all of which are more or less based on similar code. This should ideally be deduplicated; max and min heap can effectively use the same code with a different comparison function, and a priority queue should just add an index from element to position in heap on top of that.

@ROHAN13498
Copy link
Contributor Author

The current situation is a bit awkward; we have a max heap, a min heap, and a priority queue, all of which are more or less based on similar code. This should ideally be deduplicated; max and min heap can effectively use the same code with a different comparison function, and a priority queue should just add an index from element to position in heap on top of that.

So you are suggesting that Instead of having separate implementations for Max Heap and Min Heap, it's better to create a single generic heap data structure that can handle both max and min heaps by using a comparison function to determine the order of elements.

Should I work on implementing this single heap data structure or write tests for max heap?

@appgurueu
Copy link
Collaborator

appgurueu commented Oct 3, 2023

So you are suggesting that Instead of having separate implementations for Max Heap and Min Heap, it's better to create a single generic heap data structure that can handle both max and min heaps by using a comparison function to determine the order of elements.

Yes (though my suggestion goes even further: we could deduplicate some heap and priority queue code by making the priority queue extend or use the heap, but you don't need to do this because it is trickier).

Should I work on implementing this single heap data structure or write tests for max heap?

The former; the latter would pretty much be manual test duplication, which seems pretty unnecessary to me.

@ROHAN13498
Copy link
Contributor Author

So you are suggesting that Instead of having separate implementations for Max Heap and Min Heap, it's better to create a single generic heap data structure that can handle both max and min heaps by using a comparison function to determine the order of elements.

Yes (though my suggestion goes even further: we could deduplicate some heap and priority queue code by making the priority queue extend or use the heap, but you don't need to do this because it is trickier).

Should I work on implementing this single heap data structure or write tests for max heap?

The former; the latter would pretty much be manual test duplication, which seems pretty unnecessary to me.

I'll start by working on test cases file for maxheap data structure for now . Once that's in place, I'll try to tackle code deduplication.

@appgurueu
Copy link
Collaborator

I'll start by working on test cases file for maxheap data structure for now . Once that's in place, I'll try to tackle code deduplication.

As said, I don't think that's a good idea. You will be duplicating min heap tests effectively.

@ROHAN13498 ROHAN13498 closed this by deleting the head repository Oct 9, 2023
@ROHAN13498
Copy link
Contributor Author

ROHAN13498 commented Oct 9, 2023

Hey @appgurueu @raklaptudirm I have deduplicated the codes of min heap and maxheap into single binaryheap class should I create a issue for it and create a PR or should I do PR directly.

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.

[OTHER]: why element is array in Max Heap?
4 participants