Tree configuration #138
Merged
Tree configuration #138
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We don't need an interface (yet), since there is only one implementation. A client (user) of the package can easily add its own interface as necessary.
This makes the test data specify the label: number to make it more readable; it is hard to map the four numbers to the field name of original createTreeTest type.
We only need to store the IDs once; the index into the ID slice represents that ID's position in the tree. I think it will be faster for small trees to do this approach; we may consider to add a benchmark to find the breaking point where it is faster to use a map for this. If a map is needed for larger trees for speed, we could consider to revert this change, or allocate the map only if the tree is actually large.
meling
requested changes
Dec 22, 2024
meling
reviewed
Dec 22, 2024
This moves the code to compute the height together rather than splitting it with code for argument checks. This makes it easier to see that computing the height could be moved to a external function if it was ever necessary to do separately.
In Go we try to avoid using the Get prefix.
We don't need to check that t.id's position isn't -1 since we do that check in CreateTree, so we would never get -1 here.
NodeChildren returns this tree's replica's children, while the ChildrenOf(node) returns the node's children. This also removes an unnecessary allocation; we can return nil instead of an empty slice of IDs.
NodeHeight returns the height of this tree's replica's height.
We can return a subsclice of the original to avoid allocating.
The math.Pow calls in a loop can be slow; this should be faster.
This avoids the use of math.Pow in a loop. Does not matter much, but it is an easy fix and makes the code easier to read as well.
This makes it clearer that it refers to tree positions and the "To" word implies that it is a mapping to an "ID".
This avoids having to provide a lessFunc.
I removed the "currently only fault free tree config is supported" comment; don't feel it provides enough context -- at least not yet.
meling
requested changes
Jan 2, 2025
Member
meling
left a comment
meling
left a comment
There was a problem hiding this comment.
I had a bunch of comments that I had forgotten to submit a review for. Sorry about that. I fixed most of them already just now, but there are two remaining issues. Could you please fix them.
meling
reviewed
Jan 3, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
create trees from position slice