Skip to content

Done Tree 2#1579

Open
pranjay01 wants to merge 1 commit into
super30admin:masterfrom
pranjay01:master
Open

Done Tree 2#1579
pranjay01 wants to merge 1 commit into
super30admin:masterfrom
pranjay01:master

Conversation

@pranjay01
Copy link
Copy Markdown

No description provided.

@super30admin
Copy link
Copy Markdown
Owner

Construct Binary Tree from Inorder and Postorder Traversal (Problem1.py)

Strengths:

  • You have identified the recursive approach and the need for a map to store inorder indices, which is correct.
  • You are decrementing the rootIndex correctly when building the root node.

Areas for improvement:

  1. Mapping Construction: The way you build the inOrderIndexMap is incorrect. You should map each value in the inorder array to its index. Instead of:

    for i in range(0, len(inorder)):
         inOrderIndexMap[i] = inOrderIndexMap[i]   # This is wrong

    You should do:

    for idx, val in enumerate(inorder):
         inOrderIndexMap[val] = idx

    This will create a mapping from node value to its index in the inorder array.

  2. Type Hints: You are using List[int] and Optional[TreeNode] which require imports from the typing module. You should add:

    from typing import List, Optional

    Alternatively, you can use list and TreeNode | None if using Python 3.10+.

  3. Variable Naming: Use clear and consistent variable names. For example, inOrderLeftIndex and inorderRightIndex should be consistent (e.g., both camelCase or both snake_case). Also, inOrderIndexMap is a good name, but it should be built correctly.

  4. Recursive Helper: The helper function should be defined to take the necessary parameters. You are passing inOrderIndexMap and postorder correctly, but the map must be built correctly first.

  5. Initialization: The rootIndex is initialized as a class variable. This might cause issues if the class is reused. It's better to use an instance variable (by setting self.rootIndex in buildTree). However, since the problem expects a single call, it's acceptable. But note that if multiple calls are made, the rootIndex would not be reset. You can avoid this by making rootIndex local to the method and using a recursive helper that uses an index passed by reference (e.g., using a list or a mutable object). Alternatively, you can use a nested function with nonlocal variable. Here's a better approach:

    Instead of using a class variable for rootIndex, you can use a mutable object (like a list) to store the index and update it in the helper. Or you can reverse the postorder and use it as a stack. However, the reference solution uses a class variable, so it's acceptable if you reset it for each call.

    But in your current code, you set self.rootIndex = len(postorder)-1 in buildTree, which is correct for the first call. However, if the same instance is used again, it might have the wrong value. To avoid this, you can make rootIndex a local variable and pass it by reference (using a list) or use a nested function.

    Example using nested function:

    def buildTree(self, inorder: List[int], postorder: List[int]) -> Optional[TreeNode]:
         inOrderIndexMap = {}
         for idx, val in enumerate(inorder):
             inOrderIndexMap[val] = idx
         rootIndex = len(postorder) - 1
    
         def helper(left, right):
             if left > right:
                 return None
             rootVal = postorder[rootIndex]
             rootIndex -= 1
             rootIdxInorder = inOrderIndexMap[rootVal]
             rootNode = TreeNode(rootVal)
             rootNode.right = helper(rootIdxInorder+1, right)
             rootNode.left = helper(left, rootIdxInorder-1)
             return rootNode
    
         return helper(0, len(inorder)-1)

    However, note that in Python, you cannot modify outer nonlocal variables in a nested function without nonlocal keyword. So you should declare nonlocal rootIndex inside the nested function.

    Alternatively, you can use an instance variable as you did, but ensure it is reset for each call.

  6. Order of Recursion: In postorder, the root is at the end, and when we build the tree, we process the right subtree first because in postorder the right subtree is just before the root. Your order (right then left) is correct.

  7. Edge Cases: Your code handles the base case (if left > right) correctly.

Corrected code:

from typing import List, Optional

class TreeNode:
    def __init__(self, val=0, left=None, right=None):
        self.val = val
        self.left = left
        self.right = right

class Solution:
    def buildTree(self, inorder: List[int], postorder: List[int]) -> Optional[TreeNode]:
        inOrderIndexMap = {}
        for idx, val in enumerate(inorder):
            inOrderIndexMap[val] = idx
        self.rootIndex = len(postorder) - 1

        def helper(left, right):
            if left > right:
                return None
            rootVal = postorder[self.rootIndex]
            self.rootIndex -= 1
            rootIdxInorder = inOrderIndexMap[rootVal]
            rootNode = TreeNode(rootVal)
            rootNode.right = helper(rootIdxInorder+1, right)
            rootNode.left = helper(left, rootIdxInorder-1)
            return rootNode

        return helper(0, len(inorder)-1)

But note: using self.rootIndex might cause issues if multiple calls are made. To avoid this, you can use a mutable list for the index:

def buildTree(self, inorder: List[int], postorder: List[int]) -> Optional[TreeNode]:
    inOrderIndexMap = {val: idx for idx, val in enumerate(inorder)}
    rootIndex = [len(postorder)-1]

    def helper(left, right):
        if left > right:
            return None
        rootVal = postorder[rootIndex[0]]
        rootIndex[0] -= 1
        rootIdxInorder = inOrderIndexMap[rootVal]
        root = TreeNode(rootVal)
        root.right = helper(rootIdxInorder+1, right)
        root.left = helper(left, rootIdxInorder-1)
        return root

    return helper(0, len(inorder)-1)

VERDICT: NEEDS_IMPROVEMENT


Root to Leaf Sum (Problem2.py)

Your approach to use recursion is good, but there are several issues to address:

  • Building the Number: You need to build the number by multiplying the current number by 10 and then adding the node's value. In your helper, you are adding the root.val to currentNumber without first multiplying by 10 for the children. Actually, when you go to a child, you should pass currentNumber * 10 + root.val (but note: you are doing the addition in the parent call?).

  • Result Handling: You are passing result as a parameter and returning it. This is not necessary and makes the code harder to follow. Instead, you can have result as an instance variable (like in the reference solution) or have the helper function return the total sum for the subtree. Another common approach is to have the helper return the sum of numbers from the current node to leaves, but you need to accumulate the result.

  • Initial Call: In the initial call, you are doing currentNumber*10 which is 010=0. Then you add root.val to it. But for the children, you should pass the updated currentNumber (which is currentNumber10 + root.val) and then for the children, they will multiply by 10 again and add their value. However, in your helper, you are doing:
    currentNumber = currentNumber + root.val # This is wrong because currentNumber already has the value from parent multiplied by 10? Actually, you are passing currentNumber*10 from the parent, but then you add the value. This should be done in reverse.

Let me clarify the correct approach:
You should start with currentNumber = 0.
When you visit a node, update currentNumber = currentNumber * 10 + node.val.
Then, if it's a leaf, add currentNumber to the total.
Otherwise, recursively process left and right with the updated currentNumber.

In your code, you are doing:
currentNumber = currentNumber + root.val # This doesn't multiply by 10 to shift digits.

Also, you are passing currentNumber*10 to the helper, which is correct for the children? But then you add the root.val in the children? This is inconsistent.

Actually, in your helper, you are receiving currentNumber which is already multiplied by 10 by the parent. Then you add the root.val. But this means that for the root, the initial call passes 0*10=0, then adds root.val -> correct. For a child, the parent passes (currentNumber * 10) which is correct, but then the child adds its value without multiplying? So it's correct only if the parent multiplies by 10. However, the issue is that the parent's currentNumber already includes the parent's value? Wait, let's trace:

For root: currentNumber = 0 (passed as 010=0) + root.val -> becomes root.val.
Then for left child: we pass currentNumber
10 = (root.val)10.
Then in left child: currentNumber = (root.val
10) + left.val -> correct.

So actually, the way you are updating currentNumber in the helper is correct: first the parent multiplies by 10 and passes, then the child adds its value. But note: in the helper, you are not using the passed currentNumber correctly? The parameter currentNumber in the helper is actually the value passed from the parent (which is the parent's currentNumber * 10). Then you do:
currentNumber = currentNumber + root.val
This is correct because currentNumber from parent is already shifted.

But wait: the initial call is self.helper(root, currentNumber*10, result). At start, currentNumber=0, so it passes 0. Then in helper: currentNumber = 0 + root.val -> correct.

So the logic for building the number is correct. However, the handling of result is problematic.

The main issue is that you are passing result as a parameter and returning it. This can work if you are careful, but in your code, you are doing:

if root.left: result = self.helper(root.left, currentNumber10, result)
if root.right: result = self.helper(root.right, currentNumber
10, result)

But note: when you call for left and right, you are passing the same result value? And then you update result from the left call and use that updated result for the right call? This is correct for accumulation.

However, after processing children, you check if it's a leaf and then add currentNumber to result. But wait: if it is a leaf, you add currentNumber to result and return. But if it's not a leaf, you don't add anything? Actually, you should only add at leaves. So the code does:
For a non-leaf: it processes children and returns the result from the children. It does not add anything for itself. This is correct.

But for a leaf: it adds currentNumber to result and returns. So the leaf returns the updated result.

So the accumulation might be correct. However, there is a problem: the initial call passes result=0 to the helper. But the helper returns the result. So the top-level call should be:

return self.helper(root, 0, 0)

But in your code, you are doing:
return self.helper(root, currentNumber*10, result)

Here, currentNumber is 0 at start, so it passes (0*10=0) and result=0. So it's equivalent to self.helper(root,0,0).

Now, let's test with a simple tree: root=TreeNode(1), and no children.

In the helper: currentNumber = 0 + 1 = 1.
Then since no left and no right, it is a leaf -> result += 1 -> result=1, and returns 1.

So it returns 1. Correct.

Now with root=1, left=2.
Root call: helper(root,0,0)
currentNumber = 0+1=1.
Then process left: call helper(left, 1*10=10, 0)
In left: currentNumber = 10+2=12.
It is a leaf? Yes. So result = 0+12=12. Returns 12.
Then result becomes 12 from left call.
Then process right? No.
Then it returns 12.
So returns 12. Correct.

Now with root=1, right=3.
Similarly, it should return 13.

So the code might actually be correct for these cases. But wait: what if there are two children?
Example: root=1, left=2, right=3.
Root call: currentNumber=1.
Process left: call helper(left,10,0) -> returns 12.
Then result=12.
Then process right: call helper(right,10,12) ->
In right: currentNumber=10+3=13.
It is a leaf -> result = 12+13=25. Returns 25.
So returns 25. Correct.

So the code seems to work. However, there is a subtle issue: the parameter result in the helper is being used to accumulate the sum from the subtrees. This is correct because when you call for the left subtree, you pass the current result (which might be 0) and it returns the result including the left subtree. Then you use that result when calling the right subtree. So the accumulation is correct.

But note: the code is inefficient because it passes result as a parameter and returns it. This is not typical and

VERDICT: NEEDS_IMPROVEMENT

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants