Conversation
|
@dionyziz can you help reviewing and testing this change? |
|
Thanks for the feedback @dionyziz @lutzroeder . Im working on the changes |
|
|
@pkakelas could also help with this review |
|
LGTM, good job :) |
|
This pull request now fixes #87 .
|
|
PTAL |
lutzroeder
force-pushed
the
master
branch
3 times, most recently
from
64a1b87 to
cf48a25
Compare
|
Hi @mstou Thanks for the work! I'm trying to review the open PR in order to reduce the amount of issues and PRs. If not, I will try to continue it. |
|
Hi @assafsun, I'll be glad to rebase this to master and squash the commits |
|
@mstou Thanks for the update, I will follow this PR. |
|
Hi @assafsun, sorry for the delay, I rebased to master and squashed the fixup commits. Take a look whenever you have time :) |
| var outVertex = inVertex === edge.v ? edge.w : edge.v; | ||
|
|
||
| if (weightFn({ v: inVertex, w: outVertex }) < 0) { | ||
| negativeEdgeExists = true; |
There was a problem hiding this comment.
I think you can break in case a negative edge was found
| } | ||
| } | ||
|
|
||
| if (negativeEdgeExists) { |
There was a problem hiding this comment.
I think you can extract this if outside the scope and just write:
return negativeEdgeExists? bellmanFord(g, source, weightFn, edgeFn): dijkstra(g, source, weightFn, edgeFn)
| nodes = g.nodes(); | ||
|
|
||
| var relaxEdge = function(edge) { | ||
| var edgeWeight = weightFn(edge); |
There was a problem hiding this comment.
can we switch some of the 'var' declarations in this PR to let?
| postorder: require("./postorder"), | ||
| preorder: require("./preorder"), | ||
| prim: require("./prim"), | ||
| shortestPaths: require("./shortest-paths"), |
There was a problem hiding this comment.
It looks good that you extract it but the documentation will need to mention that this API can run bellman ford or Dijkstra
|
|
||
| module.exports = tests; | ||
|
|
||
| function tests(algorithm) { |
There was a problem hiding this comment.
Maybe adding a comment that this reflect for bellman ford or Dijkstra?
|
@mstou Thank you for the new iteration. I ran your code and the tests passed. |
|
Personally, I really like this pr to get the final approval. |
7 participants
Fixes #87 .
As requested in issue #87, I implemented the bellman-ford algorithm, respecting the coding style of the repository. I included the bellmanFord function in the module exported by lib/alg and added some tests in tests/alg/bellman-ford-tests.js