-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Added Kann's algorithm #1676
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.
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
Added Kann's algorithm #1676
Conversation
Codecov Report
All modified and coverable lines are covered by tests ✅
Project coverage is 84.74%. Comparing base (
9010481) to head (191993f).
Additional details and impacted files
@@ Coverage Diff @@ ## master #1676 +/- ## ========================================== + Coverage 84.65% 84.74% +0.08% ========================================== Files 378 380 +2 Lines 19744 19850 +106 Branches 2951 2974 +23 ========================================== + Hits 16715 16821 +106 Misses 3029 3029
☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's "Kahn's algorithm", not "Kann's algorithm".
The implementation looks correct, there is a runtime concern though.
Graphs/KahnsAlgorithm.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant. Just use graph.length.
Graphs/KahnsAlgorithm.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer calling this topoSort or similar.
Graphs/KahnsAlgorithm.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use a queue when you can get away with just using JS data structures, specifically, a stack?
Also, why call this "queue" rather than giving it a more descriptive name like "roots"?
Graphs/KahnsAlgorithm.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Graphs/KahnsAlgorithm.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Graphs/test/KannsAlgorithm.test.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You forgot to delete this file.
Graphs/test/KahnsAlgorithm.test.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As said, these two test blocks should be in a describe("Kahn's algorithm", () => {...}) block.
Deleted KahnaAlgorithm.js
Deleted Graphs/KannsAlgorithm.js
Graphs/test/KahnsAlgorithm.test.js
Hai,
I am closing these pull requests as they involve substantial file modifications, resulting in an overly large number of commits.
Summary
File Added