-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Added Kann's algorithm #1676
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
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
* Kahn's Algorithm is used for finding the topological sorting order of directed acyclic graph | ||
* | ||
* @param graph - Graph (adjacency list) | ||
* @param n - Size of graph |
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
* | ||
*/ | ||
|
||
export function KahnsAlgorithm(graph, n) { |
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
inDegree[neighbour] += 1 | ||
} | ||
} | ||
const queue = new Queue() |
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
const node = queue.dequeue() | ||
result.push(node) | ||
for (const neighbour of graph[node]) { | ||
inDegree[neighbour] -= 1 |
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.
inDegree[neighbour] -= 1 | |
inDegree[neighbour]-- |
Graphs/KahnsAlgorithm.js
Outdated
result.push(node) | ||
for (const neighbour of graph[node]) { | ||
inDegree[neighbour] -= 1 | ||
if (inDegree[neighbour] == 0) { |
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.
if (inDegree[neighbour] == 0) { | |
if (inDegree[neighbour] === 0) { |
Graphs/test/KannsAlgorithm.test.js
Outdated
@@ -0,0 +1,13 @@ | |||
import { kannsAlgorithm } from '../KannsAlgorithm' |
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
@@ -0,0 +1,13 @@ | |||
import { KahnsAlgorithm } from '../KahnsAlgorithm' | |||
|
|||
test('Graph without cycle', () => { |
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, |
Summary
File Added