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) {

    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>();

        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);
                if (right < items.size() && items.get(right) != null) {
                    current.right = new TreeNode(null, items.get(right), null);

1 Answer 1


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);
  • \$\begingroup\$ I agree with your point, in this code i should take off the parameterless constructor. But to answer both your concerns. \$\endgroup\$ Sep 30, 2013 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\$ Sep 30, 2013 at 1:14
  • \$\begingroup\$ stackoverflow.com/questions/19081965/… - should answer why this() is included. \$\endgroup\$ Sep 30, 2013 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, 2013 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, 2013 at 1:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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