Skip to content

feat(data-struct): binary search tree #106

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

Merged
merged 4 commits into from
Mar 9, 2023
Merged

feat(data-struct): binary search tree #106

merged 4 commits into from
Mar 9, 2023

Conversation

zFlxw
Copy link
Contributor

@zFlxw zFlxw commented Mar 5, 2023

This is an implementation of a binary search tree. If there are any inconveniences, please comment here, so I can improve the code.

Copy link
Contributor

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

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

Looks correct overall. However:

  • You should not expose implementation details like the TreeNode<T>. From the outside, your BinarySearchTree<T> should behave just like a SortedSet<T> implementation.
  • Your traversals should not take a TreeNode<T>; instead, they should all start at the root node. It'd be reasonable to implement them using either a closure (as you did with findMin/findMax) or methods of the TreeNode<T> helper class.

@zFlxw
Copy link
Contributor Author

zFlxw commented Mar 5, 2023

I resolved most of the things you mentioned directly in the code. However, I am a bit confused with the first point you mentioned as a comment. 😅

You should not expose implementation details like the TreeNode. From the outside, your BinarySearchTree should behave just like a SortedSet implementation.

Am I right with the assumption that this point was related to the search() function, where TreeNode<T> was returned? If so, it should be alright now. If you meant something else, please reach out to me again.

@zFlxw zFlxw requested review from appgurueu and removed request for raklaptudirm March 5, 2023 13:15
@appgurueu
Copy link
Contributor

Am I right with the assumption that this point was related to the search() function, where TreeNode<T> was returned? If so, it should be alright now. If you meant something else, please reach out to me again.

It was related to the traversals as well. You have addressed it properly.

@zFlxw zFlxw requested a review from appgurueu March 5, 2023 14:31
appgurueu
appgurueu previously approved these changes Mar 5, 2023
Copy link
Member

@raklaptudirm raklaptudirm left a comment

Choose a reason for hiding this comment

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

Tests are failing.

@raklaptudirm raklaptudirm merged commit 1ff8f20 into TheAlgorithms:master Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants