Feature: Random package #31
Conversation
WalkthroughThe changes introduce a new package Changes
Possibly related issues
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (5)
rand/rand.go (3)
43-43: Fix typos in function documentation.There are typos in the documentation:
- Line 43: "using the both default cons" should be "using both default constants"
- Line 95: "using both defautl const" should be "using both default constants"
Also applies to: 95-95
60-60: Fix typo in error message."generaet" should be "generate"
1-116: Consider adding memory zeroing for sensitive applications.While the implementation is secure using crypto/rand, for highly sensitive applications (e.g., generating cryptographic keys), consider adding memory zeroing after use.
You might want to add a note in the package documentation about this consideration, and potentially provide specialized functions for sensitive use cases that implement secure cleanup.
rand/rand_test.go (2)
28-70: Add edge cases to strengthen the test coverage.The test structure is good, but consider adding these test cases:
- Boundary values (when result equals min or max)
- Integer overflow scenarios
- Large ranges
Add these test cases to the test suite:
tests := []struct { name string min int64 max int64 wantErr bool }{ // ... existing cases ... + { + name: "success - large range", + min: math.MinInt64, + max: math.MaxInt64, + wantErr: false, + }, + { + name: "success - boundary test", + min: -1, + max: 1, + wantErr: false, + }, }
200-253: Add comprehensive charset test cases.Consider adding these scenarios:
- Very long string generation
- Unicode character support
- Repeated characters in charset
- Single character charset
Add these test cases:
tests := []struct { // ... existing cases ... + { + name: "success - very long string", + length: 10000, + charset: "abc", + wantLen: 10000, + wantErr: false, + }, + { + name: "success - unicode charset", + length: 10, + charset: "🎲🎮🎯", + wantLen: 10, + wantErr: false, + }, + { + name: "success - repeated charset chars", + length: 10, + charset: "aaa", + wantLen: 10, + wantErr: false, + }, + { + name: "success - single char charset", + length: 10, + charset: "x", + wantLen: 10, + wantErr: false, + }, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
rand/rand.go(1 hunks)rand/rand_test.go(1 hunks)
🔇 Additional comments (2)
rand/rand_test.go (2)
1-7: LGTM!
Package declaration and imports are appropriate for the test file.
94-134: LGTM!
TestStringWithLength has good coverage of edge cases and proper error handling.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Kashif Khan <[email protected]>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Kashif Khan <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (1)
rand/rand.go (1)
18-24: Consider the range of values returned byNumber().The
Number()function returns values from0tomath.MaxInt64 - 1. If the intention is to includemath.MaxInt64, consider adjusting the implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
rand/rand.go(1 hunks)
🔇 Additional comments (3)
rand/rand.go (3)
75-87: Ensure consistent handling of empty slices in Pick() and Shuffle().
While Pick() returns an error when the slice is empty, Shuffle() did not handle this case. With the updated check, both functions now consistently handle empty slices.
26-47: Great job addressing modulo bias in NumberInRange().
Implementing rejection sampling to eliminate modulo bias enhances the uniformity of the random number generation.
54-122: Reduce code duplication between StringWithLength() and StringWithCharset().
Both functions share similar logic. Consider refactoring StringWithLength() to call StringWithCharset() with the default charset.
This suggestion aligns with a previous review comment and is still applicable.
| } | ||
|
|
||
| // Pick returns a random element from the provided slice | ||
| func Pick[T any](slice []T) (T, error) { |
There was a problem hiding this comment.
Thanks for adding these additional Functions ❤️
Signed-off-by: Guilherme Thomas <[email protected]>
Signed-off-by: Guilherme Thomas <[email protected]>
|
I followed coderabbitai advices, did some changes and updated my files. |
Signed-off-by: Guilherme Thomas <[email protected]>
Besides what was mentioned in issue #28, I took the liberty of also adding an option to handle slices with Pick() and Shuffle().
Example of use:
Summary by CodeRabbit
New Features
Tests