Graphs | Vertex & Graph implementations #77
Conversation
BrianLusina
added
Algorithm
WalkthroughThis pull request refactors the graph data structure module by extracting the monolithic Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
datastructures/graphs/vertex.py (2)
72-91: Degree calculation is incorrect.
- Early return triggers when either incoming or outgoing is empty; should be when both are empty.
- Use Edge.is_unweighted() per API; don’t treat weight 0 as unweighted.
- degrees = 0 - - if len(self.incoming_edges) == 0 or len(self.outgoing_edges) == 0: - return degrees - - seen_edges: Set = set() - - for edge in self.edges: - if edge not in seen_edges: - seen_edges.add(edge) - if not edge.weight: - degrees += 1 - - return degrees + if not self.edges: + return 0 + return sum(1 for edge in self.edges if edge.is_unweighted())
101-109: Directed-degree checks reference non-existent attributes.Use DirectedEdge and its source/destination fields (or edge.edge_type()). Current code uses edge.type and node_one/node_two, which don’t exist for DirectedEdge.
- if len(self.edges) == 0: + if not self.edges: return in_degrees - - for edge in self.edges: - if edge.type == EdgeType.DIRECTED and edge.node_two == self: - in_degrees += 1 + for edge in self.edges: + if isinstance(edge, DirectedEdge) and edge.destination == self: + in_degrees += 1 @@ - if len(self.edges) == 0: + if not self.edges: return out_degrees - - for edge in self.edges: - if edge.type == EdgeType.DIRECTED and edge.node_one == self: - out_degrees += 1 + for edge in self.edges: + if isinstance(edge, DirectedEdge) and edge.source == self: + out_degrees += 1Also applies to: 123-127
🧹 Nitpick comments (7)
datastructures/graphs/vertex.py (1)
58-69: Minor: simplify neighbours.No behavior change.
- nodes = [] - for vertex in self.adjacent_vertices.values(): - nodes.append(vertex) - - return nodes + return list(self.adjacent_vertices.values())datastructures/graphs/edge/edge_type.py (1)
4-10: Enum looks good; considerauto()and future-proofing.Current members are fine. Optional: use
auto()to avoid manual numbering and reduce churn when adding new types. If HyperEdge won’t encode direction, introduce a genericHYPERmember and align uses accordingly.from enum import Enum, unique @unique class EdgeType(Enum): - UNDIRECTED = 1 - DIRECTED = 2 - SELF = 3 - HYPER_DIRECTED = 4 - HYPER_UNDIRECTED = 5 + UNDIRECTED = auto() + DIRECTED = auto() + SELF = auto() + HYPER_DIRECTED = auto() + HYPER_UNDIRECTED = auto()datastructures/graphs/edge/edge_hyper.py (1)
13-26: Tighten generics: useList[T]where possible.
HyperEdgeisGeneric[T]but currently usesList[Any]. PreferList[T]fornodesand return type.- def __init__(self, nodes: List[Any], ... + def __init__(self, nodes: List[T], ... ... - def vertices(self) -> List[Any]: + def vertices(self) -> List[T]: return self.nodesdatastructures/graphs/edge/edge_self.py (1)
19-26: Minor: property name clarity.For a single node, consider
self.nodeoverself.node_onefor clarity.- self.node_one = node + self.node = node ... - return f"{super().__str__()}, Node: {self.node_one}" + return f"{super().__str__()}, Node: {self.node}" ... - def vertices(self) -> List[Any]: - return [self.node_one] + def vertices(self) -> List[Any]: + return [self.node]datastructures/graphs/edge/edge_directed.py (1)
27-28: Tighten generics: returnList[T].Since the edge is
Generic[T], return a typed list to improve correctness.- def vertices(self) -> List[Any]: + def vertices(self) -> List[T]: return [self.source, self.destination]datastructures/graphs/edge/edge_undirected.py (1)
8-11: Optional: Remove redundant Generic inheritance.Since
Edgealready inherits fromGeneric[T], explicitly inheriting fromGeneric[T]again inUndirectedEdgeis redundant. While not incorrect, it can be simplified.Apply this diff to simplify the inheritance:
-class UndirectedEdge(Edge, Generic[T]): +class UndirectedEdge(Edge[T]):datastructures/graphs/edge/edge.py (1)
1-7: Optional: Consolidate typing imports for better organization.The typing imports are split across lines 2 and 4, which slightly reduces readability.
Apply this diff to consolidate the imports:
from abc import ABC, abstractmethod -from typing import AnyStr, Union +from typing import AnyStr, Union, Any, Dict, Optional, Generic, TypeVar, List from .edge_type import EdgeType -from typing import Any, Dict, Optional, Generic, TypeVar, List from uuid import uuid4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
DIRECTORY.md(1 hunks)datastructures/graphs/__init__.py(1 hunks)datastructures/graphs/edge.py(0 hunks)datastructures/graphs/edge/__init__.py(1 hunks)datastructures/graphs/edge/edge.py(1 hunks)datastructures/graphs/edge/edge_directed.py(1 hunks)datastructures/graphs/edge/edge_hyper.py(1 hunks)datastructures/graphs/edge/edge_self.py(1 hunks)datastructures/graphs/edge/edge_type.py(1 hunks)datastructures/graphs/edge/edge_undirected.py(1 hunks)datastructures/graphs/graph.py(1 hunks)datastructures/graphs/test_vertex.py(1 hunks)datastructures/graphs/vertex.py(5 hunks)
💤 Files with no reviewable changes (1)
- datastructures/graphs/edge.py
🧰 Additional context used
🧬 Code graph analysis (10)
datastructures/graphs/edge/edge_self.py (5)
datastructures/graphs/edge/edge.py (3)
edge_type(28-29)Edge(9-36)vertices(35-36)datastructures/graphs/edge/edge_directed.py (2)
edge_type(24-25)vertices(27-28)datastructures/graphs/edge/edge_hyper.py (2)
edge_type(22-23)vertices(25-26)datastructures/graphs/edge/edge_undirected.py (2)
edge_type(23-24)vertices(26-27)datastructures/graphs/edge/edge_type.py (1)
EdgeType(5-10)
datastructures/graphs/edge/__init__.py (6)
datastructures/graphs/edge/edge.py (2)
Edge(9-36)edge_type(28-29)datastructures/graphs/edge/edge_directed.py (2)
edge_type(24-25)DirectedEdge(8-28)datastructures/graphs/edge/edge_hyper.py (2)
edge_type(22-23)HyperEdge(8-26)datastructures/graphs/edge/edge_self.py (2)
edge_type(22-23)SelfEdge(8-26)datastructures/graphs/edge/edge_undirected.py (2)
edge_type(23-24)UndirectedEdge(8-27)datastructures/graphs/edge/edge_type.py (1)
EdgeType(5-10)
datastructures/graphs/edge/edge.py (5)
datastructures/graphs/edge/edge_directed.py (2)
edge_type(24-25)vertices(27-28)datastructures/graphs/edge/edge_hyper.py (2)
edge_type(22-23)vertices(25-26)datastructures/graphs/edge/edge_self.py (2)
edge_type(22-23)vertices(25-26)datastructures/graphs/edge/edge_undirected.py (2)
edge_type(23-24)vertices(26-27)datastructures/graphs/edge/edge_type.py (1)
EdgeType(5-10)
datastructures/graphs/graph.py (3)
datastructures/stacks/__init__.py (1)
Stack(7-48)datastructures/graphs/vertex.py (2)
Vertex(8-127)neighbours(59-69)datastructures/graphs/edge/edge.py (1)
Edge(9-36)
datastructures/graphs/edge/edge_hyper.py (5)
datastructures/graphs/edge/edge.py (3)
edge_type(28-29)Edge(9-36)vertices(35-36)datastructures/graphs/edge/edge_directed.py (2)
edge_type(24-25)vertices(27-28)datastructures/graphs/edge/edge_self.py (2)
edge_type(22-23)vertices(25-26)datastructures/graphs/edge/edge_undirected.py (2)
edge_type(23-24)vertices(26-27)datastructures/graphs/edge/edge_type.py (1)
EdgeType(5-10)
datastructures/graphs/test_vertex.py (2)
datastructures/graphs/vertex.py (1)
Vertex(8-127)datastructures/graphs/edge/edge_undirected.py (1)
UndirectedEdge(8-27)
datastructures/graphs/edge/edge_directed.py (2)
datastructures/graphs/edge/edge.py (3)
edge_type(28-29)Edge(9-36)vertices(35-36)datastructures/graphs/edge/edge_type.py (1)
EdgeType(5-10)
datastructures/graphs/__init__.py (7)
datastructures/graphs/edge/edge.py (1)
Edge(9-36)datastructures/graphs/edge/edge_type.py (1)
EdgeType(5-10)datastructures/graphs/edge/edge_directed.py (1)
DirectedEdge(8-28)datastructures/graphs/edge/edge_undirected.py (1)
UndirectedEdge(8-27)datastructures/graphs/edge/edge_self.py (1)
SelfEdge(8-26)datastructures/graphs/edge/edge_hyper.py (1)
HyperEdge(8-26)datastructures/graphs/graph.py (1)
Graph(12-230)
datastructures/graphs/edge/edge_undirected.py (5)
datastructures/graphs/edge/edge.py (3)
edge_type(28-29)Edge(9-36)vertices(35-36)datastructures/graphs/edge/edge_directed.py (2)
edge_type(24-25)vertices(27-28)datastructures/graphs/edge/edge_hyper.py (2)
edge_type(22-23)vertices(25-26)datastructures/graphs/edge/edge_self.py (2)
edge_type(22-23)vertices(25-26)datastructures/graphs/edge/edge_type.py (1)
EdgeType(5-10)
datastructures/graphs/vertex.py (2)
datastructures/graphs/edge/edge.py (1)
Edge(9-36)datastructures/graphs/edge/edge_type.py (1)
EdgeType(5-10)
🪛 markdownlint-cli2 (0.18.1)
DIRECTORY.md
198-198: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
199-199: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
200-200: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
201-201: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
202-202: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
203-203: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
204-204: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
205-205: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
206-206: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🔇 Additional comments (8)
datastructures/graphs/__init__.py (1)
1-14: Public API facade looks good.Re-exports are clear and usable. No action needed.
datastructures/graphs/edge/__init__.py (1)
1-6: Public exports OK; verify enum usage across modules.Re-exports look correct. Cross-check
edge_undirected.py: snippet showsEdgeType.Undirected(wrong case). Should beEdgeType.UNDIRECTEDor code will break.Suggested fix in
datastructures/graphs/edge/edge_undirected.py:- def edge_type(self) -> EdgeType: - return EdgeType.Undirected + def edge_type(self) -> EdgeType: + return EdgeType.UNDIRECTEDdatastructures/graphs/edge/edge_undirected.py (3)
1-6: Imports and type variable are set up correctly.The imports are well-organized and the TypeVar declaration follows best practices for generic classes.
20-21: LGTM!The string representation correctly extends the base class and includes the undirected edge's node information.
26-27: LGTM!The
verticesmethod correctly returns both nodes of the undirected edge, consistent with the pattern used in other edge implementations.datastructures/graphs/edge/edge.py (3)
24-25: LGTM!The string representation provides clear, informative output for debugging and logging.
27-29: LGTM!The abstract methods
edge_typeandverticescorrectly define the interface contract that all edge subclasses must implement. The use of@abstractmethodensures subclasses provide concrete implementations.Also applies to: 34-36
31-32: LGTM!The
is_unweightedhelper method provides a clean API for checking whether an edge has a weight, which is useful for graph algorithms that need to distinguish between weighted and unweighted edges.
| def __init__(self, source: Any, destination: Any, weight: Optional[Union[int, float]] = None, | ||
| properties: Optional[Dict[str, Any]] = None, | ||
| identifier: AnyStr = uuid4()): | ||
| super().__init__(weight, properties, identifier) | ||
| self.source = source |
There was a problem hiding this comment.
Bug: uuid4() as default arg; use late binding and UUID typing.
Same issue as other edges.
-from typing import AnyStr, Union, Dict, Optional, Generic, TypeVar, List, Any
-from uuid import uuid4
+from typing import Union, Dict, Optional, Generic, TypeVar, List, Any
+from uuid import uuid4, UUID
...
- def __init__(self, source: Any, destination: Any, weight: Optional[Union[int, float]] = None,
- properties: Optional[Dict[str, Any]] = None,
- identifier: AnyStr = uuid4()):
- super().__init__(weight, properties, identifier)
+ def __init__(self, source: Any, destination: Any, weight: Optional[Union[int, float]] = None,
+ properties: Optional[Dict[str, Any]] = None,
+ identifier: Optional[UUID] = None):
+ super().__init__(weight, properties, identifier or uuid4())Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In datastructures/graphs/edge/edge_directed.py around lines 14 to 18, the
constructor uses uuid4() as a default argument which causes a single UUID to be
shared across calls and the identifier typing should be a UUID; change the
signature to accept identifier: Optional[UUID] = None (import UUID from uuid or
typing), and inside __init__ set self.identifier = identifier or uuid4(); keep
calling super() with the resolved identifier and ensure imports and typing match
other edge classes for consistency.
| def __init__(self, nodes: List[Any], weight: Optional[Union[int, float]] = None, | ||
| properties: Optional[Dict[str, Any]] = None, | ||
| identifier: AnyStr = uuid4()): | ||
| super().__init__(weight, properties, identifier) |
There was a problem hiding this comment.
Bug: uuid4() used as a default argument (evaluated at import time).
This gives the same id for all instances created without an explicit identifier.
-from typing import AnyStr, Union, Dict, Optional, Generic, TypeVar, List, Any
-from uuid import uuid4
+from typing import Union, Dict, Optional, Generic, TypeVar, List, Any
+from uuid import uuid4, UUID
...
- def __init__(self, nodes: List[Any], weight: Optional[Union[int, float]] = None,
- properties: Optional[Dict[str, Any]] = None,
- identifier: AnyStr = uuid4()):
- super().__init__(weight, properties, identifier)
+ def __init__(self, nodes: List[Any], weight: Optional[Union[int, float]] = None,
+ properties: Optional[Dict[str, Any]] = None,
+ identifier: Optional[UUID] = None):
+ super().__init__(weight, properties, identifier or uuid4())Also align base Edge to accept Optional[UUID] (see follow‑up note below).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In datastructures/graphs/edge/edge_hyper.py around lines 13 to 16, the
constructor uses uuid4() as a default argument causing the same UUID to be
shared across all instances; change the signature so identifier: Optional[UUID]
= None and inside __init__ set identifier = identifier or uuid4() before calling
super().__init__; also update the base Edge type annotations and signature to
accept Optional[UUID] (not just AnyStr) so the propagated identifier type is
Optional[UUID] and maintain existing behavior when an identifier is provided.
| def edge_type(self) -> EdgeType: | ||
| return EdgeType.SELF |
There was a problem hiding this comment.
Bug: HyperEdge returns the wrong EdgeType.
edge_type() returns EdgeType.SELF. Use a hyper variant (likely undirected unless you carry direction).
- def edge_type(self) -> EdgeType:
- return EdgeType.SELF
+ def edge_type(self) -> EdgeType:
+ return EdgeType.HYPER_UNDIRECTED📝 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.
| def edge_type(self) -> EdgeType: | |
| return EdgeType.SELF | |
| def edge_type(self) -> EdgeType: | |
| return EdgeType.HYPER_UNDIRECTED |
🤖 Prompt for AI Agents
In datastructures/graphs/edge/edge_hyper.py around lines 22-23, the edge_type()
method incorrectly returns EdgeType.SELF; change it to return the hyper variant
(e.g., EdgeType.HYPER or EdgeType.UNDIRECTED_HYPER depending on the enum name
used in the codebase) so HyperEdge reports the correct type; update any imports
or references if necessary and run tests to ensure consumers expecting hyper
edges now see the correct enum value.
| def __init__(self, node: Any, weight: Optional[Union[int, float]] = None, | ||
| properties: Optional[Dict[str, Any]] = None, | ||
| identifier: AnyStr = uuid4()): | ||
| super().__init__(weight, properties, identifier) | ||
| self.node_one = node |
There was a problem hiding this comment.
Bug: uuid4() as default arg; switch to Optional[UUID] and late binding.
Avoid import-time evaluation and align identifier typing to UUID.
-from typing import AnyStr, Union, Dict, Optional, Generic, TypeVar, List, Any
-from uuid import uuid4
+from typing import Union, Dict, Optional, Generic, TypeVar, List, Any
+from uuid import uuid4, UUID
...
- def __init__(self, node: Any, weight: Optional[Union[int, float]] = None,
- properties: Optional[Dict[str, Any]] = None,
- identifier: AnyStr = uuid4()):
- super().__init__(weight, properties, identifier)
+ def __init__(self, node: Any, weight: Optional[Union[int, float]] = None,
+ properties: Optional[Dict[str, Any]] = None,
+ identifier: Optional[UUID] = None):
+ super().__init__(weight, properties, identifier or uuid4())🤖 Prompt for AI Agents
In datastructures/graphs/edge/edge_self.py around lines 13 to 17, the
constructor uses uuid4() as a default argument which evaluates at import time
and the identifier type should be a UUID; change the signature to accept
identifier: Optional[UUID] = None (import UUID from uuid or typing) and in the
body do identifier = uuid4() if identifier is None, then pass that UUID to
super().__init__; also update the type hints and any imports accordingly so
identifier is typed as UUID and default is lazily generated.
| def __init__(self, node_one: Any, node_two: Any, weight: Optional[Union[int, float]] = None, | ||
| properties: Optional[Dict[str, Any]] = None, | ||
| identifier: AnyStr = uuid4()): | ||
| super().__init__(weight, properties, identifier) | ||
| self.node_one = node_one | ||
| self.node_two = node_two |
There was a problem hiding this comment.
Critical: Mutable default argument and type mismatch for identifier parameter.
Two issues with the identifier parameter:
-
Mutable default:
uuid4()is called once at function definition time, causing all instances created without an explicit identifier to share the same UUID. This will lead to incorrect edge identification and potential data corruption. -
Type mismatch: The parameter is typed as
AnyStr(which isstr | bytes), butuuid4()returns aUUIDobject.
Apply this diff to fix both issues:
- def __init__(self, node_one: Any, node_two: Any, weight: Optional[Union[int, float]] = None,
- properties: Optional[Dict[str, Any]] = None,
- identifier: AnyStr = uuid4()):
+ def __init__(self, node_one: Any, node_two: Any, weight: Optional[Union[int, float]] = None,
+ properties: Optional[Dict[str, Any]] = None,
+ identifier: Optional[Union[str, UUID]] = None):
+ if identifier is None:
+ identifier = uuid4()
super().__init__(weight, properties, identifier)Note: The base Edge class has the same issue and should be fixed there as well.
🤖 Prompt for AI Agents
In datastructures/graphs/edge/edge_undirected.py lines 13-18, the constructor
currently uses uuid4() as a default argument and types identifier as AnyStr;
change the signature to accept identifier: Optional[UUID] = None (import UUID
from uuid), and inside __init__ do if identifier is None: identifier = uuid4()
before calling super().__init__; this avoids a single shared UUID and matches
the UUID return type; also make the same change in the base Edge class (use
Optional[UUID] = None and assign uuid4() inside the constructor).
| def find_all_paths( | ||
| self, node_one: Vertex, node_two: Vertex, path: List = None | ||
| ) -> list: | ||
| """ | ||
| Finds all paths between node_one and node_two, where node_one is the start & node_two is the end | ||
| :param node_one Graph Node | ||
| :param node_two Graph Node | ||
| :param path | ||
| """ | ||
| if path is None: | ||
| path = [] | ||
| path = path + [node_one] | ||
|
|
||
| if node_one.data == node_two.data: | ||
| return [path] | ||
|
|
||
| if node_one.data not in self.adjacency_list: | ||
| return [] | ||
|
|
||
| paths = [] | ||
|
|
||
| for node in self.adjacency_list[node_one.data]: | ||
| if node not in path: | ||
| newpaths = self.find_all_paths(Vertex(node), node_two, path) | ||
| for newpath in newpaths: | ||
| paths.append(newpath) | ||
|
|
||
| return paths |
There was a problem hiding this comment.
find_all_paths(): indexes with node_one.data and constructs new Vertex() erroneously.
- if path is None:
- path = []
- path = path + [node_one]
-
- if node_one.data == node_two.data:
+ if path is None:
+ path = []
+ path = path + [node_one]
+
+ if node_one == node_two:
return [path]
-
- if node_one.data not in self.adjacency_list:
- return []
-
- paths = []
-
- for node in self.adjacency_list[node_one.data]:
- if node not in path:
- newpaths = self.find_all_paths(Vertex(node), node_two, path)
- for newpath in newpaths:
- paths.append(newpath)
-
- return paths
+ paths: List[List[Vertex]] = []
+ for neighbour in self.adjacency_list.get(node_one, []):
+ if neighbour not in path:
+ new_paths = self.find_all_paths(neighbour, node_two, path)
+ paths.extend(new_paths)
+ return paths🤖 Prompt for AI Agents
In datastructures/graphs/graph.py around lines 169-196, find_all_paths is
indexing the adjacency_list with node_one.data and constructing new Vertex(node)
when recursing; instead, use the actual Vertex objects stored in the
adjacency_list (or consistently map between data->Vertex first) so you don't
rebuild vertices or mix data keys with Vertex keys. Update the function to look
up neighbors with self.adjacency_list.get(node_one) (or translate node_one.data
to the Vertex once at the top), iterate neighbor Vertex instances, compare
neighbor != node (or compare their unique id/identity rather than .data), and
pass the neighbor Vertex directly into the recursive call (no new Vertex(...)).
Ensure adjacency_list usage is consistent (either keys/values are Vertex objects
or you convert data->Vertex once) and adjust membership checks against path to
use Vertex objects.
| def find_shortest_path( | ||
| self, node_one: Vertex, node_two: Vertex, path: List = None | ||
| ) -> Union[List, None]: | ||
| """ | ||
| Finds the shortest path between 2 nodes in the graph | ||
| """ | ||
| if path is None: | ||
| path = [] | ||
|
|
||
| path = path + [node_one] | ||
|
|
||
| if node_one.data == node_two.data: | ||
| return path | ||
|
|
||
| if node_one.data not in self.adjacency_list: | ||
| return None | ||
|
|
||
| shortest = None | ||
|
|
||
| for node in self.adjacency_list[node_one]: | ||
| if node.data not in path: | ||
| newpath = self.find_shortest_path(node, node_two, path) | ||
| if newpath: | ||
| if not shortest or len(newpath) < len(shortest): | ||
| shortest = newpath | ||
|
|
||
| return shortest |
There was a problem hiding this comment.
find_shortest_path(): same keying issues as above.
- if path is None:
- path = []
-
- path = path + [node_one]
-
- if node_one.data == node_two.data:
+ if path is None:
+ path = []
+ path = path + [node_one]
+
+ if node_one == node_two:
return path
-
- if node_one.data not in self.adjacency_list:
- return None
-
- shortest = None
-
- for node in self.adjacency_list[node_one]:
- if node.data not in path:
- newpath = self.find_shortest_path(node, node_two, path)
- if newpath:
- if not shortest or len(newpath) < len(shortest):
- shortest = newpath
-
- return shortest
+ shortest: Union[List[Vertex], None] = None
+ for neighbour in self.adjacency_list.get(node_one, []):
+ if neighbour not in path:
+ new_path = self.find_shortest_path(neighbour, node_two, path)
+ if new_path and (shortest is None or len(new_path) < len(shortest)):
+ shortest = new_path
+ return shortest📝 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.
| def find_shortest_path( | |
| self, node_one: Vertex, node_two: Vertex, path: List = None | |
| ) -> Union[List, None]: | |
| """ | |
| Finds the shortest path between 2 nodes in the graph | |
| """ | |
| if path is None: | |
| path = [] | |
| path = path + [node_one] | |
| if node_one.data == node_two.data: | |
| return path | |
| if node_one.data not in self.adjacency_list: | |
| return None | |
| shortest = None | |
| for node in self.adjacency_list[node_one]: | |
| if node.data not in path: | |
| newpath = self.find_shortest_path(node, node_two, path) | |
| if newpath: | |
| if not shortest or len(newpath) < len(shortest): | |
| shortest = newpath | |
| return shortest | |
| def find_shortest_path( | |
| self, node_one: Vertex, node_two: Vertex, path: List = None | |
| ) -> Union[List, None]: | |
| """ | |
| Finds the shortest path between 2 nodes in the graph | |
| """ | |
| if path is None: | |
| path = [] | |
| path = path + [node_one] | |
| if node_one == node_two: | |
| return path | |
| shortest: Union[List[Vertex], None] = None | |
| for neighbour in self.adjacency_list.get(node_one, []): | |
| if neighbour not in path: | |
| new_path = self.find_shortest_path(neighbour, node_two, path) | |
| if new_path and (shortest is None or len(new_path) < len(shortest)): | |
| shortest = new_path | |
| return shortest |
🤖 Prompt for AI Agents
In datastructures/graphs/graph.py around lines 198-224, the method mixes Vertex
objects and their .data keys (e.g. checking node_one.data in adjacency_list but
indexing self.adjacency_list[node_one], and comparing node.data to items in
path), causing incorrect membership/equality checks; fix by treating vertices
consistently as Vertex objects: check membership with node_one in
self.adjacency_list, compare nodes with equality on the Vertex (e.g. node_one ==
node_two), store Vertex instances in path (no .data), and use node not node.data
when checking if a neighbor is already in path or when recursing/indexing
adjacency_list.
| class VertexTestCases(unittest.TestCase): | ||
| def test_1(self): | ||
| node_one = Vertex(data=1) | ||
| node_two = Vertex(data=2) | ||
| edge = UndirectedEdge(node_one=node_one, node_two=node_two) | ||
|
|
||
| self.assertEqual(True, False) # add assertion here | ||
|
|
||
|
|
||
| if __name__ == '__main__': | ||
| unittest.main() |
There was a problem hiding this comment.
Replace placeholder failing test with real Vertex checks.
Add adjacency and equality/hash tests.
-class VertexTestCases(unittest.TestCase):
- def test_1(self):
- node_one = Vertex(data=1)
- node_two = Vertex(data=2)
- edge = UndirectedEdge(node_one=node_one, node_two=node_two)
-
- self.assertEqual(True, False) # add assertion here
+class VertexTestCases(unittest.TestCase):
+ def test_add_adjacent_vertex_is_mutual(self):
+ v1 = Vertex(data=1)
+ v2 = Vertex(data=2)
+ v1.add_adjacent_vertex(v2)
+ self.assertIn(v2, v1.neighbours)
+ self.assertIn(v1, v2.neighbours)
+
+ def test_eq_and_hash_by_id(self):
+ v1 = Vertex(data=1)
+ v2 = Vertex(data=99, identifier=v1.id)
+ self.assertEqual(v1, v2)
+ self.assertIn(v2, {v1})📝 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.
| class VertexTestCases(unittest.TestCase): | |
| def test_1(self): | |
| node_one = Vertex(data=1) | |
| node_two = Vertex(data=2) | |
| edge = UndirectedEdge(node_one=node_one, node_two=node_two) | |
| self.assertEqual(True, False) # add assertion here | |
| if __name__ == '__main__': | |
| unittest.main() | |
| class VertexTestCases(unittest.TestCase): | |
| def test_add_adjacent_vertex_is_mutual(self): | |
| v1 = Vertex(data=1) | |
| v2 = Vertex(data=2) | |
| v1.add_adjacent_vertex(v2) | |
| self.assertIn(v2, v1.neighbours) | |
| self.assertIn(v1, v2.neighbours) | |
| def test_eq_and_hash_by_id(self): | |
| v1 = Vertex(data=1) | |
| v2 = Vertex(data=99, identifier=v1.id) | |
| self.assertEqual(v1, v2) | |
| self.assertIn(v2, {v1}) | |
| if __name__ == '__main__': | |
| unittest.main() |
🤖 Prompt for AI Agents
In datastructures/graphs/test_vertex.py around lines 6 to 16, replace the
failing placeholder assertion with real tests: create two Vertex instances and
an UndirectedEdge between them, then assert that each vertex's
adjacency/container includes the other (and the edge if your API stores edges),
assert that two Vertex instances constructed with the same data compare equal
and produce the same hash, and assert that vertices with different data are not
equal; use assertTrue/assertFalse/assertEqual/assertIn/assertNotIn as
appropriate to validate adjacency and equality/hash behavior.
| def __init__( | ||
| self, | ||
| data: T, | ||
| incoming_edges: Set[Edge], | ||
| outgoing_edges: Set[Edge], | ||
| properties: Optional[Dict[str, Any]] = None, | ||
| identifier: Any = uuid4(), | ||
| properties: Optional[Dict[str, Any]] = None, | ||
| incoming_edges: Optional[Set[Edge]] = None, | ||
| outgoing_edges: Optional[Set[Edge]] = None, | ||
| ): | ||
| if outgoing_edges is None: | ||
| outgoing_edges = set() | ||
| if incoming_edges is None: | ||
| incoming_edges = set() | ||
| self.id = identifier | ||
| self.data = data | ||
| self.incoming_edges = incoming_edges | ||
| self.outgoing_edges = outgoing_edges | ||
| self.edges = self.incoming_edges.union(self.outgoing_edges) | ||
| self.adjacent_vertices: Dict[str, 'Vertex'] = {} | ||
| self.properties = properties | ||
|
|
There was a problem hiding this comment.
Default-id bug, stale edges, and key typing. Make Vertex hashable.
- Avoid uuid4() as a default arg (same id across instances).
- Don’t cache edges; compute union on demand.
- adjacent_vertices key type should match id type.
- Add hash to use Vertex as dict key (Graph uses it).
-from .edge import Edge, EdgeType
+from .edge import Edge, EdgeType, DirectedEdge
@@
- def __init__(
+ def __init__(
self,
data: T,
- identifier: Any = uuid4(),
+ identifier: Any = None,
properties: Optional[Dict[str, Any]] = None,
incoming_edges: Optional[Set[Edge]] = None,
outgoing_edges: Optional[Set[Edge]] = None,
):
@@
- self.id = identifier
+ self.id = uuid4() if identifier is None else identifier
self.data = data
@@
- self.edges = self.incoming_edges.union(self.outgoing_edges)
- self.adjacent_vertices: Dict[str, 'Vertex'] = {}
+ self.adjacent_vertices: Dict[Any, 'Vertex'] = {}
self.properties = properties
+
+ @property
+ def edges(self) -> Set[Edge]:
+ return self.incoming_edges | self.outgoing_edges
@@
- def __eq__(self, other: "Vertex") -> bool:
- return self.id == other.id
+ def __eq__(self, other: object) -> bool:
+ if not isinstance(other, Vertex):
+ return NotImplemented
+ return self.id == other.id
+
+ def __hash__(self) -> int:
+ return hash(self.id)Committable suggestion skipped: line range outside the PR's diff.
| * Edge | ||
| * [Edge](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge.py) | ||
| * [Edge Directed](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_directed.py) | ||
| * [Edge Hyper](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_hyper.py) | ||
| * [Edge Self](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_self.py) | ||
| * [Edge Type](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_type.py) | ||
| * [Edge Undirected](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_undirected.py) | ||
| * [Graph](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/graph.py) | ||
| * [Test Vertex](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/test_vertex.py) |
There was a problem hiding this comment.
Fix list indentation to satisfy MD007 and match surrounding style.
Current bullets under “Graphs > Edge” are over‑indented. Apply:
- * Edge
- * [Edge](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge.py)
- * [Edge Directed](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_directed.py)
- * [Edge Hyper](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_hyper.py)
- * [Edge Self](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_self.py)
- * [Edge Type](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_type.py)
- * [Edge Undirected](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_undirected.py)
- * [Graph](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/graph.py)
- * [Test Vertex](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/test_vertex.py)
+ * Edge
+ * [Edge](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge.py)
+ * [Edge Directed](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_directed.py)
+ * [Edge Hyper](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_hyper.py)
+ * [Edge Self](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_self.py)
+ * [Edge Type](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_type.py)
+ * [Edge Undirected](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_undirected.py)
+ * [Graph](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/graph.py)
+ * [Test Vertex](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/test_vertex.py)📝 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.
| * Edge | |
| * [Edge](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge.py) | |
| * [Edge Directed](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_directed.py) | |
| * [Edge Hyper](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_hyper.py) | |
| * [Edge Self](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_self.py) | |
| * [Edge Type](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_type.py) | |
| * [Edge Undirected](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_undirected.py) | |
| * [Graph](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/graph.py) | |
| * [Test Vertex](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/test_vertex.py) | |
| * Edge | |
| * [Edge](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge.py) | |
| * [Edge Directed](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_directed.py) | |
| * [Edge Hyper](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_hyper.py) | |
| * [Edge Self](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_self.py) | |
| * [Edge Type](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_type.py) | |
| * [Edge Undirected](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_undirected.py) | |
| * [Graph](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/graph.py) | |
| * [Test Vertex](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/test_vertex.py) |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
198-198: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
199-199: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
200-200: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
201-201: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
202-202: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
203-203: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
204-204: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
205-205: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
206-206: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🤖 Prompt for AI Agents
In DIRECTORY.md around lines 198 to 206, the six bullet links under the "Edge"
subheading are over‑indented and break MD007; unindent those lines so the list
items for Edge are at the same indentation level as the following top-level list
entries (e.g., Graph and Test Vertex). Ensure the bullets use the same number of
leading spaces/tabs as the surrounding lists (consistent indentation style) so
the nested list structure and MD007 lint rule are satisfied.
… of a queue in find closed value
…fixes to naive implementation
Adds validation checks for the reverse_between method of the singly linked list class and tests to validate these changes. Additionally adds linting fixes to the code base
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
algorithms/arrays/two_sum/__init__.py (1)
33-61: Apply the same type safety fix totwo_sum_with_pointers.This function has the same implicit
Nonereturn issue that was just fixed intwo_sum. It should also explicitly return an empty list when no pair is found.Apply this diff:
else: last_pointer -= 1 + return []puzzles/arrays/maxlen_contiguous_binary_subarray/test_maxlen_contiguous_binary_subarray.py (1)
53-104: Dead code with critical errors.The
tests()function is defined but never invoked, making it unreachable. Additionally, it contains several critical issues:
- Line 80:
shuffle()is not imported. Should berandom.shuffle().- Lines 82, 92:
test.assert_equals()references an undefinedtestobject.- Lines 82, 92:
binarray()function is not defined or imported.If this function is intended for use, apply these fixes and ensure it's called:
def shuffs(s): - shuffle(s) + random.shuffle(s) answer = check(s) - test.assert_equals(binarray(s.copy()), answer) + self.assertEqual(find_max_length(s.copy()), answer)answer = check(s) - test.assert_equals(binarray(s.copy()), answer) + self.assertEqual(find_max_length(s.copy()), answer) if flag:If this code is no longer needed, remove the entire
tests()function and therandomimport.Do you want me to help refactor this into proper unittest test methods, or would you prefer to remove it entirely?
algorithms/two_pointers/sort_colors/__init__.py (1)
40-72: Syncsort_colors_v2docstring with implementation and clarify its role vssort_colors.The implementation looks correct (standard Dutch national flag), but:
- The docstring describes a “two pointers: low and high” algorithm that iterates with
low, which doesn’t match the actual code usingred,blue, and indexi.sort_colors_v2is effectively the same algorithm and complexity assort_colors, just with different variable names, so the distinction between the two functions isn’t obvious from the docs.I’d suggest:
Update the docstring so it reflects the actual implementation and makes clear how v2 differs conceptually from
sort_colors(or explicitly state it’s just an alternative formulation).Optionally add unit tests for
sort_colors_v2alongside the existing tests to ensure both implementations stay in sync.Example docstring tweak:
def sort_colors_v2(colors: List[int]) -> List[int]: - """ - Sorts a list of 0s, 1s, and 2s in place using two pointers - ... - The algorithm works by iterating through the list with the low pointer. If it encounters a 0, it simply increments - low. If it encounters a 2, it decrements high. If it encounters a 1, it swaps it with the element at the high index - and decrements high. This swapping process ensures that all the 1s are placed between the 0s and the 2s. - """ + """ + Sorts a list of 0s, 1s, and 2s in place using boundary pointers. + + This variant also uses the Dutch national flag idea: `red` tracks the next + position for 0, `blue` tracks the next position for 2, and `i` scans the + list, swapping elements into the correct region. + """If you’d like, I can also sketch matching tests for
sort_colors_v2based on the existingsort_colorstest cases.algorithms/arrays/subsequence/__init__.py (1)
40-72: Align v2 edge-case behavior and docstring with existing API contract
is_valid_subsequence_v2’s core two-pointer logic is correct for typical non-empty inputs, but there are a couple of contract/consistency nits:
- The docstring states
arrayandsequenceare non-empty, andis_valid_subsequenceenforces this (returnsFalsewhen either is empty). In contrast, v2 currently treats an emptysequenceas a valid subsequence (returnsTruewhensequence == [], even ifarrayis empty). That means callers will see different results between v1 and v2 on the same inputs outside the “non-empty” precondition.- The time/space complexity comments still describe a “iterate array then sequence” style algorithm; v2 actually does a single pass with two indices.
If you want v1 and v2 to be interchangeable, consider either (a) adding the same empty-input guard as v1, or (b) explicitly relaxing the precondition in both functions’ docstrings and updating tests to cover the empty-sequence case. Also, you can slightly tighten the tail by returning the comparison directly:
- if i == len(sequence): - return True - return False + return i == len(sequence)datastructures/trees/binary/search_tree/__init__.py (1)
445-462: Fix incorrect in-order traversal implementation.The method has multiple issues:
- The
nodeparameter is never used; the method always operates onself.root- Recursive calls on lines 456 and 461 discard their return values
- The result only contains the root's data, not a full traversal
Apply this fix to implement correct recursive in-order traversal:
def in_order_recurse(self, node: BinaryTreeNode) -> List[T]: """ Another type of Depth First Search (DFS) that traverses the tree from the left to middle to right of the tree. This type of search will begin at the left node and check if that node has a left child and continually check until that left node is a leaf(has no children) and will then print its data and "bubble up" back to the current node and execute that (in this case print it) and then print the right node. The same procedure is executed for the right side of the tree. """ result = [] - if self.root: - if self.root.left: - self.in_order_recurse(self.root.left) + if node: + if node.left: + result.extend(self.in_order_recurse(node.left)) - result.append(self.root.data) + result.append(node.data) - if self.root.right: - self.in_order_recurse(self.root.right) + if node.right: + result.extend(self.in_order_recurse(node.right)) return resultdatastructures/trees/heaps/binary/min_heap/__init__.py (1)
62-81: Fix the index mapping logic in__bubble_upand__bubble_down— current implementation causes KeyError.The double-indexing in lines 73–79 is incorrect. After swapping heap elements, attempting to use
self.idx_of_element[self.heap[idx]]as a key fails becauseself.idx_of_elementmapsHeapNodeobjects to integers, not integers to values. This causes KeyError whenconnect_sticks_2callsinsert_datarepeatedly.Correct the index updates to directly map the swapped nodes to their new positions:
parent_node = self.heap[parent_idx] child_node = self.heap[idx] self.idx_of_element[parent_node] = idx self.idx_of_element[child_node] = parent_idxApply the same fix to
__bubble_down(lines 52–58).datastructures/trees/binary/node.py (1)
163-176: Return type annotation inconsistent with implementation.The return type
List["BinaryTreeNode"] | NonesuggestsNonecan be returned, but all branches return lists (including empty[]on line 176). Either remove| Nonefrom the type hint or update to always return a list.@property - def children(self) -> List["BinaryTreeNode"] | None: + def children(self) -> List["BinaryTreeNode"]: """Returns children of this node. Returns: List: children of this node in a list """
🟡 Minor comments (12)
datastructures/trees/trie/suffix/suffix_tree_node.py-14-21 (1)
14-21: Remove unusedbest_indexattribute.The
best_indexattribute (line 17) is initialized but never used. Thesearch_best_index()method exclusively usesbest_infoto track and return the best word match, makingbest_indexdead code.datastructures/trees/binary/search_tree/__init__.py-252-301 (1)
252-301: Add input validation for k.The method lacks validation for the input parameter
k. Consider adding checks for edge cases such ask <= 0orkexceeding the number of nodes in the tree. While the current implementation returnsNonefor invalid inputs, this behavior should be documented or made explicit through validation.Consider applying this enhancement:
def find_kth_largest(self, k: int) -> Optional[BinaryTreeNode]: """ Finds the kth largest value in a binary search tree. This uses a reverse in order traversal moving from right to root to left until the kth largest node can be found. We don't have to traverse the whole tree since binary search trees are already ordered following the property of the right subtree has nodes which have the left sub-tree always less than their parent and the right subtree has nodes with values that are either equal to or greater than the parent. With this property in mind, we perform a reverse in order traversal to be able to move from right to root to left to find the kth largest node in the tree. Complexity: Time: O(h + k): where h is the height of the tree and k is the input Space: O(h): where h is the height of the tree. Args: k (int): The kth largest value to find Returns: Optional[BinaryTreeNode]: The kth largest node """ + if k <= 0: + return None # This is a helper class that helps to track the algorithm's progress of traversing the treealgorithms/arrays/two_sum_less_k/__init__.py-10-10 (1)
10-10: Misleading docstring: input does not need to be pre-sorted.The docstring states
nums (List[int]): A sorted list of integers, but the function sorts the input at line 18. Either update the docstring to clarify that the input does not need to be sorted, or remove the sorting step if the precondition is that the input must be sorted.Apply this diff to fix the docstring:
- nums (List[int]): A sorted list of integers + nums (List[int]): A list of integersalgorithms/search/binary_search/maxruntime_n_computers/README.md-41-48 (1)
41-48: Fix README pseudo-code and complexity typosThe algorithm description is accurate, but there are a few typos that could confuse readers:
- Line 43:
rughtshould beright.- Lines 62–63: extra closing parentheses in
O(n⋅logT))andT = sum(batteries) // n).- Line 68:
"os"→"is".You could update these like:
- - Calculate `mid = right - (rught - left) // 2` (biases the midpoint toward the higher end to avoid infinite loops). + - Calculate `mid = right - (right - left) // 2` (biases the midpoint toward the higher end to avoid infinite loops). - The time complexity of the solution is `O(n⋅logT))`, where `n` is the number of batteries and `T` is the total power of - one computer, `T = sum(batteries) // n)`. + The time complexity of the solution is `O(n ⋅ log T)`, where `n` is the number of batteries and `T` is the total power of + one computer, `T = sum(batteries) // n`. - The space complexity of the solution os `O(1)` because no extra space is used + The space complexity of the solution is `O(1)` because no extra space is usedAs indicated by the static analysis hints.
Also applies to: 60-68
puzzles/permutations_check/test_check.py-31-45 (1)
31-45: Fixtures reformat is good, but one game length and some docstrings look inconsistentThe multiline
gamesliterals intest_4/test_5are clearer and easier to maintain, but there are a couple of details worth checking:
- In
test_4, the first game[1, 6, 4, 5, 2]has only 5 entries whilen = 6. If each game is meant to be a full permutation of1..n, this likely wants a sixth value (perhaps3?) to avoid a silent mismatch betweennand the fixture data.- The docstrings for
test_3andtest_4say “should return False”, but the corresponding assertions useassertTrue. Either the expected outcome or the docstring is stale; consider aligning them to avoid confusion for future readers.Also applies to: 47-61
puzzles/heap/min_cost_to_connect_sticks/__init__.py-45-86 (1)
45-86: Correct the docstring: wrong example and space complexity.Two issues in the docstring:
The example is incorrect (same issue as in
connect_sticks). The minimum cost for[3, 4, 5]is 19, not 12.The space complexity claim of O(1) is incorrect for this implementation. Lines 62-65 create a new list of
HeapNodeobjects, requiring O(n) space.Apply this diff to correct the docstring:
- For example, given the list [3, 4, 5], the minimum cost to connect all sticks is 12 (3 + 4 + 5). + For example, given the list [3, 4, 5], the minimum cost to connect all sticks is 19. The time complexity of this function is O(nlogn), where n is the number of sticks. - The space complexity of this function is O(1), because the heap is built in place from the input list + The space complexity of this function is O(n), because it creates HeapNode objects for each stick.The implementation logic is correct, though it duplicates the algorithm from
connect_sticks.Consider whether maintaining two implementations is necessary, or if one sufficiently demonstrates the algorithm.
puzzles/heap/min_cost_to_connect_sticks/__init__.py-6-42 (1)
6-42: Correct the example in the docstring.The docstring example is incorrect. For
[3, 4, 5], the minimum cost is 19, not 12:
- First merge: 3 + 4 = 7 (cost: 7)
- Second merge: 7 + 5 = 12 (cost: 12)
- Total cost: 7 + 12 = 19
Apply this diff to correct the example:
- For example, given the list [3, 4, 5], the minimum cost to connect all sticks is 12 (3 + 4 + 5). + For example, given the list [3, 4, 5], the minimum cost to connect all sticks is 19.The implementation logic is correct and efficiently uses in-place heapification.
DIRECTORY.md-251-251 (1)
251-251: Typo: "Revese" should be "Reverse".The filename reference contains a typo.
- * [Test Singly Linked Revese](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/linked_lists/singly_linked_list/test_singly_linked_revese.py) + * [Test Singly Linked Reverse](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/linked_lists/singly_linked_list/test_singly_linked_reverse.py)Note: This may require renaming the actual file as well.
datastructures/streams/stream_checker/README.md-6-7 (1)
6-7: Incorrect example: "cat" is not in the word list.The example states
words = ["dog"]but then claims the suffix "cat" matches the word "cat" from the list. Since "cat" is not in["dog"], this example is incorrect. Consider updating towords = ["dog", "cat"]or choosing a matching example like detecting "dog" in stream "adog".-For example, if words = ["dog"] and the stream adds the characters 'd', 'c', 'a' , and 't' in sequence, the algorithm -should detect that the suffix "cat" of the stream "dcat" matches the word "cat" from the list. +For example, if words = ["cat", "dog"] and the stream adds the characters 'd', 'c', 'a', and 't' in sequence, the algorithm +should detect that the suffix "cat" of the stream "dcat" matches the word "cat" from the list.datastructures/trees/binary/tree/binary_tree.py-420-432 (1)
420-432: Unreachable code: duplicate condition check.Lines 427-428 have the same condition as lines 424-425. Since line 425 returns, lines 427-428 are never executed. This appears to be a copy-paste error—perhaps the intent was to check for nodes with exactly one child?
def is_perfect_helper(root: BinaryTreeNode, level: int = 0) -> bool: if root is None: return True if root.left is None and root.right is None: return depth == level + 1 - if root.left is None and root.right is None: + if root.left is None or root.right is None: return False return is_perfect_helper(root.left, level + 1) and is_perfect_helper( root.right, level + 1 )datastructures/linked_lists/singly_linked_list/test_singly_linked_revese.py-1-6 (1)
1-6: Fix filename typo: "revese" → "reverse".The filename
test_singly_linked_revese.pycontains a typo. Rename totest_singly_linked_reverse.pyfor consistency and discoverability.algorithms/sliding_window/repeated_dna_sequences/README.md-84-88 (1)
84-88: Fix small documentation typos and align snippet type hints with their return values.A few minor doc issues are worth adjusting:
- Around line 86, the mapping text ends with
3 → 'T(missing the closing quote). Updating to3 → 'T'will match the actual mapping dictionary.- In the step‑by‑step snippets (
find_repeated_dna_sequencesin the “Step 1” and “Step 2” sections), the function is annotated as returningList[str]but returns an encoded list of ints or a single hash value. To avoid confusion, either:
- Change the return type hints to match what those pedagogical snippets actually return, or
- Rename them to helper names (e.g.,
encode_dna_sequence,compute_initial_hash) and keep accurate annotations there.- At line 240, “10-letter long sequences” should be “10-letter-long sequences” (or simply “10-letter sequences”) to fix the hyphenation noted by the linter.
These tweaks will make the README consistent with the implementation and type hints.
Also applies to: 90-103, 143-165, 240-240
🧹 Nitpick comments (48)
puzzles/poker/__init__.py (1)
27-59: Consider simplifying the deeply nested ternary for readability.The reformatting makes the structure visible, but an 8-level nested ternary is still hard to follow. A lookup-based approach would be clearer:
def hand_rank(hand): card_ranks = ["..23456789TJQKA".index(r) for r, s in hand] groups = [(card_ranks.count(i), i) for i in set(card_ranks)] groups.sort(reverse=True) counts, ranks = zip(*groups) if ranks == [14, 5, 4, 3, 2]: ranks = [5, 4, 3, 2, 1] straight = (len(counts) == 5) and (max(ranks) - min(ranks) == 4) flush = len(set([s for r, s in hand])) == 1 count_ranks = { (5,): 9, (4, 1): 7, (3, 2): 6, (3, 1, 1): 3, (2, 2, 1): 2, (2, 1, 1, 1): 1, } if straight and flush: score = 8 elif counts in count_ranks: score = count_ranks[counts] elif flush: score = 5 elif straight: score = 4 else: score = 0 return score, ranksThis separates the pattern-matching logic from the special cases (straight/flush combinations).
datastructures/sets/union_find/__init__.py (1)
1-45: Solid DSU implementation with union-by-rank and path compression.The implementation is correct. One minor consideration:
findanduniondon't validate that indices are within bounds. An out-of-range index will raiseIndexError, which is acceptable but could be confusing. Consider adding explicit bounds checking with a descriptive error message if this is intended for educational use.def find(self, i: int) -> int: """Finds the representative (root) of the set containing element 'i'.""" + if not 0 <= i < len(self.root): + raise IndexError(f"Element {i} is out of bounds for size {len(self.root)}") if self.root[i] == i: return ialgorithms/arrays/two_sum/__init__.py (1)
30-30: LGTM! Consider documenting the empty list return case.The explicit
return []improves type safety by ensuring the function always returnsList[int]as declared. However, the docstring (lines 19-20) doesn't mention that an empty list is returned when no pair sums to the target.Consider updating the docstring:
Returns: - List of two numbers that sum up to the target + List of indices of two numbers that sum up to the target, or empty list if no such pair existsdatascience/numeric_python/statistics/TestChecker.py (2)
1-1: Use proper docstring format.The module-level documentation should use triple-quoted strings to be recognized as a docstring.
-"Class to test truths of functions" +"""Class to test truths of functions"""
5-6: Unused instance variableself.n.The
__init__method storesnbut it's never used. Consider removing it or documenting its purpose if it's intended for future use.algorithms/graphs/frog_position_after_t_seconds/README.md (1)
61-61: Use "cannot" instead of "can not".Per standard style conventions, "cannot" is the preferred form unless emphasizing "not".
Apply this diff:
- If time_left == 0, return p because the frog arrived earlier and can not move, so it remains on the target. + If time_left == 0, return p because the frog arrived earlier and cannot move, so it remains on the target.algorithms/graphs/frog_position_after_t_seconds/__init__.py (2)
18-22: Remove unusedparentparameter from queue tuples.The
parentvariable is stored in the queue state but never used. Thevisitedset already prevents revisiting nodes, making parent tracking redundant.Apply this diff:
- queue = deque([(1, -1, 1.0, 0)]) + queue = deque([(1, 1.0, 0)]) visited = {1} while queue: - vertex, parent, prob, time = queue.popleft() + vertex, prob, time = queue.popleft()And update line 53:
for neighbor in unvisited_neighbors: visited.add(neighbor) - queue.append((neighbor, vertex, next_prob, time + 1)) + queue.append((neighbor, next_prob, time + 1))
76-91: Avoid iterating over neighbors twice.The code iterates over
graph[u]twice: once to count unvisited neighbors (lines 77-79), then again to enqueue them (lines 88-91). Collect unvisited neighbors in a single pass for better efficiency.Apply this diff:
- cnt_unvisited = 0 + unvisited = [] for v in graph[u]: if not visited[v]: - cnt_unvisited += 1 + unvisited.append(v) if u == target: - if time_left == 0 or cnt_unvisited == 0: + if time_left == 0 or len(unvisited) == 0: return p return 0.0 - if cnt_unvisited > 0: - split = p / cnt_unvisited - for v in graph[u]: - if not visited[v]: - visited[v] = True - q.append((v, split)) + if unvisited: + split = p / len(unvisited) + for v in unvisited: + visited[v] = True + q.append((v, split))datastructures/trees/trie/__init__.py (1)
3-4: Consider importing from the suffix subpackage instead of individual modules.The
suffix/__init__.pyalready exportsSuffixTreeNodeandSuffixTree. Importing from the subpackage directly would be more consistent and reduce coupling to internal module structure.from datastructures.trees.trie.trie_node import TrieNode from datastructures.trees.trie.trie import Trie -from datastructures.trees.trie.suffix.suffix_tree_node import SuffixTreeNode -from datastructures.trees.trie.suffix.suffix_tree import SuffixTree +from datastructures.trees.trie.suffix import SuffixTreeNode, SuffixTreedatastructures/trees/trie/trie_node.py (3)
6-8: Remove commented-out code.The commented line
# self.char = charappears to be dead code from an earlier implementation. Remove it to improve readability.def __init__(self): - # self.char = char """ Initializes a TrieNode instance.
26-41: Add return type hint for consistency.The
insertmethod is missing a return type hint, whilesearch_prefixhas one. Add-> Nonefor consistency with the PR checklist requirement that all function parameters and return values are annotated with type hints.- def insert(self, word: str, index: int): + def insert(self, word: str, index: int) -> None:
38-41: Type inconsistency withfloat("inf")comparison.The expression
curr.index or float("inf")mixesOptional[int]withfloat, which works at runtime but creates a subtle type inconsistency. TheTrie.insertmethod intrie.pyuses the same pattern, so this is consistent with the codebase, but consider usingsys.maxsize(as done intypes.py) for type consistency.+import sys + # In the insert method: - curr.index = min(curr.index or float("inf"), index) + curr.index = min(curr.index if curr.index is not None else sys.maxsize, index)datastructures/trees/trie/suffix/suffix_tree.py (1)
14-16: Unnecessarysuper().__init__()call.
SuffixTreedoesn't inherit from any explicit parent class, so callingsuper().__init__()is redundant. This may be a leftover from refactoring.Apply this diff to remove the unnecessary call:
def __init__(self): - super().__init__() self.root = SuffixTreeNode()datastructures/arrays/matrix/settozero/__init__.py (1)
4-55: Algorithm looks correct; consider tightening the docstring/type info.The in-place marker-based implementation is clean and correct for rectangular matrices (including zeros in the first row/column and empty-matrix cases). Nothing blocking here.
Two small polish points you may want to address:
- Clarify the behavior and mutability in the docstring (it currently sounds a bit global and doesn't explicitly say "in place").
- Align the docstring type with the annotation.
Example docstring tweak:
-def set_matrix_zeros(mat: List[List[int]]) -> List[List[int]]: - """ - Sets all elements in a matrix to zero if any row or column has at least one zero element. +def set_matrix_zeros(mat: List[List[int]]) -> List[List[int]]: + """ + For every row or column that contains at least one zero, sets all elements + in that row or column to zero, modifying the matrix in place. @@ - Args: - mat (List): input matrix + Args: + mat (List[List[int]]): input matrixdatastructures/arrays/matrix/settozero/test_set_matrix_zero.py (1)
1-50: Good coverage; add edge cases and consider import/run-mode consistency.Tests exercise several meaningful patterns and look correct. A few non-blocking improvements:
Edge cases that would strengthen regression coverage
- A matrix with a zero only in the first column (and none in the first row) to fully exercise
first_col_has_zero.- An empty matrix (
[]) and a matrix with an empty first row ([[]]) to lock in the early-return behavior.- Optionally, 1×N / N×1 matrices.
For example:
class SetMatrixZeroTestCase(unittest.TestCase): @@ def test_5(self): matrix = [[3, 5, 2, 0], [1, 0, 4, 6], [7, 3, 2, 4]] expected = [[0, 0, 0, 0], [0, 0, 0, 0], [7, 0, 2, 0]] actual = set_matrix_zeros(matrix) self.assertEqual(expected, actual) + + def test_zero_in_first_column_only(self): + matrix = [[1, 2, 3], [0, 5, 6], [7, 8, 9]] + expected = [[0, 2, 3], [0, 0, 0], [0, 8, 9]] + actual = set_matrix_zeros(matrix) + self.assertEqual(expected, actual) + + def test_empty_and_empty_row(self): + self.assertEqual([], set_matrix_zeros([])) + self.assertEqual([[]], set_matrix_zeros([[]]))
- Asserting in-place semantics (if that’s part of the contract)
If you want to guarantee thatset_matrix_zerosmutates the input rather than returning a new matrix, you could add:matrix = [[1, 0], [3, 4]] actual = set_matrix_zeros(matrix) self.assertIs(matrix, actual)
- Relative import vs direct execution
from . import set_matrix_zerosassumes the test is run as part of the package (e.g.,python -m unittest ...). Running this file directly (python test_set_matrix_zero.py) will typically fail with an attempted relative import error.If you expect direct execution to work, consider switching to an absolute import appropriate for your project layout or dropping the
__main__block and relying solely on your test runner.To confirm your desired behavior, run tests the way your CI does (e.g.,
python -m unittest datastructures.arrays.matrix.settozero.test_set_matrix_zero) and decide whether directpython path/to/test_set_matrix_zero.pyshould also be supported.datastructures/arrays/subarrays_with_fixed_bounds/README.md (1)
3-16: Clarify bounds relationship and align naming with implementationThe README is clear, but you might want to (a) mention explicitly that
minK ≤ maxK(the implementation treatsmin_k > max_kas yielding 0), and (b) optionally align naming (minK/maxKvsmin_k/max_k) with the code for easier cross‑referencing.datastructures/arrays/subarrays_with_fixed_bounds/__init__.py (1)
5-28: Minor docstring polish for clarityA couple of small cleanups you could consider:
- Grammar: “subarrays … which satisfy the following conditions” instead of “which satisfies”.
- Possibly add a short note that any element
< min_kor> max_kmakes a subarray invalid for counting, matching the implementation logic.- Optionally align parameter names (
min_k,max_k) in the text with the function signature.Purely cosmetic, feel free to skip if you’re satisfied with the current wording.
datastructures/arrays/subarrays_with_fixed_bounds/test_subarrays_with_fixed_bounds.py (1)
5-56: Good coverage; consider a couple of extra edge‑case testsThe tests nicely exercise the main scenarios, including the LeetCode‑style example with an out‑of‑range element and the
min_k == max_kcases. As a small enhancement, you could add tests for:
nums = [](should return 0 per implementation)min_k > max_k(also returns 0)That would lock in the guard branch at Lines 29–31 of the implementation.
algorithms/arrays/two_sum_less_k/__init__.py (1)
18-18: In-place sorting mutates the input array.The
nums.sort()call modifies the caller's array, which is a side effect that may be unexpected. Consider usingnums = sorted(nums)to avoid mutating the input, or clearly document this behavior in the docstring.Apply this diff to avoid mutation:
- # sort the numbers in ascending order to facilitate binary search - nums.sort() + # sort the numbers in ascending order to facilitate binary search + nums = sorted(nums)Alternatively, document the side effect:
""" Finds the maximum sum of two elements in a given list of numbers that is less than k. Uses binary search to achieve a time complexity of O(n log n) and find the maximum sum of two elements that is less than k. It takes the nums array and the target value k as input. + + Note: This function sorts the input list in-place. + Args: - nums (List[int]): A sorted list of integers + nums (List[int]): A list of integers (will be sorted in-place) k int: The target value to search for Returns: The maximum sum of two elements that is less than k """algorithms/arrays/two_sum_less_k/test_two_sum.py (1)
1-48: Good test coverage for common scenarios.The test suite covers multiple important cases including valid pairs, no valid pairs, repeated elements, and the boundary case where the sum equals k. The test expectations are all correct based on manual verification of the algorithm.
Consider adding edge case tests for improved robustness:
def test_empty_array(self): """numbers = [], k = 10""" numbers = [] k = 10 expected = -1 actual = two_sum_less_than_k(numbers, k) self.assertEqual(expected, actual) def test_single_element(self): """numbers = [5], k = 10""" numbers = [5] k = 10 expected = -1 actual = two_sum_less_than_k(numbers, k) self.assertEqual(expected, actual) def test_negative_numbers(self): """numbers = [-3, -1, 2, 4], k = 1""" numbers = [-3, -1, 2, 4] k = 1 expected = -2 # -3 + 1 = -2 actual = two_sum_less_than_k(numbers, k) self.assertEqual(expected, actual)algorithms/search/binary_search/maxruntime_n_computers/__init__.py (1)
4-72: Binary search implementation looks correct; consider optional early-exit in feasibility checkThe overall approach in
max_runtime,can_run_for, andmax_run_time_2is sound and matches the documented binary-search strategy (bounds, monotonicity, and feasibility condition all look correct).If you expect very large
batterieslists, you could micro-optimizecan_run_forby short‑circuiting once enough power is accumulated, e.g.:def can_run_for(batteries: List[int], n: int, target_time: int) -> bool: power_needed = n * target_time power_available = 0 for battery in batteries: power_available += min(battery, target_time) if power_available >= power_needed: return True return FalseThis avoids traversing the full list when the condition is met early. Totally optional; the current version is already clear and correct.
algorithms/fast_and_slow/happy_number/test_happy_number.py (1)
116-121: Good additional negative-case coverage for the fast/slow implementation
test_10correctly asserts thatis_happy_number_2(20)isFalse(20 falls into a non‑1 cycle), which adds a useful extra unhappy-path check for the new implementation. No issues from a correctness standpoint.If you want to reduce duplication between the two test classes, you could consider parameterizing the cases and running them against both
is_happy_numberandis_happy_number_2, but that’s optional.algorithms/fast_and_slow/happy_number/__init__.py (1)
38-102: Fast/slow happy-number implementation looks correct; consider minor reuse & input-contract tweaksThe
is_happy_number_2implementation correctly applies Floyd’s cycle detection: the digit-sum helper is correct for non-negative integers, the fast/slow update loop is standard, and the final check onfast_pointer == 1yields the right result for happy vs. unhappy numbers. No correctness issues spotted for the documented “positive integer” domain.Two optional improvements you might consider:
- Factor out
sum_of_squared_digits/get_nextinto a shared top-level helper so bothis_happy_numbervariants reuse the same logic instead of duplicating the digit-sum loop.- Either:
- enforce the documented precondition (e.g., raise
ValueErrorforn <= 0), or- explicitly document that non-positive inputs are treated as “not happy” and return
Falsewithout raising, to make the behavior clear.puzzles/heap/min_cost_to_connect_sticks/test_min_cost_connect_sticks.py (1)
1-118: Well-structured test suite with good coverage.The test file properly validates both implementations with identical test cases, ensuring consistency. The edge case for a single stick (returning 0) is covered.
Consider adding a test case for an empty list to ensure both implementations handle this edge case gracefully:
def test_empty_list(self): sticks = [] expected = 0 actual = connect_sticks(sticks) self.assertEqual(expected, actual)DIRECTORY.md (1)
26-27: Inconsistent list indentation throughout the file.The added entries use 4-space and 6-space indentation, but MD007 expects 2-space increments. This affects multiple sections beyond the Edge entries already flagged. Consider applying consistent 2-space indentation across all changed sections for MD007 compliance.
Also applies to: 80-84
datastructures/trees/binary/tree/README.md (1)
377-393: Consider adding solution approach for consistency.The other problem sections in this README include detailed solution approaches with algorithm descriptions and complexity analysis. This section only describes the problem. Consider adding the solution approach (greedy DFS from leaves) for consistency.
Also, the constraint
Node.data == 0on line 389 appears to be copied from a LeetCode problem context where the node values are irrelevant (all zeros). If this is intentional, a brief clarification would help readers understand it's a placeholder value, not a meaningful constraint.puzzles/heap/maximal_score_after_k_operations/README.md (1)
3-6: Redundant problem statement introduction.Lines 3-6 contain two overlapping introductions: "You are given an array of integers nums..." and "You are given a 0-indexed array of integer nums...". Consider consolidating into a single, clear introduction.
-You are given an array of integers nums and an integer k. You want to perform the following operation exactly k times: - -You are given a 0-indexed array of integer nums and an integer k. Your task is to maximize a score through a series of -operations. Initially, your score is set to 0 +You are given a 0-indexed integer array `nums` and an integer `k`. Your task is to maximize a score through a series of +operations. Initially, your score is set to 0.datastructures/trees/binary/tree/test_binary_tree_min_camera_cover.py (1)
1-60: LGTM! Test cases provide good coverage for min_camera_cover.The tests cover important scenarios: single node (root must have camera), small balanced and imbalanced trees, and deeper structures. The expected values align with the greedy algorithm's behavior.
Consider adding descriptive test method names (e.g.,
test_single_node_requires_one_camera) for better readability.datastructures/streams/stream_checker/__init__.py (1)
26-39: Unnecessary reversal of words list in__build_trie.Line 28 iterates
self.words[::-1], which reverses the order of words. However, only reversing each word's characters (line 35) is needed for the reversed trie approach. The order in which words are inserted doesn't affect correctness, but reversing the words list is confusing and adds unnecessary overhead.def __build_trie(self): # insert the words in reverse order into the trie - for word in self.words[::-1]: + for word in self.words: # 1. track max length for deque optimization if len(word) > self.max_len: self.max_len = len(word)datastructures/trees/binary/tree/binary_tree.py (3)
626-645: Instance attribute pollution for tracking path length.
self.path_lengthis set as an instance attribute (line 630) to track state during recursion. This leaves stale state on the instance after the method completes. Consider usingnonlocalwith a local variable or a mutable container like thelongest_zig_zag_stackapproach.def longest_zig_zag(self) -> int: if self.root is None or (self.root.left is None and self.root.right is None): return 0 - self.path_length = 0 + path_length = [0] def dfs(node: Optional[BinaryTreeNode], go_left: bool, steps: int): if node is not None: - self.path_length = max(self.path_length, steps) + path_length[0] = max(path_length[0], steps) if go_left: dfs(node.left, False, steps + 1) dfs(node.right, True, 1) else: dfs(node.left, False, 1) dfs(node.right, True, steps + 1) dfs(self.root, False, 0) dfs(self.root, True, 0) - return self.path_length + return path_length[0]
843-856:deserializereturnsBinaryTreeNode, notBinaryTree.The method returns a
BinaryTreeNoderather than aBinaryTreeinstance, which is inconsistent withserialize()being an instance method. Consider returningBinaryTree(dfs(...))or document this asymmetry.@staticmethod - def deserialize(tree_str: str) -> Optional[BinaryTreeNode]: + def deserialize(tree_str: str) -> "BinaryTree": def dfs(nodes: Iterator[str]) -> Optional[BinaryTreeNode]: data = next(nodes) if data == "x": - return + return None current = BinaryTreeNode(data) current.left = dfs(nodes) current.right = dfs(nodes) return current - return dfs(iter(tree_str.split())) + return BinaryTree(dfs(iter(tree_str.split())))
16-17: Stub methods should raiseNotImplementedError.Methods
next,has_next,insert_node,paths, andincreasing_order_traversalare stubs that returnNoneimplicitly. Consider raisingNotImplementedErrorto make incomplete status explicit and prevent silent failures.def next(self) -> int: - pass + raise NotImplementedError("next() is not implemented") def has_next(self) -> bool: - pass + raise NotImplementedError("has_next() is not implemented") def increasing_order_traversal(self) -> TreeNode: - pass + raise NotImplementedError("increasing_order_traversal() is not implemented") def insert_node(self, value) -> TreeNode: - pass + raise NotImplementedError("insert_node() is not implemented") def paths(self) -> list: - pass + raise NotImplementedError("paths() is not implemented")Also applies to: 64-68, 78-82
datastructures/trees/node.py (2)
15-33: Clarifyparentsemantics and tighten its type hint
- Accepting and storing
parentdirectly is fine, but this changes the invariants of the node: callers now need to keepparentand anychildrencontainer (if present elsewhere) in sync manually. If the class (or subclasses) expose achildrenlist, consider documenting that responsibility clearly or updating the parent’s children when a parent is passed in, to avoid subtle inconsistencies.- For better typing of the generic, you may want to narrow the annotation to the parameterized type:
- def __init__( - self, value: T, key: Optional[Any] = None, parent: Optional["TreeNode"] = None - ): + def __init__( + self, + value: T, + key: Optional[Any] = None, + parent: Optional["TreeNode[T]"] = None, + ):
37-56: Review__eq__/__hash__behavior and align with typing & Python data modelThis block introduces several behavioral and typing changes worth double‑checking:
Behavioral change from identity to value‑based equality
Previously,
TreeNodeused object identity for==. Now two different nodes with the same.datacompare equal, regardless ofkeyorparent. That can be exactly what you want, but it may surprise existing tree algorithms that distinguish nodes by position or key. Please confirm this is intentional across current usages.Prefer
other: objectandNotImplementedfor unsupported typesThe canonical
__eq__signature and behavior are:
- def eq(self, other: "TreeNode[T]") -> bool:
...if not isinstance(other, TreeNode):return False...if other.data == self.data:return Truereturn False
- def eq(self, other: object) -> bool:
"""Checks if this node is equal to another node based on the data it contains."""if not isinstance(other, TreeNode):return NotImplementedreturn self.data == other.dataThis plays better with type checkers and with Python’s symmetric equality handling.
Ensure type‑hint completeness and be explicit about hash semantics
- To satisfy your “all functions annotated” goal, add a return type to
__hash__:
- def hash(self):
return hash(self.data)
- def hash(self) -> int:
return hash(self.data)- Because `__hash__` delegates to `hash(self.data)`, any node whose `value` is not hashable (e.g. a list) becomes unhashable even if a custom `key` is provided. If your intended contract is “nodes are hashable only when their values are hashable”, this is fine but should be assumed/documented. If you instead want to support unhashable `value` with a custom `key`, the hash/equality design may need to be revisited (e.g. basing both on `key`).datastructures/linked_lists/singly_linked_list/test_singly_linked_revese.py (2)
29-40: Remove duplicate test case.Lines 29-34 and 35-40 contain identical test data. Remove one to avoid redundant test execution.
( [103, 7, 10, -9, 105, 67, 31, 63], 1, 8, [63, 31, 67, 105, -9, 10, 7, 103], ), - ( - [103, 7, 10, -9, 105, 67, 31, 63], - 1, - 8, - [63, 31, 67, 105, -9, 10, 7, 103], - ),
78-89: Remove duplicate test case (same issue as above).Lines 78-83 and 84-89 are identical duplicates.
datastructures/linked_lists/singly_linked_list/single_linked_list.py (2)
14-18: Consider removing redundant class-levelheadattribute.The class attribute
head: Optional[SingleNode] = Noneis shadowed by the instance attribute set in the parent's__init__. While not a bug (sincesuper().__init__()assigns toself.head), the class-level declaration is unnecessary and could be misleading about shared state.class SinglyLinkedList(LinkedList): """ Implementation of a SinglyLinked List """ - head: Optional[SingleNode] = None - def __init__(self): # noinspection PyCompatibility super().__init__()
92-94: Stub method:insert_before_nodeis not implemented.This overrides an abstract method from
LinkedListbut provides no implementation. Either implement the method or document why it's intentionally left as a stub.Would you like me to generate an implementation for
insert_before_node?algorithms/sliding_window/repeated_dna_sequences/test_repeated_dna_sequences.py (1)
5-41: Consider adding tests for invalid characters and boundary lengths.The current suites validate core behavior well for both implementations. To lock in edge‑case semantics, consider adding:
- A case where
dna_sequencecontains an invalid character (e.g.,"ACGTXACGTX") asserting thatfind_repeated_dna_sequencesraisesValueError.- Boundary cases for both functions:
len(dna_sequence) < 10(e.g.,"ACGTACGTA") returning[].len(dna_sequence) == 10returning[]as there cannot be any repeats.This will better document and protect the public API behavior if the internals change later.
Also applies to: 43-79
algorithms/sliding_window/repeated_dna_sequences/__init__.py (2)
4-30: Naive implementation is correct; loop bound could be tightened if desired.The naive solution correctly identifies repeated 10‑length substrings using
seenandresult_set, with an early return forlen(dna_sequence) <= 10. If you want to make the intent a bit clearer and avoid generating short trailing substrings, you could iterate only up tolen(dna_sequence) - 9and drop thelen(subsequence) < 10guard, but that’s purely cosmetic here.
33-91: Rolling-hash implementation looks solid; consider consistent invalid-input handling across both APIs.The rolling‑hash version is well‑structured:
- Base‑4 encoding and initial hash computation are correct.
- Early return for short inputs avoids unnecessary work.
- Sliding window update
(old * 4) - left * 4^10 + rightwith precomputeda_kis consistent with the polynomial hash.- Result accumulation via
outputset ensures uniqueness, matching the naive behavior.One behavioral difference is that this function raises
ValueErroron invalid characters whilefind_repeated_dna_sequences_naivesilently accepts any input. Since the problem constraints already restrict inputs toA/C/G/T, this isn’t a correctness bug, but you may want to either:
- Mirror the same validation in the naive implementation, or
- Document that only the optimized version enforces input validation.
algorithms/graphs/course_schedule/README.md (2)
54-58: Clarify and split the combined constraints bulletThe last constraints bullet currently concatenates several conditions into one sentence, which is hard to read and easy to misparse.
Consider splitting it into separate bullets, e.g.:
-- prerequisites[i].length == 2 0 <= ai, bi < numCourses ai != bi All the pairs [ai, bi] are distinct. +- prerequisites[i].length == 2 +- 0 <= ai, bi < numCourses +- ai != bi +- All the pairs [ai, bi] are distinct.This keeps the spec aligned with the standard problem statement and makes it much clearer for readers.
62-107: Tighten terminology and complexity wording in solution breakdownsA few minor doc nits that would improve precision:
- Use the standard hyphenated term “Depth-First Search” in the “Solution 1 Breakdown” heading.
- When describing recursion space, it’s more accurate to say the call stack uses O(V) space (proportional to the longest path in vertices) rather than O(E).
- In the Solution 2 space-complexity section, “storing all of the edges” can be shortened to “storing all the edges” per the style hint.
These are small wording tweaks but make the explanations more precise and polished. Based on static analysis hints.
Also applies to: 139-147
algorithms/graphs/course_schedule/__init__.py (2)
9-73: DFS-based topological sort looks good; align docstring and input handlingThe DFS + color-marking implementation for
find_orderis standard and matches the expected behavior for the course-schedule problem.Two small points:
- The docstring currently says “a list of tuples … source and destination of the prerequisite”, while the type hint is
List[List[int]]and the code treats each pair as[course, prerequisite](for destination, source in prerequisites). It would be clearer to spell this out explicitly, e.g. “list of [course, prerequisite] pairs where[a, b]means you must takebbeforea.”- Right now you rely on the problem constraints that
0 <= course_id < num_courses. If any prerequisite has a destination outside that range but a source inside it (e.g.[4, 1]withnum_courses = 3),color[neighbour]will raise aKeyError. If you want the function to be robust to such inputs (or to match the behavior you’re testing with out-of-range IDs), consider filtering when building the adjacency list:- for destination, source in prerequisites: - adjacency_list[source].append(destination) + for destination, source in prerequisites: + if 0 <= source < num_courses and 0 <= destination < num_courses: + adjacency_list[source].append(destination)Alternatively, document the index-range precondition more explicitly so callers know invalid course IDs are undefined behavior.
76-124: Kahn’s algorithm implementation is solid; clarify prerequisites shape and optional robustnessThe
can_finishimplementation cleanly follows Kahn’s algorithm (in-degree tracking + BFS) and the early return fornum_courses <= 0is straightforward.A few minor improvements:
- As with
find_order, the docstring describes “a list of tuples … source and destination”, but the actual structure isList[List[int]]with pairs interpreted as[course, prerequisite]. Making that explicit will reduce confusion.- Your tests include a case like
num_courses = 3, prerequisites = [[1, 0], [2, 1], [4, 3]], where some course IDs are outside[0, num_courses). The current implementation effectively ignores those extra nodes when deciding feasibility, which matches the test, but the behavior for other out-of-range patterns (e.g.[4, 1]) is less obvious. If you want consistent semantics, you could either:
Filter to only edges fully within range:
- for child, parent in prerequisites:
if parent in graph:graph[parent].append(child)else:graph[parent] = [child]if child in in_degree:in_degree[child] += 1else:in_degree[child] = 1
- for child, parent in prerequisites:
if 0 <= parent < num_courses and 0 <= child < num_courses:graph[parent].append(child)in_degree[child] += 1- Or explicitly document that course IDs are expected to be in `[0, num_courses)` and that anything else is outside the function’s contract.Either approach would make the behavior clearer to future readers.
algorithms/graphs/course_schedule/test_course_schedule.py (3)
13-18: Relax find_order expectations where multiple topological orders are validIn
test_2, the graph admits more than one valid topological order (e.g.[0, 1, 2, 3]and[0, 2, 1, 3]). Asserting a single fixed sequence makes the test brittle to benign implementation changes (e.g. different neighbor iteration order).Consider asserting that the actual order is one of a small set of valid orders or, more generally, that it respects all prerequisite constraints (every edge
u -> vhasubeforev), rather than enforcing one specific permutation.
41-46: Clarify intent of test_6’s comment with out-of-range course IDs
test_6is a nice addition to check behavior whenprerequisitesmentions courses beyondnum_courses. However, the inline comment:expected = [0, 1, 2] # or [0,1,2,3,4]is misleading: with
num_courses = 3, the current API and implementation will never return a list longer thannum_courses.If the intent is to assert that extra prerequisites for out-of-range courses are ignored, you might update the comment to say that explicitly and drop the
[0,1,2,3,4]alternative.
49-85: Add a can_finish test for num_courses <= 0
can_finishhas an explicit early return fornum_courses <= 0, but there’s no test covering that path.Consider adding a simple case like:
self.assertTrue(can_finish(num_courses=0, prerequisites=[]))to lock in the intended behavior.
| - uses: actions/checkout@v5 | ||
| - uses: actions/cache@v4 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/pre-commit.yml | head -30Repository: BrianLusina/PythonSnips
Length of output: 852
🌐 Web query:
actions/setup-python latest version GitHub Actions
💡 Result:
The latest release is actions/setup-python v6.1.0 — published Nov 25, 2025. [1][2]
Sources:
[1] actions/setup-python Releases (GitHub).
[2] actions/setup-python README / Marketplace page (GitHub).
🌐 Web query:
psf/black latest version GitHub Actions
💡 Result:
Use the official action at psf/black@stable to track the latest released Black from PyPI; or pin to a specific release like psf/black@24.4.0 to lock the version. [1][2]
Sources:
- psf/black releases (example: 24.4.0). [1]
- Usage: psf/black@stable (official GitHub Action / docs/examples). [2]
Update remaining outdated actions for consistency.
The checkout@v5 and cache@v4 upgrades are good, but also update:
actions/setup-python@v2→ current latest is v6.1.0psf/black@21.4b0→ usepsf/black@stableor pin to a recent version likepsf/black@24.4.0
🤖 Prompt for AI Agents
.github/workflows/pre-commit.yml around lines 9 to 10: the workflow updates
checkout and cache actions but leaves older pins for setup-python and psf/black;
change uses: actions/setup-python@v2 to actions/setup-python@v6 or
actions/setup-python@v6.1.0 (or the latest v6.x tag) and change uses:
psf/black@21.4b0 to either psf/black@stable or pin to a recent release such as
psf/black@24.4.0 so all actions are consistent and up-to-date.
| def test_frog_position(self, n, edges, t, target, expected): | ||
| actual = frog_position_2(n, edges, t, target) | ||
| actual_rounded = round(actual, 5) | ||
| self.assertEqual(expected, actual_rounded) |
There was a problem hiding this comment.
Fix duplicate test method name.
Both test methods are named test_frog_position. The second definition (Line 40) overrides the first (Line 21), so frog_position is never tested—only frog_position_2 runs.
Apply this diff:
- def test_frog_position(self, n, edges, t, target, expected):
+ def test_frog_position_2(self, n, edges, t, target, expected):
actual = frog_position_2(n, edges, t, target)📝 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.
| def test_frog_position(self, n, edges, t, target, expected): | |
| actual = frog_position_2(n, edges, t, target) | |
| actual_rounded = round(actual, 5) | |
| self.assertEqual(expected, actual_rounded) | |
| def test_frog_position_2(self, n, edges, t, target, expected): | |
| actual = frog_position_2(n, edges, t, target) | |
| actual_rounded = round(actual, 5) | |
| self.assertEqual(expected, actual_rounded) |
🤖 Prompt for AI Agents
In
algorithms/graphs/frog_position_after_t_seconds/test_frog_position_after_t_seconds.py
around lines 40 to 43, the second test method is named test_frog_position which
overrides the earlier test and prevents frog_position from being run; rename
this second method to a unique name such as test_frog_position_2 (or similar) so
both tests run independently and keep the body calling frog_position_2 and the
existing assertions unchanged.
| def insert_after_node(self, prev_node: Any, data: Any): | ||
| if self.is_empty(): | ||
| raise EmptyLinkedList("LinkedList has no Nodes") | ||
| if not prev_node: | ||
| raise ValueError("Prev Node can not be None") | ||
| if not data: | ||
| raise ValueError("Data to insert can not be None") | ||
|
|
||
| prev_node = ( | ||
| prev_node if isinstance(prev_node, SingleNode) else SingleNode(prev_node) | ||
| ) | ||
| node_to_insert = SingleNode(data) | ||
|
|
||
| current = self.head | ||
|
|
||
| # traverse the linked list until we find the node to insert | ||
| while current.next: | ||
| if current == prev_node: | ||
| node_to_insert.next = prev_node.next | ||
| prev_node.next = node_to_insert | ||
| # we have inserted the node, now we can exit | ||
| break | ||
| current = current.next |
There was a problem hiding this comment.
Logic bug: insert_after_node will fail to find the target node.
When prev_node is not a SingleNode, line 77 creates a new SingleNode. The comparison current == prev_node at line 85 uses Node.__eq__ which compares keys. Since the newly created node isn't in the list and has a freshly computed key, it will never match existing nodes.
Additionally, line 73 rejects falsy values like 0 or "" which may be valid data.
def insert_after_node(self, prev_node: Any, data: Any):
if self.is_empty():
raise EmptyLinkedList("LinkedList has no Nodes")
if not prev_node:
raise ValueError("Prev Node can not be None")
- if not data:
+ if data is None:
raise ValueError("Data to insert can not be None")
- prev_node = (
- prev_node if isinstance(prev_node, SingleNode) else SingleNode(prev_node)
- )
node_to_insert = SingleNode(data)
current = self.head
# traverse the linked list until we find the node to insert
- while current.next:
- if current == prev_node:
+ while current:
+ # Compare by data value, not node identity
+ target_data = prev_node.data if isinstance(prev_node, SingleNode) else prev_node
+ if current.data == target_data:
node_to_insert.next = current.next
current.next = node_to_insert
- # we have inserted the node, now we can exit
- break
+ return
current = current.next🤖 Prompt for AI Agents
In datastructures/linked_lists/singly_linked_list/single_linked_list.py around
lines 68 to 90, the method creates a new SingleNode for prev_node and then
compares nodes by equality (which uses node keys), so it will never match an
existing node; also the checks use falsy checks that reject valid values like 0
or "". Fix by: accept prev_node as either an actual SingleNode instance or a
value to search for, do not construct a new node to represent prev_node, change
validations to use "is None" (e.g. if prev_node is None or if data is None) so
falsy-but-valid values are allowed, iterate the list using "current" (while
current) and compare either node identity (if prev_node is a SingleNode) or
node.data == prev_node (if prev_node is a value), insert when matched, and raise
a clear error if the target prev_node is not found.
| while current is not None: | ||
| for _ in range(position): | ||
| current = current.next | ||
|
|
||
| if current is None: | ||
| raise ValueError( | ||
| f"Invalid position {position} found, reached end of list" | ||
| ) | ||
|
|
||
| current.data = current.next.data if current.next else None | ||
| current.next = current.next.next if current.next else None | ||
|
|
||
| return self.head | ||
| return None |
There was a problem hiding this comment.
Edge-case bug: deleting the last node leaves a "zombie" node with None data.
When position points to the last node, current.next is None. The code sets current.data = None and current.next = None, but the node remains in the list with null data instead of being removed.
The mixed while/for loop structure is also confusing. Consider a cleaner traversal approach.
while current is not None:
- for _ in range(position):
+ for _ in range(position - 1):
current = current.next
if current is None:
raise ValueError(
f"Invalid position {position} found, reached end of list"
)
- current.data = current.next.data if current.next else None
- current.next = current.next.next if current.next else None
+ if current.next is None:
+ raise ValueError(f"Invalid position {position} found, reached end of list")
+ deleted = current.next
+ current.next = current.next.next
- return self.head
+ return deleted
return None🤖 Prompt for AI Agents
In datastructures/linked_lists/singly_linked_list/single_linked_list.py around
lines 134-147, the deletion logic sets the target node's data to None when
deleting the last node instead of removing the node, and the mixed while/for
traversal is confusing; change to a clearer traversal: handle position==0 by
updating self.head to head.next (or None), otherwise iterate with a single loop
to stop at the node before the target (keeping a prev and current pointer),
validate bounds and raise ValueError if position is out of range, then remove
the target by setting prev.next = target.next (so the last node is removed
rather than nulled), and return self.head. Ensure you do not set node.data =
None anywhere for deletion.
| def shift(self): | ||
| """ | ||
| Since this is a singly linked list, this will have to make the head's next to the position of head | ||
| :return: deleted node | ||
| """ | ||
| # store the head node | ||
| to_del = self.head | ||
| # replace the head with the next value in the LinkedList | ||
| self.head = self.head.next | ||
| return to_del |
There was a problem hiding this comment.
Potential AttributeError when shifting an empty list.
If self.head is None, line 289 (self.head.next) raises AttributeError. Add an empty-list guard.
def shift(self):
"""
Since this is a singly linked list, this will have to make the head's next to the position of head
:return: deleted node
"""
+ if not self.head:
+ return None
# store the head node
to_del = self.head
# replace the head with the next value in the LinkedList
self.head = self.head.next
return to_del📝 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.
| def shift(self): | |
| """ | |
| Since this is a singly linked list, this will have to make the head's next to the position of head | |
| :return: deleted node | |
| """ | |
| # store the head node | |
| to_del = self.head | |
| # replace the head with the next value in the LinkedList | |
| self.head = self.head.next | |
| return to_del | |
| def shift(self): | |
| """ | |
| Since this is a singly linked list, this will have to make the head's next to the position of head | |
| :return: deleted node | |
| """ | |
| if not self.head: | |
| return None | |
| # store the head node | |
| to_del = self.head | |
| # replace the head with the next value in the LinkedList | |
| self.head = self.head.next | |
| return to_del |
🤖 Prompt for AI Agents
In datastructures/linked_lists/singly_linked_list/single_linked_list.py around
lines 281 to 290, the shift method assumes self.head is non-None and will raise
AttributeError when called on an empty list; add an early guard that returns
None (or a clear sentinel) if self.head is None, otherwise proceed to store
to_del, set self.head to to_del.next, decrement/update list length, and if the
removed node was also the tail (i.e. list became empty) ensure self.tail is set
to None before returning the removed node.
| def find_closest_value_in_bst(self, target: T) -> Optional[BinaryTreeNode]: | ||
| """ | ||
| Finds the closest value in the binary search tree to the given target value. | ||
| Args: | ||
| target T: Value to search for | ||
| Returns: | ||
| Node with the closest value to the target | ||
| """ | ||
| # edge case for empty nodes, if none is provided, we can't find a value that is close to the target | ||
| if not self.root: | ||
| return None | ||
|
|
||
| # if the node's data is the target, exit early by returning it | ||
| if self.root.data == target: | ||
| return self.root | ||
|
|
||
| # this keeps track of the minimum on both the left and the right | ||
| closest_node = self.root | ||
| min_diff = abs(target - self.root.data) | ||
| current = self.root | ||
|
|
||
| # while the queue is not empty, we pop off nodes from the queue and check for their values | ||
| while current: | ||
| current_diff = abs(target - self.root.data) | ||
|
|
||
| if current_diff < min_diff: | ||
| min_diff = current_diff | ||
| closest_node = current | ||
|
|
||
| if current.data == target: | ||
| return current | ||
|
|
||
| if target < current.data: | ||
| current = current.left | ||
| else: | ||
| current = current.right | ||
|
|
||
| return closest_node |
There was a problem hiding this comment.
Fix critical bug in closest value calculation.
Line 327 incorrectly uses self.root.data instead of current.data when calculating the difference. This means the method always compares against the root node instead of the current node during traversal, producing incorrect results.
Apply this fix:
# while the queue is not empty, we pop off nodes from the queue and check for their values
while current:
- current_diff = abs(target - self.root.data)
+ current_diff = abs(target - current.data)
if current_diff < min_diff:
min_diff = current_diff
closest_node = current🤖 Prompt for AI Agents
In datastructures/trees/binary/search_tree/__init__.py around lines 303 to 341,
the closest-value loop mistakenly computes current_diff using self.root.data
instead of current.data; change the calculation to use current.data so each
visited node's value is compared to target, leaving the rest of the traversal
and updates (min_diff, closest_node, equality check and left/right moves)
unchanged.
| def is_complete_helper(root, idx: int) -> bool: | ||
| if root is None: | ||
| return True | ||
|
|
||
| if idx >= node_count: | ||
| return False | ||
|
|
||
| return is_complete_helper(root.left, 2 * idx + 1) and is_complete_helper( | ||
| root.right, 2 * index + 2 | ||
| ) | ||
|
|
||
| index = 0 | ||
| return is_complete_helper(self.root, index) |
There was a problem hiding this comment.
Critical bug: is_complete_helper uses wrong variable for right child index.
Line 401 uses index (the outer variable initialized to 0 at line 404) instead of idx (the function parameter). This causes incorrect index calculation for right children, breaking the completeness check.
def is_complete_helper(root, idx: int) -> bool:
if root is None:
return True
if idx >= node_count:
return False
return is_complete_helper(root.left, 2 * idx + 1) and is_complete_helper(
- root.right, 2 * index + 2
+ root.right, 2 * idx + 2
)
index = 0
return is_complete_helper(self.root, index)📝 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.
| def is_complete_helper(root, idx: int) -> bool: | |
| if root is None: | |
| return True | |
| if idx >= node_count: | |
| return False | |
| return is_complete_helper(root.left, 2 * idx + 1) and is_complete_helper( | |
| root.right, 2 * index + 2 | |
| ) | |
| index = 0 | |
| return is_complete_helper(self.root, index) | |
| def is_complete_helper(root, idx: int) -> bool: | |
| if root is None: | |
| return True | |
| if idx >= node_count: | |
| return False | |
| return is_complete_helper(root.left, 2 * idx + 1) and is_complete_helper( | |
| root.right, 2 * idx + 2 | |
| ) | |
| index = 0 | |
| return is_complete_helper(self.root, index) |
🤖 Prompt for AI Agents
In datastructures/trees/binary/tree/binary_tree.py around lines 393 to 405, the
helper uses the outer variable "index" instead of the parameter "idx" when
computing the right child index which breaks the completeness check; change the
right-child calculation to use idx (i.e., 2 * idx + 2) and keep calling
is_complete_helper(self.root, 0) (you can remove or keep the outer "index"
variable but ensure the helper consistently uses its idx parameter for both left
and right recursions).
| def is_height_balanced(node: BinaryTreeNode) -> bool: | ||
| left_height = 0 | ||
| right_height = 0 | ||
|
|
||
| if node is None: | ||
| return True | ||
|
|
||
| l = is_height_balanced(node.left) | ||
| r = is_height_balanced(node.right) | ||
|
|
||
| if abs(left_height - right_height) <= 1: | ||
| return l and r | ||
|
|
||
| return False | ||
|
|
||
| return is_height_balanced(self.root) |
There was a problem hiding this comment.
Critical bug: is_height_balanced never computes actual heights.
left_height and right_height remain 0—they're never updated from child heights. The condition abs(left_height - right_height) <= 1 always evaluates to abs(0 - 0) <= 1, which is always True. This method will incorrectly report unbalanced trees as balanced.
The helper should return height (or -1 for unbalanced) rather than a boolean:
def is_balanced(self) -> bool:
if self.root is None:
return True
- def is_height_balanced(node: BinaryTreeNode) -> bool:
- left_height = 0
- right_height = 0
-
+ def check_height(node: BinaryTreeNode) -> int:
+ """Returns height if balanced, -1 if unbalanced."""
if node is None:
- return True
-
- l = is_height_balanced(node.left)
- r = is_height_balanced(node.right)
+ return 0
- if abs(left_height - right_height) <= 1:
- return l and r
+ left_height = check_height(node.left)
+ if left_height == -1:
+ return -1
+
+ right_height = check_height(node.right)
+ if right_height == -1:
+ return -1
- return False
+ if abs(left_height - right_height) > 1:
+ return -1
- return is_height_balanced(self.root)
+ return 1 + max(left_height, right_height)
+
+ return check_height(self.root) != -1📝 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.
| def is_height_balanced(node: BinaryTreeNode) -> bool: | |
| left_height = 0 | |
| right_height = 0 | |
| if node is None: | |
| return True | |
| l = is_height_balanced(node.left) | |
| r = is_height_balanced(node.right) | |
| if abs(left_height - right_height) <= 1: | |
| return l and r | |
| return False | |
| return is_height_balanced(self.root) | |
| def check_height(node: BinaryTreeNode) -> int: | |
| """Returns height if balanced, -1 if unbalanced.""" | |
| if node is None: | |
| return 0 | |
| left_height = check_height(node.left) | |
| if left_height == -1: | |
| return -1 | |
| right_height = check_height(node.right) | |
| if right_height == -1: | |
| return -1 | |
| if abs(left_height - right_height) > 1: | |
| return -1 | |
| return 1 + max(left_height, right_height) | |
| return check_height(self.root) != -1 |
🤖 Prompt for AI Agents
In datastructures/trees/binary/tree/binary_tree.py around lines 444 to 459, the
is_height_balanced helper never computes or returns child heights (left_height
and right_height stay 0) so the balance check is wrong; change the helper to
return an integer height (or -1 to indicate unbalanced) instead of a boolean:
recursively call the helper to get left_height and right_height, if either is -1
return -1, if abs(left_height - right_height) > 1 return -1, otherwise return
max(left_height, right_height) + 1; then have the public
is_height_balanced(self) call the helper and treat a -1 result as unbalanced and
any non-negative result as balanced.
| if index is not None: | ||
| curr.index = min(curr.index or float("inf"), index) |
There was a problem hiding this comment.
Bug: Index 0 is treated as falsy, causing incorrect behavior.
When curr.index is 0, the expression curr.index or float("inf") evaluates to float("inf") because 0 is falsy in Python. This means index 0 would be incorrectly overwritten by any subsequent index.
Apply this diff to fix the issue:
- if index is not None:
- curr.index = min(curr.index or float("inf"), index)
+ if index is not None:
+ curr.index = min(curr.index if curr.index is not None else float("inf"), index)📝 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.
| if index is not None: | |
| curr.index = min(curr.index or float("inf"), index) | |
| if index is not None: | |
| curr.index = min(curr.index if curr.index is not None else float("inf"), index) |
🤖 Prompt for AI Agents
In datastructures/trees/trie/trie.py around lines 46-47, the code uses
"curr.index or float('inf')" which treats 0 as falsy and will incorrectly
replace an existing index 0; change the expression to use an explicit None check
(e.g. use curr.index if curr.index is not None else float('inf')) so min()
receives the correct existing index value and 0 is preserved.
| def search(self, word: str) -> List[str]: | ||
| if len(word) == 0: | ||
| return [] | ||
|
|
||
| curr = self.root | ||
|
|
||
| for char in word: | ||
| if char in curr.children: | ||
| curr = curr.children[char] | ||
| else: | ||
| return [] | ||
|
|
||
| output = [] | ||
|
|
||
| def dfs(node: TrieNode, prefix: str) -> None: | ||
| if node.is_end: | ||
| output.append((prefix + "".join(node.children.keys()))) | ||
|
|
||
| for child in node.children.values(): | ||
| dfs(child, prefix + "".join(node.children.keys())) | ||
|
|
||
| dfs(curr, word[:-1]) | ||
| return output |
There was a problem hiding this comment.
Critical bug in DFS logic: Incorrect word reconstruction.
The search method has multiple issues:
- Line 67:
"".join(node.children.keys())concatenates all children keys together instead of building the actual word path. - Line 70: The recursive call joins all children keys rather than passing the specific character for each child.
- Line 72:
word[:-1]drops the last character of the search prefix unnecessarily.
For a trie with words "cat", "car", the current logic would produce malformed results like "catr" instead of proper words.
Apply this diff to fix the DFS logic:
def search(self, word: str) -> List[str]:
if len(word) == 0:
return []
curr = self.root
for char in word:
if char in curr.children:
curr = curr.children[char]
else:
return []
output = []
- def dfs(node: TrieNode, prefix: str) -> None:
+ def dfs(node: TrieNode, current_word: str) -> None:
if node.is_end:
- output.append((prefix + "".join(node.children.keys())))
+ output.append(current_word)
- for child in node.children.values():
- dfs(child, prefix + "".join(node.children.keys()))
+ for char, child in node.children.items():
+ dfs(child, current_word + char)
- dfs(curr, word[:-1])
+ dfs(curr, word)
return output🤖 Prompt for AI Agents
In datastructures/trees/trie/trie.py around lines 51 to 73, the DFS builds words
incorrectly: replace the joins and wrong prefix handling by using the specific
child character when recursing and when appending completed words, and start the
DFS with the full search prefix (not word[:-1]); specifically, iterate children
as (char, child), on node.is_end append the current prefix, and recurse with
dfs(child, prefix + char), and invoke dfs(curr, word) after locating the prefix
node.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (11)
DIRECTORY.md (1)
214-226: Adjust Graphs > Edge / Undirected indentation to match other Datastructures entries and clear MD007.This block is still over‑indented relative to the rest of the “Datastructures” section, and markdownlint flags MD007 on these lines. Reusing the earlier suggestion and extending it to the new “Undirected” subsection, you can normalize as:
- * Graphs - * Edge - * [Edge](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge.py) - * [Edge Directed](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_directed.py) - * [Edge Hyper](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_hyper.py) - * [Edge Self](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_self.py) - * [Edge Type](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_type.py) - * [Edge Undirected](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_undirected.py) - * [Graph](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/graph.py) - * [Test Vertex](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/test_vertex.py) - * Undirected - * [Test Utils](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/undirected/test_utils.py) - * [Undirected Graph](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/undirected/undirected_graph.py) - * [Utils](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/undirected/utils.py) - * [Vertex](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/vertex.py) + * Graphs + * Edge + * [Edge](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge.py) + * [Edge Directed](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_directed.py) + * [Edge Hyper](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_hyper.py) + * [Edge Self](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_self.py) + * [Edge Type](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_type.py) + * [Edge Undirected](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/edge/edge_undirected.py) + * [Graph](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/graph.py) + * [Test Vertex](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/test_vertex.py) + * Undirected + * [Test Utils](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/undirected/test_utils.py) + * [Undirected Graph](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/undirected/undirected_graph.py) + * [Utils](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/undirected/utils.py) + * [Vertex](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/graphs/vertex.py)This brings these bullets to the same levels as other subsections (e.g., Arrays, Dicts) and resolves the MD007 indentation errors.
datastructures/graphs/edge/edge_hyper.py (1)
14-31: Fix HyperEdge identifier default and EdgeType to avoid subtle bugs.Two issues here:
identifier: AnyStr = uuid4()evaluatesuuid4()at import time, so allHyperEdgeinstances constructed without an explicit identifier share the same id.edge_type()returnsEdgeType.SELFinstead of the appropriate hyper edge enum, which breaks type-based dispatch.A minimal, consistent fix:
-from typing import AnyStr, Union, Dict, Optional, Generic, TypeVar, List, Any -from uuid import uuid4 +from typing import Union, Dict, Optional, Generic, TypeVar, List, Any +from uuid import uuid4, UUID @@ - def __init__( - self, - nodes: List[Any], - weight: Optional[Union[int, float]] = None, - properties: Optional[Dict[str, Any]] = None, - identifier: AnyStr = uuid4(), - ): - super().__init__(weight, properties, identifier) + def __init__( + self, + nodes: List[Any], + weight: Optional[Union[int, float]] = None, + properties: Optional[Dict[str, Any]] = None, + identifier: Optional[UUID] = None, + ): + super().__init__(weight, properties, identifier or uuid4()) @@ - def edge_type(self) -> EdgeType: - return EdgeType.SELF + def edge_type(self) -> EdgeType: + return EdgeType.HYPER_UNDIRECTED(Use
HYPER_DIRECTEDinstead if that better matches how you model hyper-edges elsewhere.)This aligns HyperEdge with the rest of the edge implementations and avoids the shared-id pitfall.
datastructures/graphs/edge/edge_directed.py (1)
15-34: Avoiduuid4()as a default argument inDirectedEdgeand align identifier typing.Like
HyperEdge, this constructor usesidentifier: AnyStr = uuid4(), which binds a single UUID at import time and reuses it for all edges without an explicit id.Recommend switching to late binding and a UUID-oriented type:
-from typing import AnyStr, Union, Dict, Optional, Generic, TypeVar, List, Any -from uuid import uuid4 +from typing import Union, Dict, Optional, Generic, TypeVar, List, Any +from uuid import uuid4, UUID @@ - def __init__( - self, - source: Any, - destination: Any, - weight: Optional[Union[int, float]] = None, - properties: Optional[Dict[str, Any]] = None, - identifier: AnyStr = uuid4(), - ): - super().__init__(weight, properties, identifier) + def __init__( + self, + source: Any, + destination: Any, + weight: Optional[Union[int, float]] = None, + properties: Optional[Dict[str, Any]] = None, + identifier: Optional[UUID] = None, + ): + super().__init__(weight, properties, identifier or uuid4())This keeps identifiers unique by default and consistent with the rest of the edge API.
datastructures/graphs/vertex.py (1)
13-31: Vertex id, hashability, edge API usage, and degree calculations need fixes to work with the new Edge model.There are several coupled issues here that will break graphs at runtime:
Default identifier uses
uuid4()in the signature.
identifier: Any = uuid4()binds a single UUID at import time; all vertices created without an explicit id share it.
Vertexis not hashable but is used as a dict key.
You define__eq__but not__hash__, so instances become unhashable.GraphusesVertexinself.adjacency_list[...], which will raiseTypeError.
self.edgesis computed once and can go stale.
It’s set toincoming_edges.union(outgoing_edges)in__init__, so later mutations of either set aren’t reflected.
adjacent_verticeskey typing is too narrow.
Keys areother.id(often a UUID), but the type isDict[str, "Vertex"].
degree,in_degree, andout_degreedon’t match the newEdgeAPI.
degreereturns 0 whenever either incoming or outgoing is empty, even if one side has edges.- It checks
if not edge.weight, which treatsweight=0as “unweighted” and ignoresEdge.is_unweighted().in_degree/out_degreeuseedge.type,edge.node_two, andedge.node_one, but the new edges exposeedge_type()andvertices()instead (andDirectedEdgeusessource/destination).A consolidated refactor to address these:
-from typing import Any, Set, Dict, Optional, Generic, TypeVar, List -from .edge import Edge, EdgeType +from typing import Any, Set, Dict, Optional, Generic, TypeVar, List +from .edge import Edge, EdgeType @@ def __init__( self, data: T, - identifier: Any = uuid4(), + identifier: Any = None, properties: Optional[Dict[str, Any]] = None, incoming_edges: Optional[Set[Edge]] = None, outgoing_edges: Optional[Set[Edge]] = None, ): if outgoing_edges is None: outgoing_edges = set() if incoming_edges is None: incoming_edges = set() - self.id = identifier + self.id = uuid4() if identifier is None else identifier self.data = data self.incoming_edges = incoming_edges self.outgoing_edges = outgoing_edges - self.edges = self.incoming_edges.union(self.outgoing_edges) - self.adjacent_vertices: Dict[str, "Vertex"] = {} + self.adjacent_vertices: Dict[Any, "Vertex"] = {} self.properties = properties @@ - def __eq__(self, other: "Vertex") -> bool: - return self.id == other.id + def __eq__(self, other: object) -> bool: + if not isinstance(other, Vertex): + return NotImplemented + return self.id == other.id + + def __hash__(self) -> int: + return hash(self.id) @@ @property def neighbours(self) -> List["Vertex"]: @@ - nodes = [] - for vertex in self.adjacent_vertices.values(): - nodes.append(vertex) - - return nodes + return list(self.adjacent_vertices.values()) @@ @property def degree(self) -> int: @@ - degrees = 0 - - if len(self.incoming_edges) == 0 or len(self.outgoing_edges) == 0: - return degrees - - seen_edges: Set = set() - - for edge in self.edges: - if edge not in seen_edges: - seen_edges.add(edge) - if not edge.weight: - degrees += 1 - - return degrees + degrees = 0 + if len(self.edges) == 0: + return degrees + + seen_edges: Set[Edge] = set() + for edge in self.edges: + if edge not in seen_edges: + seen_edges.add(edge) + if edge.is_unweighted(): + degrees += 1 + + return degrees @@ @property def in_degree(self) -> int: @@ - in_degrees = 0 - if len(self.edges) == 0: - return in_degrees - - for edge in self.edges: - if edge.type == EdgeType.DIRECTED and edge.node_two == self: - in_degrees += 1 - - return in_degrees + in_degrees = 0 + if len(self.edges) == 0: + return in_degrees + + for edge in self.edges: + if edge.edge_type() == EdgeType.DIRECTED: + vertices = edge.vertices() + if len(vertices) >= 2 and vertices[1] is self: + in_degrees += 1 + + return in_degrees @@ @property def out_degree(self) -> int: @@ - out_degrees = 0 - if len(self.edges) == 0: - return out_degrees - - for edge in self.edges: - if edge.type == EdgeType.DIRECTED and edge.node_one == self: - out_degrees += 1 - - return out_degrees + out_degrees = 0 + if len(self.edges) == 0: + return out_degrees + + for edge in self.edges: + if edge.edge_type() == EdgeType.DIRECTED: + vertices = edge.vertices() + if len(vertices) >= 1 and vertices[0] is self: + out_degrees += 1 + + return out_degrees + + @property + def edges(self) -> Set[Edge]: + return self.incoming_edges | self.outgoing_edgesWith these changes:
- Vertex ids are unique by default but still overridable.
- Vertices become usable as keys in
Graph.adjacency_list.- Degree / in_degree / out_degree work with the new
Edgeinterface and avoid stale state.- Neighbour management remains adjacency-based and a bit cleaner.
Also applies to: 39-56, 58-67, 72-91, 93-109, 111-127
datastructures/graphs/edge/edge.py (1)
2-5: FixEdge.identifierdefault & type to avoid shared UUID and mismatch
identifiercurrently has two problems:
uuid4()is evaluated at import time, so allEdgeinstances created without an explicit identifier will share the same UUID.- Annotation
AnyStrdoes not matchuuid4()’sUUIDreturn type.Given this is the base class, this breaks identity for all edge subclasses.
Consider updating imports and
__init__as follows:-from typing import AnyStr, Union -from .edge_type import EdgeType -from typing import Any, Dict, Optional, Generic, TypeVar, List -from uuid import uuid4 +from typing import Any, Dict, Optional, Generic, TypeVar, List, Union +from uuid import UUID, uuid4 +from .edge_type import EdgeType @@ def __init__( self, weight: Optional[Union[int, float]] = None, properties: Optional[Dict[str, Any]] = None, - identifier: AnyStr = uuid4(), + identifier: Optional[UUID] = None, ): - self.id = identifier + self.id = identifier if identifier is not None else uuid4() self.weight = weight self.properties = propertiesThis keeps UUID generation per-instance and aligns the type annotation with actual values.
Also applies to: 15-23
datastructures/graphs/graph.py (6)
17-25: Fix adjacency list factory and node bookkeeping in__init__Current constructor has several issues:
defaultdict(List[Vertex])uses the typing alias as a factory; you wantlistto get fresh lists.self.nodesis initialised empty andnode_countis derived from that, ignoring any vertices present in the providededge_list.node_countthen becomes stale as nodes are added later.A safer pattern:
def __init__(self, edge_list: List[Edge] = None): if edge_list is None: edge_list = [] self.edge_list = edge_list - self.adjacency_list = defaultdict(List[Vertex]) - self.__construct_adjacency_list() - self.nodes = [] - self.node_count = len(self.nodes) + # maps Vertex -> List[Vertex] + self.adjacency_list = defaultdict(list) + self.nodes: List[Vertex] = [] + + # Seed nodes from existing edges + for edge in self.edge_list: + for vertex in edge.vertices(): + if vertex not in self.nodes: + self.nodes.append(vertex) + + self.__construct_adjacency_list()Also drop
self.node_countentirely and uselen(self.nodes)orif not self.nodeselsewhere (seetopological_sorted_order).
26-37:add()misusesVertex.neighboursand instantiates abstractEdge
addis currently broken:
Vertex.neighboursis a computed property; appending to it does not persist adjacency.Edgeis abstract and its constructor doesn’t acceptnode_one/node_two, soEdge(node_one=..., node_two=...)will fail.self.nodesgets duplicate entries for the same vertex.Leverage
Vertex.add_adjacent_vertexand a concrete edge type, and avoid duplicates:def add(self, source_node: Vertex, destination_node: Vertex): @@ - source_node.neighbours.append(destination_node) - destination_node.neighbours.append(source_node) - edge = Edge(node_one=source_node, node_two=destination_node) - self.edge_list.append(edge) - self.adjacency_list[source_node].append(destination_node) - self.adjacency_list[destination_node].append(source_node) - self.nodes.append(source_node) - self.nodes.append(destination_node) + # Update vertex-level adjacency (undirected) + source_node.add_adjacent_vertex(destination_node) + + # Create an undirected edge between the two vertices + edge = UndirectedEdge(node_one=source_node, node_two=destination_node) + self.edge_list.append(edge) + + # Update graph-level adjacency + self.adjacency_list[source_node].append(destination_node) + self.adjacency_list[destination_node].append(source_node) + + if source_node not in self.nodes: + self.nodes.append(source_node) + if destination_node not in self.nodes: + self.nodes.append(destination_node)You’ll also need to import
UndirectedEdge:-from datastructures.graphs.edge import Edge +from datastructures.graphs.edge import Edge, UndirectedEdge
7-8: Build adjacency correctly for directed vs undirected edges
__construct_adjacency_listassumesedge.node_one/edge.node_twoonly and ignores direction, so directed edges won’t be represented correctly and undirected edges are only stored in one direction.Given the presence of
DirectedEdgeandUndirectedEdge, you can distinguish them explicitly:-from datastructures.graphs.edge import Edge +from datastructures.graphs.edge import Edge, UndirectedEdge, DirectedEdge @@ def __construct_adjacency_list(self): """ Construct adjacency list """ - for edge in self.edge_list: - self.adjacency_list[edge.node_one].append(edge.node_two) + for edge in self.edge_list: + if isinstance(edge, UndirectedEdge): + self.adjacency_list[edge.node_one].append(edge.node_two) + self.adjacency_list[edge.node_two].append(edge.node_one) + elif isinstance(edge, DirectedEdge): + self.adjacency_list[edge.source].append(edge.destination)If you later support hyper-edges, this method will need to be extended to handle multiple incident vertices per edge.
Also applies to: 39-45
61-112: Rewritetopological_sorted_order(visited map, recursion target, abstractStack)Current implementation has multiple correctness issues:
- Uses
self.node_countand buildsvisited_nodeskeyed byrange(self.node_count)instead of actualVertexinstances.- Recurses with
dfs(node)instead ofdfs(neighbour).- Cycle check compares
visited_nodes[node]instead of the neighbour’s state.- Instantiates abstract
Stackand relies on a non-existentstack.stackattribute.A DFS with color states and a plain list is simpler and correct:
def topological_sorted_order(self) -> List[Vertex]: @@ - # Nothing to do here - if self.node_count == 0: - return [] - - is_possible = True - stack = Stack() - - # By default all nodes are WHITE - visited_nodes = {node: white for node in range(self.node_count)} - - def dfs(node: Vertex): - nonlocal is_possible - - # Don't recurse further if we found a cycle already - if not is_possible: - return - - # start recursion - visited_nodes[node] = gray - - # Traverse on neighbouring nodes/vertices - if node in self.adjacency_list: - for neighbour in self.adjacency_list[node]: - if visited_nodes[neighbour] == white: - dfs(node) - elif visited_nodes[node] == gray: - # An Edge to a Gray vertex/node represents a cycle - is_possible = False - - # Recursion ends. We mark if as BLACK - visited_nodes[node] = black - stack.push(node) - - for node in self.nodes: - # if the node is unprocessed, then call DFS on it - if visited_nodes[node] == white: - dfs(node) - - return list(stack.stack) if is_possible else [] + if not self.nodes: + return [] + + is_possible = True + visited = {} # Vertex -> color + order: List[Vertex] = [] + + def dfs(node: Vertex): + nonlocal is_possible + + if not is_possible: + return + + visited[node] = gray + + # Traverse neighbouring nodes/vertices + for neighbour in self.adjacency_list.get(node, []): + state = visited.get(neighbour, white) + if state == white: + dfs(neighbour) + elif state == gray: + # An Edge to a Gray vertex/node represents a cycle + is_possible = False + return + + visited[node] = black + order.append(node) + + for node in self.nodes: + if visited.get(node, white) == white: + dfs(node) + + return list(reversed(order)) if is_possible else []This removes the dependency on an abstract
Stackimplementation and operates directly onVertexinstances.
118-138: Fix exception type inremove()and adjacency bucket checked inis_connected()Two issues here:
list.removeraisesValueError, notKeyError, so the currentexcept KeyErrorwill never fire.is_connectedchecksself.adjacency_list[node_two]instead ofnode_one’s adjacency, so it answers the wrong question.Suggested changes:
def remove(self, node: Vertex) -> None: @@ - for _, cxns in self.adjacency_list.items(): - try: - cxns.remove(node) - except KeyError: - pass - - try: - del self.adjacency_list[node] - except KeyError: - pass + for _, cxns in self.adjacency_list.items(): + try: + cxns.remove(node) + except ValueError: + # node not present in this adjacency bucket + pass + + # Remove the node's own adjacency bucket if present + self.adjacency_list.pop(node, None) @@ def is_connected(self, node_one: Vertex, node_two: Vertex) -> bool: - return ( - node_one in self.adjacency_list - and node_two in self.adjacency_list[node_two] - ) + return node_two in self.adjacency_list.get(node_one, [])You may also want to remove
nodefromself.nodesand prune any incident edges inself.edge_listfor full consistency, but that can be done as a follow-up.
140-167: Unify path methods to useVertexkeys (not.data) and correct path constructionAll three path helpers (
find_path,find_all_paths,find_shortest_path) share the same issues:
- They mix
Vertexinstances andnode.datawhen indexingadjacency_listand when trackingpath, which currently stores a nested structure (path = [path] + [node_one]).- Membership checks like
if node.data not in pathcompare a raw value againstVertexobjects, so visited detection fails and recursion can misbehave.They should consistently:
- Treat
adjacency_listasDict[Vertex, List[Vertex]].- Store
Vertexobjects inpath.- Use
node_one == node_two(or id) for equality.Concrete refactors:
def find_path( self, node_one: Vertex, node_two: Vertex, path=None @@ - if path is None: - path = [] - path = [path] + [node_one] - - if node_one.data == node_two.data: - return path - - if node_one.data not in self.adjacency_list: - return None - - for node in self.adjacency_list[node_one]: - if node.data not in path: - new_path = self.find_path(node, node_two, path) - - if new_path: - return new_path - - return None + if path is None: + path = [] + path = path + [node_one] + + if node_one == node_two: + return path + + for neighbour in self.adjacency_list.get(node_one, []): + if neighbour not in path: + new_path = self.find_path(neighbour, node_two, path) + if new_path: + return new_path + return None @@ def find_all_paths( self, node_one: Vertex, node_two: Vertex, path: List = None @@ - if path is None: - path = [] - path = path + [node_one] - - if node_one.data == node_two.data: - return [path] - - if node_one.data not in self.adjacency_list: - return [] - - paths = [] - - for node in self.adjacency_list[node_one.data]: - if node not in path: - newpaths = self.find_all_paths(Vertex(node), node_two, path) - for newpath in newpaths: - paths.append(newpath) - - return paths + if path is None: + path = [] + path = path + [node_one] + + if node_one == node_two: + return [path] + + paths: List[List[Vertex]] = [] + for neighbour in self.adjacency_list.get(node_one, []): + if neighbour not in path: + new_paths = self.find_all_paths(neighbour, node_two, path) + paths.extend(new_paths) + return paths @@ def find_shortest_path( self, node_one: Vertex, node_two: Vertex, path: List = None @@ - if path is None: - path = [] - - path = path + [node_one] - - if node_one.data == node_two.data: - return path - - if node_one.data not in self.adjacency_list: - return None - - shortest = None - - for node in self.adjacency_list[node_one]: - if node.data not in path: - newpath = self.find_shortest_path(node, node_two, path) - if newpath: - if not shortest or len(newpath) < len(shortest): - shortest = newpath - - return shortest + if path is None: + path = [] + + path = path + [node_one] + + if node_one == node_two: + return path + + shortest: Union[List[Vertex], None] = None + + for neighbour in self.adjacency_list.get(node_one, []): + if neighbour not in path: + new_path = self.find_shortest_path(neighbour, node_two, path) + if new_path and (shortest is None or len(new_path) < len(shortest)): + shortest = new_path + + return shortestThis makes all three helpers consistent and usable on the same
Vertex-keyed adjacency structure.Also applies to: 169-196, 198-224
🧹 Nitpick comments (3)
datastructures/graphs/undirected/utils.py (1)
5-94:closest_nodeimplementation looks correct; consider documenting assumptions and failure mode.The BFS distance and parent-pointer path reconstruction are sound for a connected tree with nodes in
[0, n-1], and the tie-breaking on index is implemented correctly. The only thing I’d suggest is:
- Documenting that
nis expected to be the number of nodes labeled0..n-1and that the graph is assumed connected / tree-like.- Optionally deciding how you want to handle invalid queries where
start/endare disconnected or unreachable fromtarget— right now those yield-1in the result without an explicit error.These are non-blocking API/UX clarifications rather than correctness issues.
DIRECTORY.md (2)
83-84: Fix indentation for “Frog Position After T Seconds” to satisfy MD007.markdownlint expects these bullets to use 2 spaces for the parent and 4 for its child. You can align them with the surrounding sections like this:
- * Frog Position After T Seconds - * [Test Frog Position After T Seconds](https://github.com/BrianLusina/PythonSnips/blob/master/algorithms/graphs/frog_position_after_t_seconds/test_frog_position_after_t_seconds.py) + * Frog Position After T Seconds + * [Test Frog Position After T Seconds](https://github.com/BrianLusina/PythonSnips/blob/master/algorithms/graphs/frog_position_after_t_seconds/test_frog_position_after_t_seconds.py)This matches the “Algorithms” section style and clears the MD007 warnings for lines 83–84.
305-308: Reduce indentation for Binary Tree “Tree” entries to be consistent with other nested lists.Lines 305–308 are indented 2 spaces more than markdownlint expects for that nesting level. Align them with the rest of the “Trees > Binary > Tree” hierarchy:
- * Tree - * [Binary Tree](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/trees/binary/tree/binary_tree.py) - * [Test Binary Tree](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/trees/binary/tree/test_binary_tree.py) - * [Test Binary Tree Deserialize](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/trees/binary/tree/test_binary_tree_deserialize.py) - * [Test Binary Tree Min Camera Cover](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/trees/binary/tree/test_binary_tree_min_camera_cover.py) + * Tree + * [Binary Tree](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/trees/binary/tree/binary_tree.py) + * [Test Binary Tree](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/trees/binary/tree/test_binary_tree.py) + * [Test Binary Tree Deserialize](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/trees/binary/tree/test_binary_tree_deserialize.py) + * [Test Binary Tree Min Camera Cover](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/trees/binary/tree/test_binary_tree_min_camera_cover.py)(Adjust the exact structure as you prefer, but the key is to use 6 spaces for items at this depth instead of 8 so MD007 passes.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
datastructures/graphs/undirected/images/examples/closest_node_to_path_in_tree_example_1.pngis excluded by!**/*.pngdatastructures/graphs/undirected/images/examples/closest_node_to_path_in_tree_example_2.pngis excluded by!**/*.pngdatastructures/graphs/undirected/images/examples/closest_node_to_path_in_tree_example_3.pngis excluded by!**/*.png
📒 Files selected for processing (14)
DIRECTORY.md(3 hunks)datastructures/graphs/edge/edge.py(1 hunks)datastructures/graphs/edge/edge_directed.py(1 hunks)datastructures/graphs/edge/edge_hyper.py(1 hunks)datastructures/graphs/edge/edge_self.py(1 hunks)datastructures/graphs/edge/edge_undirected.py(1 hunks)datastructures/graphs/graph.py(1 hunks)datastructures/graphs/test_vertex.py(1 hunks)datastructures/graphs/undirected/README.md(1 hunks)datastructures/graphs/undirected/__init__.py(1 hunks)datastructures/graphs/undirected/test_utils.py(1 hunks)datastructures/graphs/undirected/undirected_graph.py(1 hunks)datastructures/graphs/undirected/utils.py(1 hunks)datastructures/graphs/vertex.py(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- datastructures/graphs/undirected/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- datastructures/graphs/edge/edge_self.py
- datastructures/graphs/test_vertex.py
- datastructures/graphs/edge/edge_undirected.py
🧰 Additional context used
🧬 Code graph analysis (8)
datastructures/graphs/undirected/test_utils.py (1)
datastructures/graphs/undirected/utils.py (1)
closest_node(5-94)
datastructures/graphs/undirected/utils.py (1)
datastructures/graphs/graph.py (1)
find_path(140-167)
datastructures/graphs/graph.py (5)
datastructures/stacks/__init__.py (1)
Stack(7-48)datastructures/graphs/vertex.py (2)
Vertex(8-127)neighbours(59-69)datastructures/graphs/edge/edge.py (1)
Edge(10-37)datastructures/graphs/undirected/undirected_graph.py (2)
bfs_from_root_to_target(11-12)bfs_from_node(14-15)datastructures/graphs/undirected/utils.py (1)
find_path(31-70)
datastructures/graphs/undirected/undirected_graph.py (2)
datastructures/graphs/graph.py (3)
Graph(12-230)bfs_from_root_to_target(47-52)bfs_from_node(55-59)datastructures/graphs/vertex.py (1)
Vertex(8-127)
datastructures/graphs/undirected/__init__.py (1)
datastructures/graphs/undirected/undirected_graph.py (1)
UndirectedGraph(6-15)
datastructures/graphs/edge/edge_directed.py (5)
datastructures/graphs/edge/edge.py (3)
edge_type(29-30)Edge(10-37)vertices(36-37)datastructures/graphs/edge/edge_hyper.py (2)
edge_type(27-28)vertices(30-31)datastructures/graphs/edge/edge_self.py (2)
edge_type(27-28)vertices(30-31)datastructures/graphs/edge/edge_undirected.py (2)
edge_type(31-32)vertices(34-35)datastructures/graphs/edge/edge_type.py (1)
EdgeType(5-10)
datastructures/graphs/vertex.py (2)
datastructures/graphs/edge/edge.py (1)
Edge(10-37)datastructures/graphs/edge/edge_type.py (1)
EdgeType(5-10)
datastructures/graphs/edge/edge.py (5)
datastructures/graphs/edge/edge_directed.py (2)
edge_type(30-31)vertices(33-34)datastructures/graphs/edge/edge_hyper.py (2)
edge_type(27-28)vertices(30-31)datastructures/graphs/edge/edge_self.py (2)
edge_type(27-28)vertices(30-31)datastructures/graphs/edge/edge_undirected.py (2)
edge_type(31-32)vertices(34-35)datastructures/graphs/edge/edge_type.py (1)
EdgeType(5-10)
🪛 markdownlint-cli2 (0.18.1)
DIRECTORY.md
83-83: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
84-84: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
214-214: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
215-215: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
216-216: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
217-217: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
218-218: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
219-219: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
220-220: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
221-221: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
222-222: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
223-223: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
224-224: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
225-225: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
226-226: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
305-305: Unordered list indentation
Expected: 6; Actual: 8
(MD007, ul-indent)
306-306: Unordered list indentation
Expected: 6; Actual: 8
(MD007, ul-indent)
307-307: Unordered list indentation
Expected: 6; Actual: 8
(MD007, ul-indent)
308-308: Unordered list indentation
Expected: 6; Actual: 8
(MD007, ul-indent)
🔇 Additional comments (1)
datastructures/graphs/undirected/__init__.py (1)
1-4: Re-export ofUndirectedGraphlooks good.The package-level import and
__all__definition are straightforward and keep the public surface clean (datastructures.graphs.undirected.UndirectedGraph).
| @parameterized.expand( | ||
| [ | ||
| (3, [[0, 1], [1, 2]], [[0, 2, 1]], [1]), | ||
| (4, [[0, 1], [1, 2], [1, 3]], [[2, 3, 0]], [1]), | ||
| ( | ||
| 6, | ||
| [[0, 1], [0, 2], [0, 3], [0, 4], [0, 5]], | ||
| [[1, 5, 2], [2, 3, 4]], | ||
| [0, 0], | ||
| ), | ||
| ( | ||
| 7, | ||
| [[0, 1], [0, 2], [0, 3], [1, 4], [2, 5], [2, 6]], | ||
| [[5, 3, 4], [5, 3, 6]], | ||
| [0, 2], | ||
| ), | ||
| (3, [[0, 1], [1, 2]], [[0, 1, 2]], [1]), | ||
| (3, [[0, 1], [1, 2]], [[0, 0, 0]], [0]), | ||
| ] | ||
| ) | ||
| def test_closest_node( | ||
| self, n: int, edges: List[List[int]], query: List[List[int]], expected: int | ||
| ): | ||
| actual = closest_node(n, edges, query) | ||
| self.assertEqual(expected, actual) |
There was a problem hiding this comment.
Fix type hint for expected to match closest_node’s return type.
The test cases pass lists (e.g., [1], [0, 0]), and closest_node returns List[int], but the signature currently uses expected: int. To keep hints accurate:
- def test_closest_node(
- self, n: int, edges: List[List[int]], query: List[List[int]], expected: int
- ):
+ def test_closest_node(
+ self, n: int, edges: List[List[int]], query: List[List[int]], expected: List[int]
+ ):Optional: consider renaming query to queries to reflect that it’s a list of query triples.
📝 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.
| @parameterized.expand( | |
| [ | |
| (3, [[0, 1], [1, 2]], [[0, 2, 1]], [1]), | |
| (4, [[0, 1], [1, 2], [1, 3]], [[2, 3, 0]], [1]), | |
| ( | |
| 6, | |
| [[0, 1], [0, 2], [0, 3], [0, 4], [0, 5]], | |
| [[1, 5, 2], [2, 3, 4]], | |
| [0, 0], | |
| ), | |
| ( | |
| 7, | |
| [[0, 1], [0, 2], [0, 3], [1, 4], [2, 5], [2, 6]], | |
| [[5, 3, 4], [5, 3, 6]], | |
| [0, 2], | |
| ), | |
| (3, [[0, 1], [1, 2]], [[0, 1, 2]], [1]), | |
| (3, [[0, 1], [1, 2]], [[0, 0, 0]], [0]), | |
| ] | |
| ) | |
| def test_closest_node( | |
| self, n: int, edges: List[List[int]], query: List[List[int]], expected: int | |
| ): | |
| actual = closest_node(n, edges, query) | |
| self.assertEqual(expected, actual) | |
| @parameterized.expand( | |
| [ | |
| (3, [[0, 1], [1, 2]], [[0, 2, 1]], [1]), | |
| (4, [[0, 1], [1, 2], [1, 3]], [[2, 3, 0]], [1]), | |
| ( | |
| 6, | |
| [[0, 1], [0, 2], [0, 3], [0, 4], [0, 5]], | |
| [[1, 5, 2], [2, 3, 4]], | |
| [0, 0], | |
| ), | |
| ( | |
| 7, | |
| [[0, 1], [0, 2], [0, 3], [1, 4], [2, 5], [2, 6]], | |
| [[5, 3, 4], [5, 3, 6]], | |
| [0, 2], | |
| ), | |
| (3, [[0, 1], [1, 2]], [[0, 1, 2]], [1]), | |
| (3, [[0, 1], [1, 2]], [[0, 0, 0]], [0]), | |
| ] | |
| ) | |
| def test_closest_node( | |
| self, n: int, edges: List[List[int]], query: List[List[int]], expected: List[int] | |
| ): | |
| actual = closest_node(n, edges, query) | |
| self.assertEqual(expected, actual) |
🤖 Prompt for AI Agents
In datastructures/graphs/undirected/test_utils.py around lines 9 to 33, the
test_closest_node signature types expected an int but the test data and
closest_node return a List[int]; update the type hint for expected to be
List[int] to match the function return type and test cases, and optionally
rename the parameter query to queries (and its usages) to reflect it is a list
of query triples.
| def bfs_from_root_to_target(self, root: Vertex, target: Vertex) -> Set[Vertex]: | ||
| pass | ||
|
|
||
| def bfs_from_node(self, source: Vertex) -> Set[Vertex]: | ||
| pass |
There was a problem hiding this comment.
BFS methods currently no-op and violate the Graph contract.
bfs_from_root_to_target and bfs_from_node both just pass, so they return None despite their signatures and the base class docstrings promising a BFS result. That’s a behavioral bug even if these methods aren’t used yet.
I’d recommend at least failing loudly for now:
- def bfs_from_root_to_target(self, root: Vertex, target: Vertex) -> Set[Vertex]:
- pass
+ def bfs_from_root_to_target(self, root: Vertex, target: Vertex) -> Set[Vertex]:
+ raise NotImplementedError("bfs_from_root_to_target is not implemented yet")
- def bfs_from_node(self, source: Vertex) -> Set[Vertex]:
- pass
+ def bfs_from_node(self, source: Vertex) -> Set[Vertex]:
+ raise NotImplementedError("bfs_from_node is not implemented yet")Or, if you’re ready, implement proper BFS using self.adjacency_list so these methods become usable.
📝 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.
| def bfs_from_root_to_target(self, root: Vertex, target: Vertex) -> Set[Vertex]: | |
| pass | |
| def bfs_from_node(self, source: Vertex) -> Set[Vertex]: | |
| pass | |
| def bfs_from_root_to_target(self, root: Vertex, target: Vertex) -> Set[Vertex]: | |
| raise NotImplementedError("bfs_from_root_to_target is not implemented yet") | |
| def bfs_from_node(self, source: Vertex) -> Set[Vertex]: | |
| raise NotImplementedError("bfs_from_node is not implemented yet") |
Vertex & Graph implementations
Checklist:
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.