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.

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

Closed

Conversation

PingGuoMiaoMiao
Copy link
Contributor

PR Description​

​Summary​

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

@coveralls
Copy link
Collaborator

coveralls commented Jul 2, 2025

Pull Request Test Coverage Report for Build 344

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.61%

Totals Coverage Status
Change from base Build 343: 0.0%
Covered Lines: 3493
Relevant Lines: 3898

💛 - Coveralls

Copy link

peter-jerry-ye-code-review bot commented Jul 8, 2025

Potential bug in deterministic test random function

Category
Correctness
Code Snippet
fn rand(upper : Int) -> Int {
(upper * 3 + 7) % upper // Deterministic pseudo-random for testing
}
Recommendation
Use a proper deterministic random function that covers the full range. Consider: fn rand(upper : Int) -> Int { if upper <= 1 { 0 } else { (upper * 3 + 7) % upper } }
Reasoning
The current function may not provide good distribution and could have edge cases when upper is 1 or very small values, potentially causing modulo by zero or poor test coverage

Test coverage is insufficient for shuffle functionality

Category
Maintainability
Code Snippet
test "shuffle_in_place" {
// Only tests basic properties but not edge cases
Recommendation
Add tests for edge cases like empty deque, single element deque, and verify that shuffle actually changes order in most cases. Also test with different deque states (wrapped vs non-wrapped circular buffer).
Reasoning
The current test only verifies basic properties but doesn't test important edge cases or verify that the shuffle algorithm works correctly across different deque internal states

Unnecessary copy operation in shuffle method

Category
Performance
Code Snippet
pub fn[A] shuffle(self : T[A], rand~ : (Int) -> Int) -> T[A] {
// Create a copy of the original deque
let new_deque = self.copy()
// Shuffle the copy in place
new_deque.shuffle_in_place(rand~)
// Return the shuffled copy
new_deque
}
Recommendation
Consider implementing shuffle without relying on copy+shuffle_in_place if copy is expensive, or document the performance characteristics clearly in the function documentation.
Reasoning
The current implementation creates a full copy then shuffles in place, which may not be the most efficient approach depending on the deque's copy implementation. This could be optimized for better performance.

@PingGuoMiaoMiao
Copy link
Contributor Author

Ask me about this PR and the above questions

@peter-jerry-ye peter-jerry-ye requested a review from Copilot July 10, 2025 05:24
Copilot

This comment was marked as outdated.

Copy link
Collaborator

@peter-jerry-ye peter-jerry-ye left a comment

Choose a reason for hiding this comment

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

Tests are missing.

Copilot

This comment was marked as outdated.

@PingGuoMiaoMiao PingGuoMiaoMiao requested a review from Copilot July 12, 2025 11:58
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces two new shuffle operations on the deque using the Fisher–Yates algorithm and adds a test for both methods.

  • Added interface declarations for shuffle and shuffle_in_place in deque.mbti.
  • Implemented shuffle_in_place and shuffle in deque.mbt.
  • Added a combined test covering both shuffle methods in deque_test.mbt.

Reviewed Changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.

File Description
deque/deque.mbti Declared new shuffle and shuffle_in_place APIs
deque/deque.mbt Implemented the shuffle methods
deque/deque_test.mbt Added a test for in-place and non-in-place shuffle
Comments suppressed due to low confidence (2)

deque/deque_test.mbt:1158

  • [nitpick] The test is named "shuffle_in_place" but also covers the non-in-place shuffle method. Consider renaming it to reflect both methods or splitting it into two focused tests.
test "shuffle_in_place" {

deque/deque_test.mbt:1175

  • Add separate test cases for edge conditions (e.g., empty and single-element deques) to ensure both shuffle_in_place and shuffle behave correctly across all deque sizes.
  // Test non-in-place version

PingGuoMiaoMiao and others added 7 commits July 14, 2025 10:01
	modified:   deque/deque.mbti
with '#' will be ignored, and an empty message aborts the commit.

On branch shuffle-implement
Your branch is up to date with 'origin/shuffle-implement'.

Changes to be committed:
	modified:   deque/deque_test.mbt
	modified:   deque/moon.pkg.json
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.

3 participants