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

Add solution for BinarySearchTree #11

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

Conversation

Showndarya
Copy link

@Showndarya Showndarya commented Apr 8, 2018

Solution for the BinaryTree problem in Session8.
Answer for question in BinarySearchTree.java in comments.
Comments for each function also included.

I observed that the solution for LinkedList was present already. So, didn't add that.


public BinarySearchTree()
{
super(0);
Copy link
Owner

Choose a reason for hiding this comment

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

Why is the root randomly 0? It will be extremely unexpected if a user constructs an "empty" binary search tree, but minValue or maxValue will return 0.

// insert node
@Override
public boolean insert(int value) {
super.root = insert(super.root, value);
Copy link
Owner

Choose a reason for hiding this comment

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

What if the node already exists? This function always returns true regardless of whether the insert "fails" or succeeds.

private TreeNode insert(TreeNode root, int data)
{
// if current node is null, insert node with data
if (root == null) root = new TreeNode(data);
Copy link
Owner

Choose a reason for hiding this comment

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

If both your if and else blocks have a single line, this formatting is fine, like the way you have used below.

However, in this case, please adhere to the repo's coding style of

if (<condition>) {
}
else {
}

Curly braces are good. Always use them, even when they are not needed.

@Override
public boolean delete(int value) {
super.root = deleteRec(super.root, value);
if(flag==1) {flag=0;return false;}
Copy link
Owner

@havanagrawal havanagrawal Apr 12, 2018

Choose a reason for hiding this comment

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

Please format as

if (flag == 1) {
    flag = 0;
    return false;
}

Note the spaces


class BinarySearchTree extends BinaryTree {

private int flag = 0;
Copy link
Owner

Choose a reason for hiding this comment

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

Any reason why this is not a boolean? Also, why is it a member variable? What does "BinarySearchTree.flag" mean for the tree as a whole?

else return true;
}

TreeNode deleteRec(TreeNode root, int key)
Copy link
Owner

Choose a reason for hiding this comment

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

Please follow code style of opening the curly brace on the same line.

return root;
}

// delete node
Copy link
Owner

Choose a reason for hiding this comment

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

Is this comment really necessary? The function name is delete. Self-documenting code.

super(0);
}

// insert node
Copy link
Owner

Choose a reason for hiding this comment

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

Is this comment really necessary? The function name is insert. Self-documenting code.

}

// find the lowest value in the tree by reaching the leftmost leaf node
int minValue(TreeNode root)
Copy link
Owner

Choose a reason for hiding this comment

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

What happens when the tree is empty?

return minv;
}

// check if node exists
Copy link
Owner

Choose a reason for hiding this comment

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

Is this comment really necessary? The function name is exists. Self-documenting code.


public void inOrderTraversal() {
//TODO
inorderRec(root);
Copy link
Owner

Choose a reason for hiding this comment

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

I am assuming that inOrderRec() stands for inOrderRecursive. However, there is no way for me to know this, since it's not a convention. Please name it inOrderRecursive

}

// set left node
public void setLeft(TreeNode n)
Copy link
Owner

@havanagrawal havanagrawal Apr 12, 2018

Choose a reason for hiding this comment

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

This is really bad! We do not want setters on a TreeNode. Imagine what happens if I insert a TreeNode into a BinarySearchTree, and then set its value to be something else. Now you've violated the BinarySearchTree invariant.

Additionally, member variables without an explicit access modifier are default, and have package level access. Thus we don't really need getters either.

(In the ideal case, the TreeNode class would be immutable.)

// traverse to right sub-tree if value to be inserted is greater than value in current node
else if (key > root.data) root.right = deleteRec(root.right, key);

// if value is same as current value, delte it
Copy link
Owner

Choose a reason for hiding this comment

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

typo in comment, should be "delete"

if (root == null) root = new TreeNode(data);
else
{
// traverse to left sub-tree if value to be inserted is lesser than value in current node
Copy link
Owner

Choose a reason for hiding this comment

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

Your comment says "lesser than", but your code says "lesser than or equal to". Decide which one is incorrect.

@havanagrawal
Copy link
Owner

tl;dr
Hi @Showndarya , thank you for the PR. I appreciate your initiative, and it is an excellent attempt. Please see comments, and update your PR.

The long version
I see that you're a student in the C2C 2018 cohort, and once again, I admire your willingness to learn. Please do note that the above comments are something you would have learned before we reached this topic. We focus a lot on good design.

Some broad improvements you can make:

  1. Follow the code-style of the project. Look at how the code is structured in the rest of the repo, and try to mimic it. Before you contribute to any project, it is important that you follow their code-style, rather than your personal choice.
  2. Don't use global "flags".
  3. Don't use an IDE (at this stage). It inserts boilerplate for you, and more often than not, you don't need it (at least not for this course). You should think long and hard before you add a public function to a class.
  4. Contrary to popular belief, comments aren't really that great. Robert Martin (Uncle Bob) has a fantastic one-liner on this, (I paraphrase) "It is a mistake to think that a programmer who failed to express his thought in code will be able to do it in English." Avoid comments. Put them only where absolutely necessary, and then think if you can restructure your code to make the idea clearer.

If you want, go ahead and make the above changes, and I'll review again.

Cheers and happy learning 😄

@@ -0,0 +1,123 @@
// This class fails to compile, can you figure out why, and fix it?
// This is beacuse BinaryTree has a constructor which is not default. Hence, when we extend this class and try to call a no-argument constructor, it'll give a compile-time error. so, we call it by passing the parameters (to instantiate the root node) in super() called inside the constructor of BinarySearchTree
Copy link
Owner

Choose a reason for hiding this comment

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

Please use multiline comments for long comments.

/* line1
 * line2
 */

else return tn;
}

// the node value present in between a and b is the lca. Assuming, a<b
Copy link
Owner

Choose a reason for hiding this comment

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

Why assume that a < b? Why not ensure it?

{
if(root == null) {flag=1;return null;}

if(root.data>b && root.data>a) lca(root.left,a,b);
Copy link
Owner

Choose a reason for hiding this comment

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

If you have assumed that a < b, is it really necessary to check if root.data is greater than both of them?

if(root == null) {flag=1;return null;}

if(root.data>b && root.data>a) lca(root.left,a,b);
else if(root.data<b && root.data<a) lca(root.right,a,b);
Copy link
Owner

Choose a reason for hiding this comment

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

If you have assumed that a < b, is it really necessary to check if root.data is lesser than both of them?

@Showndarya
Copy link
Author

Showndarya commented Apr 17, 2018

Thank you for the review.

I have taken all the improvements and suggestions into consideration and have come up with a more refined solution.

Let me know if more changes are required.

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