-
Notifications
You must be signed in to change notification settings - Fork 45
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
Fix/shortest paths #71
Conversation
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.
Excellent work! I have a few suggestions here.
src/index.ts
Outdated
removeEdge(path[0], path[1]); | ||
removeEdge(path[1], path[0]); | ||
removedEdges.push([path[0], path[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.
This code creates many new arrays and one new closure each time it runs, which could degrade performance as all of those things need to be garbage collected.
I would suggest the following refactoring:
const u = path[0];
const v = path[1];
if (hasEdge(u, v)) {
removedEdges.push({ u, v, weight: getEdgeWeight(u, v) });
removeEdge(u, v);
}
if (hasEdge(v, u)) {
removedEdges.push({ u: v, v: u, weight: getEdgeWeight(v, u) });
removeEdge(v, u);
}
return d === item; | ||
}).length > 0 | ||
); | ||
return arr.includes(item); |
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.
Nice
src/index.ts
Outdated
@@ -394,12 +394,18 @@ export function Graph(serialized?: Serialized) { | |||
function shortestPaths(source: NodeId, destination: NodeId) { | |||
let path = shortestPath(source, destination); | |||
const paths = [path], | |||
removedEdges = [], | |||
removedEdges: any = [], |
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 bet it's possible to add proper types 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.
LGTM! Thank you.
Better to check if the link really exists before deletion otherwise it can create unwanted links at the end