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

Water@Hanh Solo - Tree practices #34

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

seattlefurby17
Copy link

No description provided.

Copy link

@CheezItMan CheezItMan left a comment

Choose a reason for hiding this comment

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

Thanks for getting this in Hanh. It's clear you found this project challenging. I left some comments, I hope you find helpful. Let me know what questions you have and I'm happy to work with you on this one.

lib/tree.rb Outdated
Comment on lines 33 to 37
if key < current_node.key
current_node = current_node.left
else
current_node = current_node.right
end

Choose a reason for hiding this comment

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

What if key and current_node.key are equal?

lib/tree.rb Outdated
# Time Complexity: O(n)
# Space Complexity: O(1)
def inorder(current_node = @root, answer = [])
return [] if current_node.nil?

Choose a reason for hiding this comment

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

Suggested change
return [] if current_node.nil?
return answer if current_node.nil?

lib/tree.rb Outdated
@root = new_node
else
current_node = @root
previouse_node = @root

Choose a reason for hiding this comment

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

Spelling?

lib/tree.rb Outdated
def find(key)
raise NotImplementedError
def find_helper(key, current_node)

Choose a reason for hiding this comment

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

Making an inner method like this in Ruby is discouraged and messy. It leads to tree.rb:54: warning: method redefined; discarding old find_helper

lib/tree.rb Outdated
Comment on lines 23 to 47
new_node = TreeNode.new(key, value)

if @root == nil
@root = new_node
else
current_node = @root
previouse_node = @root

while current_node != nil
previouse_node = current_node
if key < current_node.key
current_node = current_node.left
else
current_node = current_node.right
end

if key < previouse_node.key
previouse_node.left = new_node
else
previouse_node.right = new_node
end
end

return new_node
end

Choose a reason for hiding this comment

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

Suggested change
new_node = TreeNode.new(key, value)
if @root == nil
@root = new_node
else
current_node = @root
previouse_node = @root
while current_node != nil
previouse_node = current_node
if key < current_node.key
current_node = current_node.left
else
current_node = current_node.right
end
if key < previouse_node.key
previouse_node.left = new_node
else
previouse_node.right = new_node
end
end
return new_node
end
new_node = TreeNode.new(key, value)
if @root == nil
@root = new_node
else
current_node = @root
previous_node = @root
until current_node.nil?
previous_node = current_node
if key < current_node.key
current_node = current_node.left
else
current_node = current_node.right
end
end
if key < previous_node.key
previous_node.left = new_node
else
previous_node.right = new_node
end
return new_node
end
end

lib/tree.rb Outdated
if current_node
inorder(current_node.left, answer)
answer.push({key: current_node.key, value: current_node.value})
inorder(current_node.right)

Choose a reason for hiding this comment

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

Suggested change
inorder(current_node.right)
inorder(current_node.right, answer)

lib/tree.rb Outdated
Comment on lines 90 to 91
preorder(current_node.right, answer)
preorder(current_node.left)

Choose a reason for hiding this comment

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

These look wrong.

Comment on lines +109 to +111
# Time Complexity: O(n)
# Space Complexity: O(n)
def height(current_node = @root)

Choose a reason for hiding this comment

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

👍 However your space complexity is O(n) only if the tree is unbalanced and O(log n) if it is balanced.

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.

2 participants