Skip to content

complete Trees-1#1726

Open
paridhimalviya wants to merge 1 commit intosuper30admin:masterfrom
paridhimalviya:master
Open

complete Trees-1#1726
paridhimalviya wants to merge 1 commit intosuper30admin:masterfrom
paridhimalviya:master

Conversation

@super30admin
Copy link
Copy Markdown
Owner

Strengths:

  • You have implemented multiple approaches (in-order traversal with a global flag, in-order with boolean return, and range-based) which shows a good understanding of the problem.
  • The code is well-commented and structured.
  • You have considered time and space complexity in your comments.

Areas for Improvement:

  1. The range-based method in BinarySearchTree2 has a critical error in the condition. It should be:

    if root!.val <= min || root!.val >= max {
        return false
    }

    Note: The condition should use || because if the value is not within the range (i.e., it is less than or equal to min OR greater than or equal to max), it is invalid.

  2. The inOrderWithoutPreviousVariable method is incorrect. It only checks the immediate children, but for a BST, all nodes in the left subtree must be less than the current node, and all in the right must be greater. This method would pass an invalid tree like:

         5
        / \
       3   6
      / \
     2   4
    

    But it would fail for:

         5
        / \
       3   7
      / \
     2   6   // 6 is greater than 5? No, but it is in the left subtree of 5.
    

    Actually, in this example, 6 is in the left subtree of 5 but is greater than 5, which is invalid. However, the method only checks immediate children, so it would not catch this because 6 is not an immediate child of 5. Therefore, this method is not sufficient. You should remove this method or correct it.

  3. The BinarySearchTree2 class is not integrated with the main solution. It seems like you are exploring different approaches, which is good, but for the purpose of this problem, you should focus on one correct method.

  4. In the inorderToCheckValidBST method, you have a condition if (!flag) { return } to break early. This is a good optimization.

  5. The code contains some unrelated code (like ConstructBinaryTreeFromPreorder), which might be from other practice problems. For clarity, it's better to separate solutions for different problems.

  6. The TreeNode class is defined correctly, but it should be outside the BinarySearchTree class to avoid nesting.

Recommendation:

  • Use the in-order traversal with a prev pointer (as in inorderToCheckValidBSTBooleanBased) because it is efficient and correct.
  • Alternatively, fix the range-based method and use that.
  • Remove incorrect methods like inOrderWithoutPreviousVariable.

@super30admin
Copy link
Copy Markdown
Owner

Strengths:

  • You have implemented multiple approaches (in-order traversal and range-based) which shows a good understanding of the problem.
  • The in-order traversal with a prev node is correctly implemented and efficient.
  • The code is well-commented and structured.

Areas for Improvement:

  • The inOrderWithoutPreviousVariable method is incorrect because it only checks the immediate children. For example, in a tree where a left child is less than its parent but greater than the grandparent, it would fail. You should remove this method.
  • It's better to avoid global variables (like prev and flag) in a class if possible. The reference solution uses a helper method with instance variables, which is cleaner. Alternatively, you can use a nested function with inout parameters in Swift to avoid globals.
  • The file contains unrelated code (like BinaryTreeFromInorderPreorder). It's good practice to separate different problems into different files.
  • For the range-based method, the condition if (root!.val >= max && root!.val <= min) is incorrect. It should be if root!.val <= min || root!.val >= max. Also, note that the parameters min and max might be confusing: typically, we check that the node's value is within (min, max). Please correct this.
  • Consider using optionals for prev to avoid force-unwrapping (!) and make the code safer.

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