Skip to content

Conversation

@Asterless
Copy link
Contributor

Based on #2850. This pull request updates the rev_iter2 for the Deque data structure to ensure that the index values correctly reflect their position when iterating in reverse order. The index now starts from length - 1 and decrements with each step, rather than starting from 0 and incrementing. This change makes the reverse iterator's behavior more intuitive and consistent with typical expectations.

@peter-jerry-ye-code-review
Copy link

peter-jerry-ye-code-review bot commented Oct 27, 2025

Test logic error in rev_eachi test case

Category
Correctness
Code Snippet
Line 189: if index != j - 2 || elem != i - 1 {
failed = true
}
Recommendation
Change the condition to if index != j || elem != i - 1 since j now correctly tracks the expected index
Reasoning
The test is using j - 2 which appears to be an incorrect adjustment. Since j now starts at 6 and decrements to match the new indexing behavior, the comparison should be direct index != j

Inconsistent variable naming in iterator implementation

Category
Maintainability
Code Snippet
Lines 1613-1627: let mut j = len - 1
...
j -= 1
Recommendation
Rename variable j to index or current_index to better reflect its purpose as the index counter
Reasoning
The variable name j is not descriptive and makes the code harder to understand. Since this variable represents the current index being yielded, a more descriptive name would improve code readability

Test initialization values need verification

Category
Correctness
Code Snippet
Lines 185-186: let mut i = 6
let mut j = 6
Recommendation
Verify that initial values are correct: for a deque of length 5, j should start at 4 (length-1), not 6
Reasoning
The test uses a deque of 5 elements [1,2,3,4,5], so the initial index should be 4 (length-1), but j is initialized to 6. This suggests either the test data or initialization values are incorrect

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 1626

Details

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 89.853%

Totals Coverage Status
Change from base Build 1621: 0.0%
Covered Lines: 9785
Relevant Lines: 10890

💛 - Coveralls

@peter-jerry-ye
Copy link
Collaborator

We need to discuss this furthur before making changes.

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