feat(datastructures, trees): tree traversal algorithms#169
Merged
BrianLusina merged 25 commits intomainfrom Feb 6, 2026
Merged
feat(datastructures, trees): tree traversal algorithms#169BrianLusina merged 25 commits intomainfrom
BrianLusina merged 25 commits intomainfrom
Conversation
BrianLusina
added
enhancement
dependencies
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds a new standalone BinarySearchTree implementation and exposes public BST symbols from the package; augments BinaryTree with range-sum and univalue-path helpers; and introduces many new algorithm modules, READMEs, and tests across backtracking, dynamic programming, graphs, sliding-window, two-pointers, intervals, and pymath. DIRECTORY.md updated. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (3 warnings)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@datastructures/trees/binary/search_tree/binary_search_tree.py`:
- Around line 11-14: The iterator stack is created empty in __init__ but never
populated, so next() and has_next() fail; update the BinarySearchTree __init__
to populate the iterator by calling __leftmost_inorder(root) (i.e., invoke the
class's leftmost inorder initializer after creating DynamicSizeStack) so the
stack contains the leftmost path from root, ensuring has_next() and next()
behave immediately; reference __init__, __leftmost_inorder, next, has_next,
DynamicSizeStack, BinaryTreeNode and mirror how BinarySearchTreeIterator
initializes its stack.
- Around line 789-800: The loop mutates the tree by reassigning self.root while
searching for the LCA (you use self.root = self.root.left / self.root =
self.root.right and finally return self.root); fix it by using a local traversal
pointer (e.g., current = self.root) inside the method that checks current.data
against node_one.data and node_two.data, move current to current.left or
current.right as needed, and return current (not self.root) so the tree's root
remains unchanged; ensure all references to left/right comparisons and the final
returned node use that local variable.
- Around line 218-224: The has_next() method currently returns
self.stack.is_empty() which is inverted; update has_next (in class
BinarySearchTree iterator logic) to return the opposite boolean (i.e., True when
self.stack is not empty) so the iterator reports there are remaining items;
locate the has_next method referencing self.stack and change the return to the
negation of is_empty (return not self.stack.is_empty()) to fix the logic.
- Around line 404-422: The breadth_first_search method currently enqueues and
dequeues nodes but never collects or returns values; modify breadth_first_search
to create a results list (e.g., results = []), when dequeuing a node append its
value (e.g., current_node.value or current_node.data depending on your node
field) to results, continue enqueuing left/right children as before, and after
the loop return the results list; ensure you handle the case where self.root is
None by returning an empty list and use the existing FifoQueue, current_node,
and self.root symbols to locate the code to update.
- Around line 349-354: The loop incorrectly computes current_diff using
self.root.data instead of the traversing node; update the calculation to use
current.data (i.e., compute current_diff = abs(target - current.data)) so
min_diff and closest_node are compared/updated against the node actually being
visited (variables: current, min_diff, closest_node); make the single-line
change where current_diff is assigned and ensure no other references still use
self.root.data inside this traversal.
- Around line 690-708: merge_trees currently always operates on self.root and
calls itself with other_node children, corrupting recursion; implement a helper
function (e.g., _merge_nodes(self_node: BinaryTreeNode, other_node:
BinaryTreeNode) -> BinaryTreeNode) that merges two corresponding nodes: if one
node is None return the other, if both present sum data into self_node.data,
then set self_node.left = _merge_nodes(self_node.left, other_node.left) and
self_node.right = _merge_nodes(self_node.right, other_node.right); change
merge_trees to call and return _merge_nodes(self.root, other_node) so merging
correctly traverses both trees.
- Around line 506-534: The in_order_morris_traversal implementation is
corrupting the tree by permanently nulling left links (temp.left = None); update
the function to implement the standard Morris algorithm: for each node current,
if current.left is None append current.data and move current = current.right;
otherwise find the inorder predecessor pre = rightmost node in current.left and
if pre.right is None set pre.right = current and move current = current.left,
else (pre.right == current) set pre.right = None, append current.data and move
current = current.right. Remove the permanent left-clearing (temp.left = None)
and use the predecessor's right pointer restoration to leave the tree intact
(references: in_order_morris_traversal, current, pre, temp).
- Around line 468-485: The in_order_recurse method is wrong because it ignores
its node parameter and always uses self.root; change it to use the passed node
as the recursion root, add a base-case check for None, call
self.in_order_recurse(node.left) and self.in_order_recurse(node.right) and
aggregate their returned lists (extend or concat) into result before and after
appending node.data, and ensure the function returns the aggregated result;
update references in the function body (notably in_order_recurse and any uses of
self.root) to use node instead.
In `@datastructures/trees/binary/tree/binary_tree.py`:
- Around line 947-950: The DFS pruning is wrong for an unordered BinaryTree:
inside the inner function dfs(node: Optional[BinaryTreeNode]) stop-checking on
node.data being out of [low, high] must be removed because children can still be
in-range; instead, change dfs to only return 0 for a None node, recursively sum
dfs(node.left) and dfs(node.right), and then add node.data to the sum only if
low <= node.data <= high (referencing dfs, BinaryTreeNode, low, high).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Fix all issues with AI agents
In `@algorithms/backtracking/letter_tile_possibilities/README.md`:
- Around line 3-73: The README currently describes a factorial/count-based
method but the implementation uses set(permutations(...)); either update the
README to describe the implemented approach (explicitly mention using Python's
permutations and de-duplication via set(permutations(...)) and its performance
trade-offs) or change the implementation to the factorial combinatorics
approach; also reword the ambiguous sentence "each sequence must include at most
one tile, tiles[i]" to "each tile may be used at most once per sequence", and
correct the formula to show the product over repeated letter counts as n! / (∏
c_i!) where c_i are counts of each repeated letter.
In `@algorithms/dynamic_programming/frog_jump/README.md`:
- Around line 82-84: Update the Space Complexity section to mention both the
mapper and the DP table: state that the mapper/map uses O(n) extra space and
explicitly call out the 2D dp array (named dp) allocated as 1001 x 1001; note
that this fixed-size table can be described as O(1) space if treating the bounds
as constants or O(n²) if you view the table size as dependent on input
magnitude—include both interpretations and clarify which one the README adopts.
In `@algorithms/dynamic_programming/max_profit_in_job_scheduling/__init__.py`:
- Around line 15-20: Precompute the list of end times once before the loop and
use it in bisect_right to avoid rebuilding [job[1] for job in jobs] on every
iteration: create end_times = [job[1] for job in jobs] before the for i in
range(1, job_len + 1) loop and replace bisect_right([job[1] for job in jobs],
start) with bisect_right(end_times, start); also change the unpacking inside the
loop from "start, end, profit = jobs[i - 1]" to "start, end, p = jobs[i - 1]"
(and use p when updating dp) to avoid shadowing the function parameter named
profit.
In `@algorithms/dynamic_programming/max_profit_in_job_scheduling/README.md`:
- Line 1: The top-level markdown heading "# Maximum Profit in Job Scheduling"
has two spaces after the hash which violates MD019; change it to a single space
by updating the heading to "# Maximum Profit in Job Scheduling" (locate the
header text "Maximum Profit in Job Scheduling" in README.md and remove the extra
space after the '#' so there is exactly one space).
In `@algorithms/dynamic_programming/maximal_rectangle/README.md`:
- Around line 69-70: The README contains a typo: when matrix[i][j] is '0' the
text says "right[j] is reset to ~" but the implementation uses n (or cols);
update the sentence to state that right[j] is reset to n (or cols) and
current_right is set to j, referencing the variables right[j], current_right,
matrix[i][j], and n/cols so it matches the algorithm implementation.
In `@algorithms/dynamic_programming/word_break/test_word_break.py`:
- Around line 45-47: The test suite contains a duplicate test tuple for the
input "applepenapple" with dictionary ["apple", "pen"] (the repeated entry in
test_word_break.py); remove the redundant tuple from the test cases list so each
scenario appears only once (locate the duplicate tuple in the list of test
inputs and delete the second occurrence).
In `@algorithms/graphs/network_delay_time/__init__.py`:
- Around line 19-31: The early-return check "if k not in graph: return -1" in
network_delay_time is wrong for sink start nodes and the unused vertices set is
unnecessary; remove that early exit, delete the unused "vertices" set population
(or stop populating it), and instead add a single special-case: if n == 1 return
0; let the normal shortest-path/Dijkstra logic operate when k has no outgoing
edges so reachability is determined by the algorithm rather than the incorrect
graph key check.
In `@algorithms/graphs/network_delay_time/test_network_delay_time.py`:
- Around line 39-45: The test case tuple's human-readable description
("times=[[1,2,1],[2,3,2],[3,4,3],[4,1,4]], n=4, k=1") does not match the actual
k argument (2); update the description string to use k=2 so it accurately
reflects the inputs in the tuple (the entry containing [[1, 2, 1], [2, 3, 2],
[3, 4, 3], [4, 1, 4]], 4, 2, 9), or alternatively change the numeric k to 1 if
the intended case was k=1—ensure the text and the value in the test tuple (the
test case tuple in test_network_delay_time.py) are consistent.
In `@algorithms/intervals/data_stream/README.md`:
- Around line 114-115: Update the complexity note header by hyphenating
"Worst-case" (change "Worst case relation to n:" to "Worst-case relation to n:")
in the README line that discusses Add Num(int value) and Get Intervals()
complexity so the grammar is correct; leave the rest of the sentence and
references to Add Num and Get Intervals() unchanged.
In `@datastructures/trees/binary/search_tree/bst_utils.py`:
- Around line 17-30: The current dfs uses float("-inf")/float("inf") as
sentinels which fails for non-numeric T; change dfs to accept Optional[T] bounds
(e.g., dfs(node: Optional[BinaryTreeNode], min_value: Optional[T], max_value:
Optional[T]) -> bool) and replace comparisons with guarded checks (if min_value
is not None and node.data <= min_value: return False; if max_value is not None
and node.data >= max_value: return False), recurse as dfs(node.left, min_value,
node.data) and dfs(node.right, node.data, max_value), and start with dfs(root,
None, None).
In `@datastructures/trees/binary/search_tree/README.md`:
- Around line 453-463: The README text uses min_ and max_ which can be parsed as
emphasis in some Markdown renderers; update the wording to wrap the parameter
names in inline code formatting (e.g., `min_` and `max_`) wherever they
appear—including the phrases "Pass the current node's value as the new max_
value" and "Pass the current node's value as the new min_ value" and any mention
of the helper function—to ensure they render correctly and unambiguously.
In `@datastructures/trees/binary/search_tree/test_binary_search_tree_valid.py`:
- Around line 7-12: IS_VALID_BST_TEST_CASES currently uses level-order-looking
lists but the test harness builds trees by sequentially calling insert_node
(which ignores None), so the constructed trees differ; update the tests in
IS_VALID_BST_TEST_CASES and the test setup in test_binary_search_tree_valid.py
to either (A) treat the lists as insertion sequences by renaming the variable
and adding a brief comment/documentation that these are insert sequences for
insert_node, or (B) implement and call a level-order builder (e.g.,
build_tree_level_order or build_tree_from_level_list) that accepts None
placeholders and constructs the exact tree shape before calling is_valid_bst;
pick one approach and update references to insert_node/is_valid_bst accordingly
so the test input semantics match the construction method.
🧹 Nitpick comments (11)
algorithms/graphs/single_cycle_check/__init__.py (2)
14-17: Redundant negative index check in Python.In Python, the modulo operator with a positive divisor always returns a non-negative result (e.g.,
(-5) % 6 = 1). The conditional on line 17 is unnecessary sincenext_indexwill always be>= 0whenn > 0.♻️ Simplified version
def get_next_index(current_idx: int) -> int: jump = array[current_idx] next_index = (jump + current_idx) % n - return next_index if next_index >= 0 else next_index + n + return next_index
4-27: Consider handling empty array edge case.If
arrayis empty (n = 0), the function returnsTrue(sincecurrent_index == 0). This may be unexpected behavior depending on requirements. Consider adding an early return or validation.🛡️ Optional guard for empty input
def has_single_cycle(array: List[int]) -> bool: """ Checks if the provided array has a single cycle. Args: array (List[int]): list of integers. Returns: bool: True if the given array has a single cycle, False otherwise. """ n = len(array) + if n == 0: + return Falsealgorithms/graphs/single_cycle_check/test_single_cycle_check.py (2)
6-8: Consider adding more test cases for comprehensive coverage.Only one test case is provided. Consider adding edge cases such as:
- Array returning
False(e.g.,[1, 1, 1, 1, 1]forms a shorter cycle)- Single element array (
[0]should returnTrueif it stays in place)- All elements are same (
[1, 1]visits only even indices)📋 Additional test cases
SINGLE_CYCLE_CHECK_TEST_CASES = [ ([2, 3, 1, -4, -4, 2], True), + ([1, 1, 1, 1, 1], False), # Shorter cycle, doesn't visit all + ([1, -1], True), # Two elements forming a cycle + ([0], True), # Single element pointing to itself ]
1-4: Unused import:Listfrom typing.The
Listtype hint is imported but not strictly necessary sincearrayparameter is already annotated in the function signature being tested.🧹 Remove unused import
import unittest -from typing import List from parameterized import parameterized from algorithms.graphs.single_cycle_check import has_single_cyclealgorithms/dynamic_programming/frog_jump/__init__.py (1)
10-11: Hardcoded DP dimensions may fail for large inputs.The DP array is fixed at 2001×2001, which allocates ~16MB regardless of input size. While this matches typical LeetCode constraints (stones.length ≤ 2000), it will:
- Fail with IndexError if
n > 2001- Waste memory for small inputs
♻️ Consider dynamic sizing or bounds check
Option 1: Add a bounds check:
def can_cross(stones: List[int]) -> bool: n = len(stones) + if n > 2001: + raise ValueError("Input exceeds maximum supported size")Option 2: Size dynamically (trades off memory efficiency for flexibility):
- dp = [[0] * 2001 for _ in range(2001)] + dp = [[0] * (n + 1) for _ in range(n)]algorithms/graphs/network_delay_time/__init__.py (1)
1-4: Unused import:Set.
Setis imported fromtypingbut theverticesset that used it is dead code. After removing the unusedverticeslogic,Setcan also be removed from the import.algorithms/dynamic_programming/maximal_rectangle/__init__.py (1)
4-6: Consider adding a docstring for consistency.
maximal_rectangle_2andlargest_rectangle_areahave docstrings, butmaximal_rectangledoes not. Adding one would improve consistency and documentation.📝 Suggested docstring
def maximal_rectangle(matrix: List[List[int]]) -> int: + """ + Find the largest rectangle containing only 1s in a binary matrix. + + Uses dynamic programming with height, left, and right boundary arrays + to efficiently compute the maximal rectangle area in O(m*n) time. + + Args: + matrix: 2D binary matrix with 0s and 1s + + Returns: + Area of the largest rectangle containing only 1s + """ if not matrix: return 0algorithms/dynamic_programming/maximal_rectangle/test_maximal_rectangle.py (1)
9-76: Consider adding an empty matrix test case.The implementations handle empty matrices with
if not matrix: return 0, but there's no test case verifying this behavior. Adding([], 0)would ensure this edge case is covered.📝 Suggested addition
MAXIMAL_RECTANGLE_TEST_CASES = [ + ([], 0), # empty matrix ( [ [1, 0, 1, 0, 1],algorithms/backtracking/letter_tile_possibilities/__init__.py (2)
1-56: Reduce factorial memory by counting permutations instead of materializing them.
set(permutations(current))builds all permutations in memory for every subset. Since you already sketched the factorial-based approach, consider turning that into real helpers and using it to compute permutation counts directly (and remove the commented-out block).♻️ Suggested refactor
-from itertools import permutations - def num_tile_possibilities(tiles: str) -> int: # Store unique sequences to avoid duplicates unique_letter_sets: Set[str] = set() @@ def generate_sequences(current: str, index: int): @@ if current not in unique_letter_sets: unique_letter_sets.add(current) - total_permutations = len(set(permutations(current))) + total_permutations = count_permutations(current) return total_permutations return 0 @@ - # Optionally, you can write the count_permutations and the factorial method if not using builtin methods - # def factorial(n): - # - # # Computes the factorial of a given number. - # if n <= 1: - # return 1 - # - # result = 1 - # for num in range(2, n + 1): - # result *= num - # return result - # - # def count_permutations(sequence): - # - # # Calculates the number of unique permutations of a sequence, accounting for duplicate characters. - # permutations = factorial(len(sequence)) - # - # # Adjust for repeated characters by dividing by factorial of their counts - # for count in Counter(sequence).values(): - # permutations //= factorial(count) - # - # return permutations + def factorial(n: int) -> int: + result = 1 + for num in range(2, n + 1): + result *= num + return result + + def count_permutations(sequence: str) -> int: + counts = {} + for ch in sequence: + counts[ch] = counts.get(ch, 0) + 1 + total = factorial(len(sequence)) + for count in counts.values(): + total //= factorial(count) + return total
76-94: Optionally guard against non‑uppercase input to avoid IndexError.
If this function is used outside the problem constraints, a non‑A–Zcharacter yields a negative/overflow index. Consider a defensive check or a clearer error.🛡️ Possible defensive check
def num_tile_possibilities_with_optimized_recursion(tiles: str) -> int: char_count: List[int] = [0] * 26 for letter in tiles: + if letter < "A" or letter > "Z": + raise ValueError("tiles must contain only uppercase A-Z letters") char_count[ord(letter) - ord("A")] += 1algorithms/backtracking/letter_tile_possibilities/test_num_tile_possibilities.py (1)
9-17: Remove the duplicate test vector to reduce redundant runs.
The dataset includes the same("ABC", 15)case twice.✂️ Suggested change
LETTER_TILE_POSSIBILITIES = [ ("AAB", 8), ("ABC", 15), - ("ABC", 15), ("CDB", 15), ("ZZZ", 3), ("AAABBC", 188), ("V", 1), ]
algorithms/dynamic_programming/max_profit_in_job_scheduling/README.md
Outdated
Show resolved
Hide resolved
datastructures/trees/binary/search_tree/test_binary_search_tree_valid.py
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@algorithms/sliding_window/minimum_window_substring/README.md`:
- Around line 24-26: The image alt texts in the README (lines with markdown like
"") are
mismatched — they read "Example 1", "Example 3", "Example 4" while the files are
_example_1, _example_2, _example_3 and the examples below are numbered 1–3;
update the alt text strings to match the file sequence and example numbers
(e.g., change the second and third image alt texts to "Example 2" and "Example
3") so the markdown image tags correspond to min_window_substring_example_1/2/3
and the numbered examples.
In `@algorithms/sliding_window/substring_concatenation/__init__.py`:
- Around line 48-58: The function find_substring_2 lacks guards for empty inputs
and will IndexError on len(words[0]); add an early return that checks if not s
or not words or not words[0] and return an empty list; ensure this check sits at
the top of find_substring_2 before computing word_len, num_words, total_len, or
using Counter so subsequent logic (word_len, total_len, word_count, result) is
safe.
In `@algorithms/sliding_window/substring_concatenation/README.md`:
- Line 10: Fix the typo in the example string: replace the incorrect token
`However,z"acdbef"` with `However, "acdbef"` in the README text so the stray `z`
is removed and the quoted example is formatted correctly.
🧹 Nitpick comments (4)
algorithms/two_pointers/sort_by_parity/test_sort_by_parity.py (1)
10-11: Test cases with all-even inputs violate problem constraints.
[100, 200, 300, 400]and[10, 100, 1000, 10000]contain no odd elements. The problem states "exactly half of the elements are even, and the other half are odd." These cases pass only because the even pointer outruns the bounds before a mismatch is detected, so they don't meaningfully exercise the swap logic. Consider replacing them with valid inputs (e.g., a mix of even/odd where the array is already correctly arranged) or documenting them as explicit out-of-spec edge-case tests.algorithms/sliding_window/minimum_window_substring/__init__.py (1)
113-113: Inconsistent casing:float("Inf")vsfloat("inf").Line 84 uses
float("inf")(lowercase) while line 113 usesfloat("Inf")(uppercase). Both are valid Python and evaluate equally, so this isn't a bug, but the inconsistency is a readability nit. Consider normalizing to lowercase"inf"to match the rest of the file (line 26 also uses lowercase).Suggested fix
- return "" if ans[0] == float("Inf") else s[ans[1] : ans[2] + 1] + return "" if ans[0] == float("inf") else s[ans[1] : ans[2] + 1]pymath/self_crossing/test_is_self_crossing.py (1)
6-13: Consider adding edge-case test inputs.The test suite covers the main crossing scenarios well, but consider adding tests for boundary conditions: an empty list
[], a very short list like[1]or[1, 2, 3](should returnFalseper thelen < 4guard), and all-zeros[0, 0, 0, 0].algorithms/sliding_window/substring_concatenation/README.md (1)
66-66: Minor: missing bold formatting on second case heading.The "Fourth line crosses the first" and "Sixth line crosses first" headings use
**bold**, but the "Fifth line meets first" heading on Line 66 does not.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…ADME.md Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your change:
Tree traversal algorithms
Checklist:
Fixes: #{$ISSUE_NO}.Summary by CodeRabbit