Skip to content

Conversation

@phyrwork
Copy link
Contributor

@phyrwork phyrwork commented Aug 6, 2017

No description provided.

Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@phyrwork Thank you for filing this PR, I agree that this feature makes perfect sense!

Can you add some test cases to verify this works as expected?

// track depth
$nodesNextLevel = $children->count();
if (--$nodesThisLevel === 0) {
if (++$currentDepth > $maxDepth) goto done;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested alternative:

if (++$currentDepth > $maxDepth) {
    break;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will break to foreach($children...) and not done:.

If you're anti goto: even when it's appropriate, I suggest return new Vertices($visited) instead, though I see this as being less DRY.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I follow, but the break would be within the do … while loop only? Either way, I'm not anti goto and don't think any of the suggested alternatives really make that much of a difference. The less code, the better; the more structured, the better. This line of code currently violates existing coding standards due to lack of braces (easy fix), while we're at it we could replace the goto and remove another line. Slightly off-topic, but DRY is not really about lines of code, but more about not making business decisions at different locations which doesn't really apply here.

@phyrwork
Copy link
Contributor Author

phyrwork commented Oct 5, 2018

Test cases added, goto removed, brace added for if statement. Ship it!

@clue clue added this to the v0.8.2 milestone Oct 5, 2018
Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, changes LGTM, thank you! :shipit: 🎉

@clue clue merged commit 6b15c28 into graphp:master Oct 5, 2018
@clue clue modified the milestones: v0.8.2, v0.9.0 Oct 9, 2018
@clue clue modified the milestones: v0.9.0, v0.8.1, v0.8.2 Feb 17, 2020
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