3
\$\begingroup\$

Ok, code reviewers, I want you to pick my code apart and give me some feedback on how I could make it better or more simple. ( Generics would be added a bit later).

public class CreateABinaryTree {
    private TreeNode root;

    public CreateABinaryTree() {
    }

    /**
     * Constructs a binary tree in order of elements in an array.
     * After the number of nodes in the level have maxed, the next 
     * element in the array would be a child of leftmost node.
     * 
     * http://codereview.stackexchange.com/questions/31334/least-common-ancestor-for-binary-search-tree/31394?noredirect=1#comment51044_31394
     */
    public CreateABinaryTree(List<Integer> items) {
        this();
        create(items);
    }

    private static class TreeNode {
        TreeNode left;
        int element;
        TreeNode right;

        TreeNode(TreeNode left, int element, TreeNode right) {
            this.left = left;
            this.element = element;
            this.right = right;
        }
    }

    private void create (List<Integer> items) {   
        root = new TreeNode(null, items.get(0), null);

        final Queue<TreeNode> queue = new LinkedList<TreeNode>();
        queue.add(root);

        final int half = items.size() / 2;

        for (int i = 0; i < half; i++) {
            if (items.get(i) != null) {
                final TreeNode current = queue.poll();
                final int left = 2 * i + 1;
                final int right = 2 * i + 2;

                if (items.get(left) != null) {
                    current.left = new TreeNode(null, items.get(left), null);
                    queue.add(current.left);
                }
                if (right < items.size() && items.get(right) != null) {
                    current.right = new TreeNode(null, items.get(right), null);
                    queue.add(current.right);
                }
            }
        }
    }
}
\$\endgroup\$
4
\$\begingroup\$

I only see one really questionable thing in your code. Why do you call this() in your constructor? Your parameter-less constructor does absolutely nothing, so this line is irrelevant. If anything, I would actually even remove that constructor entirely so that your API can only be used with your meaningful and useful one.

Also, as a side note, the following line opens you up to a NullPointerException since you never check to see if items is null or if it has any elements.

root = new TreeNode(null, items.get(0), null);
\$\endgroup\$
  • \$\begingroup\$ I agree with your point, in this code i should take off the parameterless constructor. But to answer both your concerns. \$\endgroup\$ – JavaDeveloper Sep 30 '13 at 1:13
  • \$\begingroup\$ stackoverflow.com/questions/19082841/… - should answer why null ptr check was not done. Plz correct me if i am still wrong \$\endgroup\$ – JavaDeveloper Sep 30 '13 at 1:14
  • \$\begingroup\$ stackoverflow.com/questions/19081965/… - should answer why this() is included. \$\endgroup\$ – JavaDeveloper Sep 30 '13 at 1:14
  • 1
    \$\begingroup\$ @JavaDeveloper I don't get what you mean by the answer you linked. It seems to support performing a null check of the parameter before using it. "What I would do in this case, would either check at the beginning if the param is null, and then return false (so it doesn't break the runtime flow - Objective-C style), OR assertIsNotNull at the beginning of the method, and specify it in the javadoc (e.g. "the thing must not be null, yo")." \$\endgroup\$ – asteri Sep 30 '13 at 1:16
  • \$\begingroup\$ @JavaDeveloper Yeah, I get why you would have this() if the parameter-less constructor actually did anything. :) It just doesn't, and so it's pointless. That's all. \$\endgroup\$ – asteri Sep 30 '13 at 1:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.