What is wrong with my Preorder traversal?

Question

I am trying to solve this problem https://oj.leetcode.com/problems/binary-tree-preorder-traversal/ , i.e. preorder traversal with recursive slution.

EDIT: The whole code:

import java.util.ArrayList;
import java.util.List;

public class Solution {

    public class TreeNode {
         int val;
         TreeNode left;
         TreeNode right;
         TreeNode(int x) { val = x; }
    }
     static List<Integer> res = new ArrayList<Integer>();
     public List<Integer> preorderTraversal(TreeNode root) {
            if(root == null){
                return new ArrayList<Integer>();
            }
            res.add(root.val);
            if(root.left != null){
                res.add(preorderTraversal(root.left));
            }
            if(root.right != null){
                res.add(preorderTraversal(root.right));
            }
            return res;
      }
}

I have wrong answer because of the following:

Input:  {1,2}
Output:     [1,1,2]
Expected:   [1,2]

Can someone tell me how to fix this?

EDIT: I dont have main() method or unit tests. If you open the link that I posted you will see that this is online judging system.

This isn't your actual code. Post the code you're actually using. (copy-paste, don't retype) — user253751, Jul 26 '14 at 09:35
I assume there is more code, a main/unit test or something. Can we see that? — David Frye, Jul 26 '14 at 09:35
No. You don't show how this input is used and how the output is created. — laune, Jul 26 '14 at 09:36
Using a `static` mutable `List` and returning it or something else from an _instance_ method smells very odd. — Boris the Spider, Jul 26 '14 at 09:37
Your edit has done little more than combine what you showed us before into a single class and showed us your includes. Where is the main()/unit test? — David Frye, Jul 26 '14 at 09:39
This is an online judging system. This is all information that I have — sammy333, Jul 26 '14 at 09:39
Then I'm not really sure what to tell you. You've given us your tools, but you haven't told us how you've tried to use them. Based on what you've told me, I kind of have to assume that the missing code is correct, and since I don't see anything immediately and obviously wrong with the above... — David Frye, Jul 26 '14 at 09:41
@DavidFrye What more should I say? I gave you the link + my whole solution + the output that I receive from the system. If I've missed something, please tell me? — sammy333, Jul 26 '14 at 09:44
Yeah, I was so focused on what was missing that I didn't think about this carefully enough and missed an obvious problem. I apologize for being an idiot. — David Frye, Jul 26 '14 at 09:48

alfredaday · Accepted Answer · 2014-07-26T09:48:37.660

4

The problem is that within each recursive loop, you are adding the entire array again into your final result.

For example, given the following tree:

  1
 /
2

Your first iteration adds 1 into 'res' variable. The problem is when it gets to this line:

res.add(preorderTraversal(root.left));

Then it recursively calls itself for the left side. That preorderTraversal will return the res array, which will be [1,2]. Hence, when [1,2] gets added to res (which was [1], remember?), then you get [1,1,2].

Here's code that should work:

public List<Integer> preorderTraversal(TreeNode root) {
    List<Integer> result = new ArrayList<Integer>();

    if(root == null){
        return result;
    }

    result.add(root.val);
    if(root.left != null){
        result.addAll(preorderTraversal(root.left));
    }
    if(root.right != null){
        result.addAll(preorderTraversal(root.right));
    }

    return result;
}

edited Jul 26 '14 at 09:48

answered Jul 26 '14 at 09:43

alfredaday

2,048
18
14

Thanks a lot for the help! I also tried this but I got `Time Limit` – sammy333 Jul 26 '14 at 09:46
I will try to solve it iteratively, I guess it was the idea of having time limit (i.e. avoiding "obvious" recursive solution). But once again thanks for the explanation! – sammy333 Jul 26 '14 at 09:54
It's probably `addAll` that's slow. Try implementing a helper method like `public void preorderTraversal(List result, TreeNode root)` – fgb Jul 26 '14 at 09:55
Generating loads of `List` instances and adding them all together is probably costing you. You can pass the accumulator in the method signature - this is a common pattern in recursive code. – Boris the Spider Jul 26 '14 at 10:03
Yes, I agree with both fgb and Boris. The code I put above works but does not have the best performance. As they said, a helper method is the way to go. – alfredaday Jul 26 '14 at 10:05

Viorel Ghelbert · Answer 2 · 2014-07-26T10:16:02.697

For the left and right nodes, just call preorderTraversal recursively, and it will add the values to res. Calling add/addAll on res with the results of these recursive calls is wrong.

public class Solution {
     List<Integer> res = new ArrayList<Integer>();
     public List<Integer> preorderTraversal(TreeNode root) {
            if(root == null){
                return new ArrayList<Integer>();
            }
            res.add(root.val);
            if(root.left != null){
                preorderTraversal(root.left);
            }
            if(root.right != null){
                preorderTraversal(root.right);
            }
            return res;
      }
}

There's a little problem with this solution -- res retains the values from previous calls, so calling preorderTraversal more than once on the same instance may return incorrect results. Here's a solution that does not have this drawback:

public class Solution {
     public List<Integer> preorderTraversal(TreeNode root) {
            List<Integer> res = new ArrayList<Integer>();
            preorderTraversal(root, res);
            return res;
      }
      private void preorderTraversal(TreeNode node, List<Integer> res) {
          if (node != null) {
              res.add(node.val);
              preorderTraversal(node.left, res);
              preorderTraversal(node.right, res);
          }
      }
}

score 1 · Answer 3 · answered Jul 26 '14 at 09:43

1

try this method code:

public List<Integer> preorderTraversal(TreeNode root) {
        if(root == null){
            return new ArrayList<Integer>();
        }
        res.add(root.val);
        if(root.left != null){
            res = preorderTraversal(root.left);
        }
        if(root.right != null){
            res = preorderTraversal(root.right);
        }
        return res;
   }

btw, you were doing res.add in the recursion. You should rather do res= in it.

answered Jul 26 '14 at 09:43

Pat

2,223
4
19
25

Duh. I was so focused on what was missing that I missed this. Nice :) – David Frye Jul 26 '14 at 09:46
With this I have again Wrong answer with the same paramethers – sammy333 Jul 26 '14 at 09:48
What if root.left and root.right are both not null? – fgb Jul 26 '14 at 09:49

score 1 · Answer 4 · answered Jul 26 '14 at 10:16

A working solution :

import java.util.ArrayList;
import java.util.List;



class TreeNode 
{
      int val;
     TreeNode left;
     TreeNode right;
     TreeNode(int x) { val = x; }
 }

class Solution {
    public List<Integer> preorderTraversal(TreeNode root) 
    {
       ArrayList<Integer> res = new ArrayList<Integer>();

        if(root == null)
            return res;
        else res.add(root.val);

        if(root.left != null)
        {
            res.add(root.left.val);
            preorderTraversal(root.left);
        }

        if(root.right != null)
        {
            res.add(root.right.val);
            preorderTraversal(root.right);
        }


        return res;    
    }
}

public class TreeTest
{


    public static void main(String[] args)
    {

        Solution c = new Solution();

        TreeNode t1 = new TreeNode(1);
        TreeNode t2 = new TreeNode(2);
        TreeNode t3 = new TreeNode(3);

        //Link the nodes of the tree
        t1.left = t2;
        t1.right = t3;


        List<Integer> list = c.preorderTraversal(t1);

        System.out.println(list);
    }
}

score 0 · Answer 5 · answered Jul 26 '14 at 09:48

0

Since you have not posted the whole code how the input is done and output is produced it is difficult to tell. For preorder traversal it should be something like this.

 void traverse(TreeNode node){
   if(node != null){
      res.add(node.val);
      traverse(root.left);
      traverse(root.right);
    }
}

answered Jul 26 '14 at 09:48

Niru

732
5
9

1

Hi! I posted a link + all the code that I have + the output from the system. So I dont have anything else to post. + this is an online system so there is a method signature and I have stick to it. – sammy333 Jul 26 '14 at 09:50
I got it. Try the code I posted. Since your 'res' variable is static so I dont think there is any need to return the same. Either stick to your signature and use code posted by others or use my code iof you want to keep 'res' as static. – Niru Jul 26 '14 at 10:10

score 0 · Answer 6 · answered Aug 20 '17 at 21:08

public ArrayList<E> getPreOrderTraversal() {
    return getPreOrderTraversal(root);
}

private ArrayList<E> getPreOrderTraversal(Node tree) {
    ArrayList<E> list = new ArrayList<E>();

    if (tree != null) {
        list.add((E) tree.value);
        list.addAll(getPreOrderTraversal(tree.left));
        list.addAll(getPreOrderTraversal(tree.right));
    }

    return list;
}

score 0 · Answer 7 · edited May 02 '20 at 07:02

A working and LeetCode accepted solution:

class Solution {
    public List<Integer> preorderTraversal(TreeNode root) {
        List<Integer> list = new ArrayList<>();
        return preOrderTraversalInternal(root, list);
    }
    private List<Integer> preOrderTraversalInternal(TreeNode root, List<Integer> list){
    if(root == null) return list;
    list.add(root.val);
    if(root.left != null){
      preOrderTraversalInternal(root.left, list);
    }
    if(root.right != null){
      preOrderTraversalInternal(root.right, list);
    }
    return list;
  }
}

score 0 · Answer 8 · answered Apr 24 '22 at 08:49

The problem is that within each recursive loop, you are adding the entire array again into your final result. You need to add left or right nodes only one time while traversing but in your code whole arr is getting added to the new res array. This result will contain repetitive nodes.

For these issues, please use Print statements after every recursive call to debug the problem.

Solution - Use addAll method: res.addAll(node.left);

What is wrong with my Preorder traversal?

8 Answers8

Linked