Skip to content
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

dibs: implement binary-search-tree #388

Closed

Conversation

laurauzcategui
Copy link

I will implement binary-search-tree in : Binary Search Tree - X-Common

@kytrinyx
Copy link
Member

@laurauzcategui
Copy link
Author

Hi @behrtam @kytrinyx,

Could you do quick review ?

Cheers,

@laurauzcategui
Copy link
Author

laurauzcategui commented Oct 31, 2016

Also @behrtam ,

I think as I rebased the upstream, I brought your changes from there to my branch. I hope this is not a problem. Just wanted to update my branch with latest changes from the upstream


class BST(object):
def __init__(self, root):
self.root = Node(root)
Copy link
Contributor

Choose a reason for hiding this comment

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

Requiring an initial value for the BST seems like a very odd design choice...

def __init__(self, root):
self.root = Node(root)

def insert(self, new_val):
Copy link
Contributor

Choose a reason for hiding this comment

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

You already wrote this function below. self.pre_insert(self.root, new_val)

else:
self.pre_insert(node.left, val)

def search(self, find_val):
Copy link
Contributor

Choose a reason for hiding this comment

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

This search function is probably a little easier to read if you do something like this:

cur = self.root
while cur:
    if find_val == cur.value:
        return True
    cur = cur.left if find_val < cur.value else cur.right
return False

@arcuru
Copy link
Contributor

arcuru commented Nov 1, 2016

@laurauzcategui You can clean up the extra git commits by resetting instead of rebasing. It's explained here - https://github.com/exercism/x-common/blob/master/CONTRIBUTING.md#resetting-master

You also need to add a few more tests. Right now you only ever test inserting things into the left nodes. You should at least add some tests to try to insert a more complex tree and values that are already in the tree.

@laurauzcategui
Copy link
Author

Thanks @patricksjackson will check it over the weekend 👍

@stale
Copy link

stale bot commented May 6, 2017

This issue has been automatically marked as `on hiatus because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@kytrinyx
Copy link
Member

@laurauzcategui Would you have a chance to finish this one up? We'd love to include it on the site.

@stale stale bot removed the on hiatus label May 19, 2017
@laurauzcategui
Copy link
Author

Hi @kytrinyx yes, sure thing. I'll be able to take a look probably next week and over the weekend.

Cheers,

@stale stale bot added the on hiatus label Jul 18, 2017
@stale
Copy link

stale bot commented Jul 18, 2017

This issue has been automatically marked as on hiatus because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot closed this Jul 25, 2017
@devkabiir
Copy link

@kytrinyx Looks like issue was automatically closed, but I have a BST implementation ready in python2 I would like to include this exercise. Can you assign this issue to me and add hacktoberfest label too?

@kytrinyx kytrinyx reopened this Oct 3, 2017
@stale stale bot removed the on hiatus label Oct 3, 2017
@kytrinyx kytrinyx closed this Oct 3, 2017
@kytrinyx
Copy link
Member

kytrinyx commented Oct 3, 2017

@kabiir that's great. Please submit your pull request. It doesn't need to have an open issue or a hacktoberfest label in order to be counted towards your Hacktoberfest metrics.

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.

5 participants