Fix topological sort order#14609
Conversation
algorithms-keeper
Bot
added
enhancement
| sort = topological_sort(neighbor, visited, sort) | ||
| # if all neighbors visited add current to sort | ||
| sort.append(current) | ||
| # if all neighbors visited add current before its descendants |
There was a problem hiding this comment.
This comment seems inaccurate. At this point, the DFS has already visited all reachable descendants, and current is being inserted before them in the final ordering. Consider rewording it to avoid confusion.
| # if all neighbors visited add current to sort | ||
| sort.append(current) | ||
| # if all neighbors visited add current before its descendants | ||
| sort.insert(0, current) |
There was a problem hiding this comment.
Using insert(0, current) fixes the ordering, but it is O(n) for every insertion. For larger graphs, consider appending during DFS and reversing once at the end for better performance.
| # if all neighbors visited add current before its descendants | ||
| sort.insert(0, current) | ||
| # if all vertices haven't been visited select a new one to visit | ||
| if len(visited) != len(vertices): |
There was a problem hiding this comment.
This function depends on the global vertices list, which makes it less reusable. Consider passing vertices as a parameter instead of relying on module-level state.
There was a problem hiding this comment.
Consider adding a small assertion-based test instead of only printing the result. That would make it easier to verify the corrected topological order automatically.
| sort = topological_sort(vertice, visited, sort) | ||
| # return sort | ||
| _visit(vertice, visited, sort, graph) | ||
| sort.reverse() |
There was a problem hiding this comment.
This is the core fix — adding sort.reverse() correctly produces
topological order since DFS post-order appends nodes after all
descendants are visited, giving reverse topological order.
Worth adding a brief comment explaining why the reverse is needed
so future readers understand the intent:
Post-order DFS gives reverse topological order, so reverse to fix
sort.reverse()
| """Perform topological sort on a directed acyclic graph.""" | ||
| current = start | ||
| # add current to visited | ||
| def _visit( |
There was a problem hiding this comment.
Good refactor extracting _visit() as a private helper — separating
the DFS traversal from the main topological_sort() logic makes both
functions easier to understand and test independently.
One suggestion: _visit() modifies visited and post_order in place
via side effects but has no doctest. Since it's a private helper
this is acceptable, but a one-line docstring explaining the side
effects would help:
def _visit(current, visited, post_order, graph):
"""DFS helper — appends nodes to visited and post_order in place."""
| start: str, | ||
| visited: list[str], | ||
| sort: list[str], | ||
| graph: dict[str, list[str]] | None = None, |
There was a problem hiding this comment.
Using mutable default arguments as None with an explicit check on
lines 50-53 is the correct Python pattern — good call avoiding the
classic mutable default argument trap.
However the parameter name "sort" on line 35 is a bit confusing
since "sort" is also a Python built-in. Consider renaming to
"result" or "order" to avoid shadowing and improve readability:
def topological_sort(
start: str,
visited: list[str],
order: list[str], # renamed from "sort"
graph: dict[str, list[str]] | None = None,
vertices_list: list[str] | None = None,
) -> list[str]:
3 participants
Describe your change:
Fixes #12192.
topological_sort()was appending each vertex after visiting its descendants, so the returned list was the reverse of the intended topological order. This keeps the existing DFS structure and inserts each completed vertex at the front of the result instead, so parents appear before their descendants.Verified example output:
Checklist:
Local verification:
python3 sorts/topological_sort.pyruff check sorts/topological_sort.pyruff format --check sorts/topological_sort.pygit diff --checkNote:
python3 -m pytest sorts/topological_sort.pycollects no tests because this existing file has no doctests.