8000 feat(deque): add shuffle_in_place and shuffle method by PingGuoMiaoMiao · Pull Request #2398 · moonbitlang/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat(deque): add shuffle_in_place and shuffle method #2398

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

feat(deque): add shuffle_in_place and shuffle method #2398

wants to merge 7 commits into from
+47 −0

Conversation

PingGuoMiaoMiao
Copy link
Contributor

PR Description​​

​​Summary​

This pr adds the shuffle method, but does not write a test

@PingGuoMiaoMiao
modified: deque/deque.mbt
6455931 
	modified:   deque/deque.mbti
@coveralls
Copy link
Collaborator
coveralls commented Jul 2, 2025 •
edited
Loading

Pull Request Test Coverage Report for Build 261

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 89.151%
Totals Coverage Status
Change from base Build 258: 0.0%
Covered Lines: 3476
Relevant Lines: 3899
💛 - Coveralls

@peter-jerry-ye-code-review peter-jerry-ye code review
Copy link
Inconsistent naming convention between method and function call

Category
Maintainability
Code Snippet
pub fn[A] T::shuffle_in_place(self : T[A], rand~ : (Int) -> Int) -> Unit {
// ...
}

pub fn[A] shuffle(self : T[A], rand~ : (Int) -> Int) -> T[A] {
T::shuffle_in_place(new_deque, rand~)
}
Recommendation
Use consistent naming convention. Either make both methods associated functions (T::shuffle and T::shuffle_in_place) or both instance methods (shuffle and shuffle_in_place)
Reasoning
The current implementation has shuffle_in_place as an associated function (T::shuffle_in_place) while shuffle is an instance method. This inconsistency makes the API confusing and harder to discover.

Unnecessary copy operation for shuffle method

Category
Performance
Code Snippet
pub fn[A] shuffle(self : T[A], rand~ : (Int) -> Int) -> T[A] {
let new_deque = self.copy()
T::shuffle_in_place(new_deque, rand~)
new_deque
}
Recommendation
Consider implementing shuffle directly without relying on copy + shuffle_in_place, or clearly document the performance implications of the copy operation
Reasoning
The current approach creates a full copy of the deque before shuffling, which doubles memory usage temporarily. For large deques, this could be inefficient and cause memory pressure.

Missing validation for rand function behavior

Category
Correctness
Code Snippet
pub fn[A] T::shuffle_in_place(self : T[A], rand~ : (Int) -> Int) -> Unit {
let n = self.len
let buf_length = self.buf.length()
for i = n - 1; i > 0; i = i - 1 {
let j = rand(i + 1)
Recommendation
Add validation or documentation to ensure rand function returns values in the expected range [0, n), or add bounds checking for the returned value
Reasoning
The function assumes the rand function will always return a valid index within bounds, but there's no validation. If rand returns an out-of-bounds value, it could cause array access violations or incorrect shuffling behavior.

@PingGuoMiaoMiao
Copy link
Contributor Author

Ask me about this PR and the above questions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@PingGuoMiaoMiao @coveralls
0