Skip to content

Fix topological sort order #14609

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Kcstring wants to merge 2 commits into
base: master
Choose a base branch
from
+43 −17
Open

Fix topological sort order #14609

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a0a8209
Fix topological sort order
Apr 29, 2026
504c6a6
fix: address topological sort review
May 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions sorts/topological_sort.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ def topological_sort(start: str, visited: list[str], sort: list[str]) -> list[st
# if neighbor not in visited, visit
if neighbor not in visited:
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
Copy link
Copy Markdown

@jgohel9902 jgohel9902 Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

sort.insert(0, current)
Copy link
Copy Markdown

@jgohel9902 jgohel9902 Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 vertices haven't been visited select a new one to visit
if len(visited) != len(vertices):
Copy link
Copy Markdown

@jgohel9902 jgohel9902 Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

for vertice in vertices:
Expand Down