2
\$\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.

public class PrintAllPath {

    private TreeNode root;

    private class TreeNode {
        TreeNode left;
        int item;
        TreeNode right;


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

    public void create (Integer[] arr) {
        if (arr == null)  {
            throw new NullPointerException("The input array is null.");
        }

        root = new TreeNode(null, null, arr[0]);

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

        final int half = arr.length / 2;

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

                if (arr[left] != null) {
                    current.left = new TreeNode(null, null, arr[left]);
                    queue.add(current.left);
                }
                if (right < arr.length && arr[right] != null) {
                    current.right = new TreeNode(null, null, arr[right]);
                    queue.add(current.right);
                }
            }
        }
    }

    public void printPath () {
        doPrint(root, new ArrayList<TreeNode>());
    }


    private void doPrint(TreeNode node, List<TreeNode> path) {
        if (node == null) return;

        path.add(node);

        if (node.left == null && node.right == null) {
            System.out.println("Path from root: " + root.item + " to leaf: " + node.item + " - ");
            for (TreeNode treeNode : path) {
                System.out.print(treeNode.item + " ");
            }
            System.out.println();
        }
        doPrint(node.left , path);
        doPrint(node.right, path);

        path.remove(path.size() - 1);
    }
| improve this question | | | | |
\$\endgroup\$
1
\$\begingroup\$

It looks good overall.

I see you have taken my previous suggestion to rearrange the TreeNode constructor arguments. Now you have to readjust your calling convention accordingly.

Your create() method acts like a constructor, so you should just make it a constructor. It desperately needs a JavaDoc comment explaining how the input array is to be interpreted.

In create(), I would not bother to check for arr == null. You'll get a NullPointerException anyway when you fetch arr[0]. It's only worth the explicit check if such a check would make debugging substantially easier, i.e. when an improper null would get stored or propagated but not dereferenced until much later. Even then, I'd throw an IllegalArgumentException rather than NullPointerException to emphasize that the caller is at fault.

In doPrint(), path.remove(path.size() - 1) should be just path.removeLast().

| improve this answer | | | | |
\$\endgroup\$
  • \$\begingroup\$ Accepted the rest except - Effective Java mentions to validate each method input, throw exception sooner than later, and use NPE rather than IllegalArgE. I would be receptive if you still have some points to add. Thanks again. \$\endgroup\$ – JavaDeveloper Sep 29 '13 at 16:41
  • \$\begingroup\$ Cross tallied with SO, you were correct. Thanks for all pointers \$\endgroup\$ – JavaDeveloper Sep 29 '13 at 20:34

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.