feat(data structures, linked list) reorder list #122
Conversation
BrianLusina
added
enhancement
Algorithm
WalkthroughThis pull request introduces linked list manipulation utilities and features. It adds a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
datastructures/linked_lists/singly_linked_list/single_linked_list_utils.py (2)
23-23: Fix typo in comment."loose" should be "lose".
🔎 Proposed fix
- # save the pointer 1 next node to not loose it + # save the pointer 1 next node to not lose it
65-65: Correct misleading comment.The comment "return the new tail of the k-group which is our head" appears to be copied from a different context (
reverse_groups). This function simply reverses the entire list and returns the new head.🔎 Proposed fix
- # return the new tail of the k-group which is our head + # return the new head of the reversed list return previousDIRECTORY.md (1)
258-258: Address markdown list indentation.The static analysis tool flags that these list items use 6-space indentation where 4 spaces are expected per the project's markdown style (MD007). While not critical, consistent formatting improves readability.
As per static analysis hints, consider adjusting indentation from 6 to 4 spaces to match the expected style.
Also applies to: 265-265, 267-267
datastructures/linked_lists/__init__.py (1)
719-750: Implementation is correct but duplicated.This static utility correctly reverses a linked list. However, as noted in the review of
single_linked_list_utils.py, this same logic exists in three places. Consider whether this static method should be the canonical implementation that other modules import, or if the utility module should be the source of truth.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
datastructures/linked_lists/singly_linked_list/images/examples/singly_linked_list_reorder_list_example_1.pngis excluded by!**/*.pngdatastructures/linked_lists/singly_linked_list/images/examples/singly_linked_list_reorder_list_example_2.pngis excluded by!**/*.pngdatastructures/linked_lists/singly_linked_list/images/examples/singly_linked_list_reorder_list_example_3.pngis excluded by!**/*.pngdatastructures/linked_lists/singly_linked_list/images/examples/singly_linked_list_reorder_list_example_4.pngis excluded by!**/*.pngdatastructures/linked_lists/singly_linked_list/images/examples/singly_linked_list_reorder_list_example_5.pngis excluded by!**/*.png
📒 Files selected for processing (8)
DIRECTORY.md(1 hunks)datastructures/linked_lists/__init__.py(1 hunks)datastructures/linked_lists/singly_linked_list/README.md(1 hunks)datastructures/linked_lists/singly_linked_list/single_linked_list.py(2 hunks)datastructures/linked_lists/singly_linked_list/single_linked_list_utils.py(1 hunks)datastructures/linked_lists/singly_linked_list/test_singly_linked_list_reorder_list.py(1 hunks)datastructures/linked_lists/singly_linked_list/test_singly_linked_merge_and_weave.py(1 hunks)datastructures/linked_lists/singly_linked_list/test_singly_linked_revese.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
datastructures/linked_lists/__init__.py (3)
datastructures/linked_lists/singly_linked_list/single_linked_list.py (1)
reverse_list(908-928)datastructures/linked_lists/singly_linked_list/single_linked_list_utils.py (1)
reverse_list(36-66)datastructures/linked_lists/build_one_2_3/__init__.py (1)
Node(1-4)
datastructures/linked_lists/singly_linked_list/test_singly_linked_merge_and_weave.py (1)
datastructures/linked_lists/singly_linked_list/single_linked_list_utils.py (1)
merge_and_weave(5-33)
datastructures/linked_lists/singly_linked_list/test_singly_linked_revese.py (2)
datastructures/linked_lists/__init__.py (2)
reverse_list(720-750)reverse(196-203)datastructures/linked_lists/singly_linked_list/single_linked_list_utils.py (1)
reverse_list(36-66)
datastructures/linked_lists/singly_linked_list/single_linked_list.py (3)
datastructures/linked_lists/__init__.py (2)
reverse_list(720-750)middle_node(671-687)datastructures/linked_lists/singly_linked_list/single_linked_list_utils.py (2)
reverse_list(36-66)merge_and_weave(5-33)datastructures/linked_lists/singly_linked_list/node.py (1)
SingleNode(4-10)
datastructures/linked_lists/singly_linked_list/single_linked_list_utils.py (3)
datastructures/linked_lists/singly_linked_list/node.py (1)
SingleNode(4-10)datastructures/linked_lists/__init__.py (1)
reverse_list(720-750)datastructures/linked_lists/singly_linked_list/single_linked_list.py (1)
reverse_list(908-928)
🪛 markdownlint-cli2 (0.18.1)
DIRECTORY.md
258-258: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
259-259: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
260-260: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
261-261: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
262-262: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
263-263: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
264-264: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
265-265: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
266-266: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
267-267: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
🔇 Additional comments (8)
datastructures/linked_lists/singly_linked_list/test_singly_linked_list_reorder_list.py (1)
8-44: LGTM - comprehensive test coverage.The test suite is well-structured with good coverage of various scenarios including:
- Lists of different lengths (odd and even)
- Lists with positive, negative, and mixed values
- Edge cases like 3-node and 4-node lists
The test correctly builds the linked list, invokes
reorder_list, and validates the reordered sequence by traversing the result.datastructures/linked_lists/singly_linked_list/single_linked_list.py (2)
7-10: Imports look correct.The imports of
reverse_listandmerge_and_weavefrom the utility module are appropriate for the newreorder_listimplementation.
990-1019: Verify reorder_list logic flows correctly after fixing method call.The overall algorithm is sound:
- Find the middle node
- Split into two halves
- Reverse the second half
- Merge/weave the halves
Once the critical issue on line 1013 is fixed, ensure the logic handles edge cases properly, especially for odd-length lists where the middle node stays in the first half.
datastructures/linked_lists/singly_linked_list/test_singly_linked_merge_and_weave.py (1)
11-49: LGTM - good test coverage for merge_and_weave utility.The tests appropriately cover:
- Empty input scenario (both lists empty)
- Different length lists being merged
- Correct interleaving/weaving of nodes
The test correctly constructs two linked lists, calls
merge_and_weave, and validates the merged result by traversing and comparing to the expected sequence.datastructures/linked_lists/singly_linked_list/test_singly_linked_revese.py (4)
7-9: Import addition is appropriate.Adding the
reverse_listutility import enables testing of the standalone function alongside the instance method.
11-34: Good refactoring to centralize test cases.Moving test cases to a module-level
TEST_CASESconstant improves maintainability and makes it easier to reuse test data across multiple test methods.
42-44: Correct expectation for invalid input.The change to expect
ValueErrorwhen callingreverse_between(0, 1)on an empty list is correct, asreverse_betweenvalidates thatleftandrightmust be positive integers (≥ 1).
101-136: LGTM - comprehensive testing of reverse functionality.The two new tests provide good coverage:
test_reverse_linked_list: validates the instance method.reverse()test_reverse_linked_list_static: validates the standalonereverse_listutility functionBoth correctly build a linked list, perform reversal, traverse the result, and assert the expected reversed sequence.
| if first_half_head is None or second_half_head is None: | ||
| return None |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd datastructures/linked_lists/singly_linked_list && find . -name "*.py" | head -20Repository: BrianLusina/PythonSnips
Length of output: 611
🏁 Script executed:
cat -n datastructures/linked_lists/singly_linked_list/single_linked_list_utils.py | head -80Repository: BrianLusina/PythonSnips
Length of output: 2483
🏁 Script executed:
# Also search for usage in single_linked_list.py
grep -n "merge_and_weave\|reorder" datastructures/linked_lists/singly_linked_list/single_linked_list.py | head -20Repository: BrianLusina/PythonSnips
Length of output: 259
🏁 Script executed:
sed -n '985,1025p' datastructures/linked_lists/singly_linked_list/single_linked_list.pyRepository: BrianLusina/PythonSnips
Length of output: 1636
Fix early-return logic for edge cases in merge_and_weave.
The function returns None when either input is None. For single-node lists passed from reorder_list, this causes incorrect behavior—a single node should be returned unchanged, not None. Change lines 16-17 to return the non-None input:
if first_half_head is None:
return second_half_head
if second_half_head is None:
return first_half_head🤖 Prompt for AI Agents
In datastructures/linked_lists/singly_linked_list/single_linked_list_utils.py
around lines 16 to 17, the early-return incorrectly returns None when one list
is missing; instead return the non-None head so single-node lists are preserved.
Replace the combined None-check with two checks: if first_half_head is None
return second_half_head; if second_half_head is None return first_half_head,
preserving the existing list when the other side is absent.
| def reverse_list(head: SingleNode) -> Optional[SingleNode]: | ||
| """ | ||
| Reverses a linked list given the head node | ||
| Args: | ||
| head Node: the head node of the linked list | ||
| Returns: | ||
| Optional[Node]: the new head node of the reversed linked list | ||
| """ | ||
| if head is None or head.next is None: | ||
| return head | ||
|
|
||
| # track previous node, so we can point our next pointer to it | ||
| previous = None | ||
| # track node to loop through | ||
| current_node = head | ||
|
|
||
| while current_node: | ||
| # track the next node to not lose it while adjusting pointers | ||
| nxt = current_node.next | ||
|
|
||
| # set the next pointer to the node behind it, previous | ||
| current_node.next = previous | ||
|
|
||
| # adjust the new previous node to the current node for subsequent loops | ||
| previous = current_node | ||
|
|
||
| # move our node pointer up to the next node in front of it | ||
| current_node = nxt | ||
|
|
||
| # return the new tail of the k-group which is our head | ||
| return previous |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Eliminate code duplication for reverse_list.
This reverse_list function duplicates the implementation in:
datastructures/linked_lists/__init__.py(lines 719-749)datastructures/linked_lists/singly_linked_list/single_linked_list.py(lines 907-927, inline inreverse_groups)
Consider consolidating to a single canonical implementation (likely the static utility in __init__.py) and importing it where needed, rather than maintaining three identical copies.
Based on learnings, code duplication should be avoided to reduce maintenance burden and potential for divergence.
🤖 Prompt for AI Agents
datastructures/linked_lists/singly_linked_list/single_linked_list_utils.py lines
36-66: this reverse_list implementation is a duplicate; remove the local
function and instead import the canonical reverse_list from
datastructures.linked_lists (the static utility in __init__.py), update this
module to export or use that imported function, ensure Optional and SingleNode
types are imported/referenced consistently, and update any local calls to use
the imported function so there’s a single maintained implementation across the
codebase.
| # The reversal step involves taking the second half of the linked list and reversing it in place | ||
| # for example, if the linked list is 1 -> 2 -> 3 -> 4 -> 5, the second half is 3 -> 4 -> 5 | ||
| # after reversing, it becomes 5 -> 4 -> 3 | ||
| reversed_second_half = self.reverse_list(second_half_head) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the file location
FILE_PATH=$(find . -name "single_linked_list.py" -type f 2>/dev/null | head -1)
if [ -z "$FILE_PATH" ]; then
echo "File not found. Searching for variations..."
find . -name "*linked_list*.py" -type f 2>/dev/null | grep -i single | head -5
else
echo "Found file: $FILE_PATH"
# Check line 8 for imports
echo "=== Lines around line 8 (imports) ==="
sed -n '1,20p' "$FILE_PATH"
# Check line 1013
echo ""
echo "=== Line 1013 (the problematic line) ==="
sed -n '1010,1016p' "$FILE_PATH"
# Check if reverse_list is defined as a method in the class
echo ""
echo "=== Searching for reverse_list definitions ==="
grep -n "def reverse_list" "$FILE_PATH"
# Check for self.reverse_list calls
echo ""
echo "=== Checking for self.reverse_list calls ==="
grep -n "self\.reverse_list" "$FILE_PATH"
fiRepository: BrianLusina/PythonSnips
Length of output: 1556
Fix incorrect method call on line 1013.
self.reverse_list(second_half_head) attempts to call reverse_list as a class method, but reverse_list is imported as a standalone function (line 8), not a method of SinglyLinkedList. This will cause an AttributeError at runtime.
- reversed_second_half = self.reverse_list(second_half_head)
+ reversed_second_half = reverse_list(second_half_head)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| reversed_second_half = self.reverse_list(second_half_head) | |
| reversed_second_half = reverse_list(second_half_head) |
🤖 Prompt for AI Agents
In datastructures/linked_lists/singly_linked_list/single_linked_list.py around
line 1013, the code calls self.reverse_list(second_half_head) but reverse_list
is imported as a standalone function (not a SinglyLinkedList method); replace
the method call with a call to the imported function
(reverse_list(second_half_head)) and ensure the top-level import for
reverse_list remains correct and used consistently in this file.
Describe your change:
Reorder linked list by splitting it into two halves and then weaving and merging the two halves into one linked list
Checklist:
Fixes: #{$ISSUE_NO}.Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.