refine _build_tree #3
Conversation
Liwink commented Dec 14, 2016
Liwink commented - using enumerate to replace while and index
- better indentation
1 similar comment
joowani left a comment
joowani left a comment There was a problem hiding this comment.
Thanks for the PR! I appreciate your interest in and contribution to the binarytree project :)
I've reviewed the changes and left a few comments.
binarytree/__init__.py Outdated
| _add_right(parent_node, child_node) | ||
| nodes[index] = child_node | ||
| index += 1 | ||
| for index, value in enumerate(values[1:], 1): |
There was a problem hiding this comment.
A slicing operation like values[1:] creates a new list, and is costly when the number of elements in the list is large. While your suggested change does make the code cleaner, I don't think it justifies sacrificing performance albeit small.
There was a problem hiding this comment.
Thank you, great advice! And I made a change.
| index += 1 | ||
| for index, value in enumerate(values[1:], 1): | ||
| if value == _null: | ||
| continue |
There was a problem hiding this comment.
I think using the continue here is fine 👍
joowani left a comment
joowani left a comment There was a problem hiding this comment.
Left you another comment. Also I created a dev branch for future merges and development so please redirect your PR there. Thanks!
| nodes[index] = child_node | ||
| index += 1 | ||
| for index, value in enumerate(values): | ||
| if value == _null or index == 0: |
There was a problem hiding this comment.
Hmm this doesn't seem very elegant as we are adding an unnecessary check every iteration. I don't see a compelling reason to use enumerate here. We can keep the continue as I've said before to reduce indentation!
| Hi @Liwink I will be closing this PR as there does not seem to be changes that add compelling value. Thank you for your contribution regardless, and feel free to make more pull requests in the future! |
3 participants
