Tarjan's strongly connected component algorithm #14
Conversation
clue
changed the title
| use Fhaculty\Graph\Vertex; | ||
|
|
||
| /** | ||
| * @see http://github.com/Trismegiste/Mondrian/blob/master/Graph/Tarjan.php |
There was a problem hiding this comment.
Some documentation would be much appreciated :)
|
Thanks for your continuous effort and migrating this over! 👍 This new PR is much easier to review, some of the initial discussion can be found in the original PR linked above. |
|
Ping @dav-m85, perhaps you can use some support so that we can get this in? :) |
|
All requested fixes have been done. Sorry for the lag :) |
|
Thanks for the update and your patience @dav-m85! :-) I'll review this shortly. |
| * @throws InvalidArgumentException For undirected graph argument. | ||
| * @return Vertices[] Array of Strongly Connected components. | ||
| */ | ||
| public function getStronglyConnectedVerticesFromDirectedGraph(Graph $graph) |
There was a problem hiding this comment.
Naming this method is indeed a tough challenge :-)
Afaict this appears to be the first method so far which returns not only an array of vertex instances, but an array of vertices 😮
Admittedly, I'm not a big fan of the current nor the suggested naming convention. The current suggested name matches this rather elaborate style, how about using something more concise?
Does naming this "TarjansAlgorithm::getConnectedComponents(Graph)" make sense to you?
|
Awesome, functionally the changes LGTM! 👍 Let's address the outstanding naming issues and get this in 👍 |
|
This ticket got automatically closed after renaming the master branch to This pull request is open for quite a while now and didn't receive any updates since, this is why I suggest we leave this closed for now. If this topic is still relevant and needs some further investigation, we can always reopen it in the future and take another look 👍 |
As requested, reopened graphp/graph#106 here.
Closes #11.