Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Disjoint Sets primitives #9145
Disjoint Sets primitives #9145
Changes from all commits
fbfd1da
60b8f06
dbe0313
0652c01
ce53191
3f41c0e
c17861b
a023ad7
571aece
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 also like the idea that this is a Property renamed to "DisjointSets".
@gregory-paidis-sonarsource wdyt?
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 prefer it as a method, since "property" semantically means light-weight execution, and this method has a lot of logic.
On a more general note, what about not exposing any of the
Union
,FindRoot
methods?Isn't what we want from this class always the same?
We could make the class static and expose only an
Execute
, and return the list of lists. E.g.:What is the reason that the caller needs to care about the algorithm's implementation?
This is even more true now that we renamed it from
DisjointSetPrimitives
toDisjointSets
and made it non-static.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 like @gregory-paidis-sonarsource suggestion even more.
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.
We discussed it offline, here is the wrap-up:
(Antonio feel free to resolve this when you read it)
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 would expect the person who opened the discussion thread to resolve it.
@mary-georgiou-sonarsource As a reviewer and initiator of the proposal to extract the three methods into a dedicated PR, please solve the discussion if you agree with the conclusion above.
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 think the last
OrderBy
can be removed.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 think it is there to get deterministic ordering of the groups.
If this is something pertinent specifically to the too-many-responsibilities PR, we might want to do it after this method returns the value, in the analyzer.
If you think it's supposed to always happen, we should leave it 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.
That was my understanding as well, but then if you remove it, tests don't fail.
We should add a test that fails if it's removed :/
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.
The method returns a list of lists. Lists have order, so if an order is present, it either has semantics or should be at least deterministic. Returning a set of sets to go through them again and sort them has just a performance cost and doesn't bring any benefits.
I think that generalizing prematurely this data structure to capture cases that don't exist yet is unfruitful.
Yes, that's a good point. I have added a test that specifically tests for ordering.
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.
Please also add a comment about the last OrderBy to explain that it's for retaining the lists order.