Skip to main content
deleted 3 characters in body
Source Link
janos
  • 113.1k
  • 15
  • 154
  • 396

The implementation violates a requirement of a Binary Search Tree: it will add duplicate elements. You can fixLet's expose this bug by adding a unit test:

@Test
public void testNoDups() {
    CreateBinarySearchTree tree = new CreateBinarySearchTree();
    tree.add(4, 4);
    tree.add(4);
    assertEquals("[null,4,null]", tree.toString());
}

Easy enough to fix, by adding an else in the main loop:

    while (true) {
        if (item < node.item) {
            if (node.left == null) {
                node.left = new TreeNode(null, item, null);
                break;
            }
            node = node.left;
        } else if (item > node.item) {
            if (node.right == null) {
                node.right = new TreeNode(null, item, null);
                break;
            }
            node = node.right;
        } else {
            break;
        }
    }

And while at it let's add a unit test for this too:

@Test
public void testNoDups() {
    CreateBinarySearchTree tree = new CreateBinarySearchTree();
    tree.add(4, 4);
    tree.add(4);
    assertEquals("[null,4,null]", tree.toString());
}

Instead of a while loop, it'sit would be more elegant to implement adding a node using recursion:

The constructor that takes an array calls this(), but since thethat other constructor does nothing, this is pointless. The variable names a and i are poor, it would be better to rename athem to arr or items, and i to item, respectively.

As others have pointed out, it would be bettermake sense to move the adding logic outside of the constructor, so that callers could benefit from it too. In fact, how about removing all thosethe constructors, and addingusing this new method instead to add elements:

The implementation violates a requirement of a Binary Search Tree: it will add duplicate elements. You can fix this by adding an else in the main loop:

    while (true) {
        if (item < node.item) {
            if (node.left == null) {
                node.left = new TreeNode(null, item, null);
                break;
            }
            node = node.left;
        } else if (item > node.item) {
            if (node.right == null) {
                node.right = new TreeNode(null, item, null);
                break;
            }
            node = node.right;
        } else {
            break;
        }
    }

And while at it let's add a unit test for this too:

@Test
public void testNoDups() {
    CreateBinarySearchTree tree = new CreateBinarySearchTree();
    tree.add(4, 4);
    tree.add(4);
    assertEquals("[null,4,null]", tree.toString());
}

Instead of a while loop, it's more elegant to implement adding a node using recursion:

The constructor that takes an array calls this(), but since the other constructor does nothing, this is pointless. The variable names are poor, it would be better to rename a to arr or items, and i to item.

As others have pointed out, it would be better to move the adding logic outside of the constructor, so that callers could benefit from it too. In fact, how about removing all those constructors, and adding this method instead:

The implementation violates a requirement of a Binary Search Tree: it will add duplicate elements. Let's expose this bug by adding a unit test:

@Test
public void testNoDups() {
    CreateBinarySearchTree tree = new CreateBinarySearchTree();
    tree.add(4, 4);
    tree.add(4);
    assertEquals("[null,4,null]", tree.toString());
}

Easy enough to fix, by adding an else in the main loop:

    while (true) {
        if (item < node.item) {
            if (node.left == null) {
                node.left = new TreeNode(null, item, null);
                break;
            }
            node = node.left;
        } else if (item > node.item) {
            if (node.right == null) {
                node.right = new TreeNode(null, item, null);
                break;
            }
            node = node.right;
        } else {
            break;
        }
    }

Instead of a while loop, it would be more elegant to implement adding a node using recursion:

The constructor that takes an array calls this(), but since that other constructor does nothing, this is pointless. The variable names a and i are poor, it would be better to rename them to arr or items, and item, respectively.

As others have pointed out, it would make sense to move the adding logic outside of the constructor. In fact, how about removing all the constructors, and using this new method instead to add elements:

added 1 character in body
Source Link
janos
  • 113.1k
  • 15
  • 154
  • 396

This class is totoo basic to be useful. You can only add nodes to it, but you don't provide a way to access those nodes. As such this is something incomplete.

This class is to basic to be useful. You can only add nodes to it, but you don't provide a way to access those nodes. As such this is something incomplete.

This class is too basic to be useful. You can only add nodes to it, but you don't provide a way to access those nodes. As such this is something incomplete.

added 621 characters in body
Source Link
janos
  • 113.1k
  • 15
  • 154
  • 396

Recursive add

Instead of a while loop, it's more elegant to implement adding a node using recursion:

public void add(int item) {
    if (root == null) {
        root = new TreeNode(null, item, null);
    } else {
        add(root, item);
    }
}

public void add(TreeNode node, int item) {
    if (item < node.item) {
        if (node.left == null) {
            node.left = new TreeNode(null, item, null);
        } else {
            add(node.left, item);
        }
    } else if (item > node.item) {
        if (node.right == null) {
            node.right = new TreeNode(null, item, null);
        } else {
            add(node.right, item);
        }
    }
}

Minor things

Minor things

Recursive add

Instead of a while loop, it's more elegant to implement adding a node using recursion:

public void add(int item) {
    if (root == null) {
        root = new TreeNode(null, item, null);
    } else {
        add(root, item);
    }
}

public void add(TreeNode node, int item) {
    if (item < node.item) {
        if (node.left == null) {
            node.left = new TreeNode(null, item, null);
        } else {
            add(node.left, item);
        }
    } else if (item > node.item) {
        if (node.right == null) {
            node.right = new TreeNode(null, item, null);
        } else {
            add(node.right, item);
        }
    }
}

Minor things

Source Link
janos
  • 113.1k
  • 15
  • 154
  • 396
Loading