Skip to main content
2 of 2
Rewritten the time complexity with LaTeX. Omitted the base of the logarithm as CS literature do not (usually) specify bases.; added 20 characters in body
coderodde
  • 29.8k
  • 14
  • 77
  • 194

Much of the code looks good. Some comments could be clearer, and there are some correctness issues.

In is_leaf(), please strike this redundant remark:

    # Check if both left child and right child have no value

The code makes perfect sense as it stands. Code should say what is specific, and comments should speak in generalities that wouldn't be obvious from the code.

Similar remarks for is_branch(). Thank you for supplying docstrings, that is very nice. The one for is_branch() might mention the standard term of "interior node". The "return True if this node is a branch" phrasing is obvious already, given the (well chosen) function name. The code is redundant, as it is just an expansion of De Morgan's rule for the function above. Better to simply return not is_leaf() if you feel this function needs to be part of your public API.

In height(), shorten the docstring to just the parenthetical. The performance question would go better in a comment. Or add a comment that points out a degenerate tree created in sorted order will suffer \$\mathcal{O}(n)\$ performance, and the best case is \$\mathcal{O}(\log n)\$ for a balanced tree.

Where you defined __repr__(), twice, it would probably be better to stick to __str__(). Repr is supposed to show enough to recreate an object, while str just needs to be human readable.

The BST height() docstring is awkwardly phrased; it correctly uses "longest" but should pair that with "any". Better to simply copy the docstring of the function it calls.

The contains() docstring neglects the unbalanced case, e.g. cost to search a BST created from sorted input items. Similarly for insert().

This code doesn't make sense to me:

    if item < parent.data:
        parent.left = BinaryTreeNode(item)

Suppose the parent already had a left branch. What, we just nuke it, overwriting it? Similarly for right branch.

This code doesn't make sense to me:

    self.size += 1

In the equality case, we don't create any new nodes, yet we report tree growth?

    while queue.is_empty() == False:

This would be more naturally phrased as while not queue.is_empty():.

        if node.left != None:

Please phrase this as is not None. Similarly for right.

Thank you for including your tests. Consider passing multiple permutations to the code under test.

In BinaryTreeNodeTest, the asserts work fine. Consider using self.assertEqual() instead, as it produces a more informative result in the event of failure.

J_H
  • 35.2k
  • 3
  • 32
  • 129