-
Notifications
You must be signed in to change notification settings - Fork 19.8k
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
Add ReservoirSampling algorithm to randomized module #6204
base: master
Are you sure you want to change the base?
Conversation
What if sampleSize > stream.length? Perhaps there should be error handling for invalid input, the rest lgtm |
✅ Added input validation for Let me know if anything else is needed — happy to improve further 🙌 |
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 code looks good, could you please add some JUnit tests and remove main? You could check that the correct number of elements is returned, that they are all from the initial set, maybe other properties of the algorithm (see https://github.com/TheAlgorithms/Java/tree/master/src/test/java/com/thealgorithms)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6204 +/- ##
=========================================
Coverage 73.78% 73.79%
- Complexity 5299 5304 +5
=========================================
Files 671 672 +1
Lines 18344 18356 +12
Branches 3546 3549 +3
=========================================
+ Hits 13536 13545 +9
- Misses 4262 4264 +2
- Partials 546 547 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
✅ Applied clang-format and added JUnit test to All requested changes completed. Ready for final review 💪 |
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.
Code looks good, please fix PR checks and it's ready to merge. Thank you for patience, I'm very busy at the moment so response times are high :)
✅ Final test file now formatted with CI will re-run shortly. Appreciate all the support — ready for merge when you are 💪 |
Error: /home/runner/work/Java/Java/src/main/java/com/thealgorithms/randomized/ReservoirSampling.java:19:1: Utility classes should not have a public or default constructor. [HideUtilityClassConstructor] |
Algorithm Overview:
Implementation Details:
ReservoirSampling
com.thealgorithms.randomized
main()
methodclang-format
Reference:
Author: Michael Alexander Montoya (@cureprotocols)
clang-format -i --style=file path/to/your/file.java