Skip to content
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

Topologicalsort #13

Merged
merged 2 commits into from
Mar 4, 2013
Merged

Conversation

clemens-tolboom
Copy link
Collaborator

I've added some more tests. My point of view is tests should help new developers to understand more so I used letters and added more documentation.

I'm not sure why the Algorithm returns Vertices instead of Ids. Is there a reason for this?

The new tests are explicitly written for the
"
the topologic sorting may be non-unique depending on your edges. this
algorithm tries to keep the order of vertices as added to the graph in
this case.
"
which feels 'wrong' as we should not rely on the order.

What do you think?

@clue
Copy link
Member

clue commented Mar 4, 2013

I'm not sure why the Algorithm returns Vertices instead of Ids. Is there a reason for this?

All algorithms operate on object instances instead of IDs, because it's easier to work with and one can easily access the remaining attributes such as IDs.

[..] we should not rely on the order

That's something I've been thinking about as well, but IMHO having a strict order makes things more predictable - and testable. Any thoughts / other suggestions?

Your additional tests look great, thanks for the effort!

clue added a commit that referenced this pull request Mar 4, 2013
@clue clue merged commit 239838b into graphp:topologicalsort Mar 4, 2013
@clemens-tolboom clemens-tolboom deleted the topologicalsort branch March 4, 2013 15:19
@clemens-tolboom clemens-tolboom restored the topologicalsort branch March 4, 2013 15:20
@clemens-tolboom
Copy link
Collaborator Author

Maybe I should wait deleting the branch.

Regarding testing I did a in_array head / tail tests:

    $tsl = $graph_object->getTopologicalSortedList();
    $dummy = $tsl;

    $head = array_shift($dummy);
    $this->assertTrue(in_array($head, array('a', 'f', 'p')), "The TSL starts with a, f or p: " . join(', ', $tsl));

    $tail = array_pop($dummy);
    $this->assertTrue(in_array($tail, array('c', 'g', 'q')), "The TSL ends with c, g or q: " . join(', ', $tsl));

Taken from patch http://drupal.org/files/core-graph-1915308-37_1.patch from issue http://drupal.org/node/1915308

BTW you would you generate a big Graph for recursion depth testing. I want to try unwinding the recursion.

@clue
Copy link
Member

clue commented Mar 4, 2013

Your changes have been merged already, so it's safe to delete this branch and/or create a new branch and open a new PR?

Additional tests are always welcome! Your suggestion seems more like an unclean API you're testing against, so maybe we should provide some additional convenience methods?

Regarding generating a bigger graph, just testing it against something like this is likely going to fail if you have the xdebug extension installed:

$length = 1000;
$graph = new Graph();
$last = $graph->createVertex(0);
for ($i = 1; $i <= $length; ++$i) {
    $new = $graph->createVertex($i);
    $last->createEdgeTo($new);
    $last = $new;
}

@clemens-tolboom clemens-tolboom deleted the topologicalsort branch March 5, 2013 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants