HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

Binary tree in Python

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
binarypythontree

Problem

As a Python newbie, I have coded this binary tree as best as I could to follow Python
idioms. However, I know it is still very verbose and not up to standard. I would like Python experts to tear this apart and identify its weaknesses.

class TreeNode(object):
  """ applicable to all tree nodes including root node """
  def __init__(self, value, left=None, right=None,root=False):
    self.value = value 
    TreeNode.check(left)
    TreeNode.check(right)
    self.left = left
    self.right = right
    self.root = root

  def inorder(self):
    s = list()
    if not self.left:
      s.append('')  
    else:
      s.append(self.left.inorder())

    s.append(str(self.value))

    if not self.right:
      s.append('')
    else:
      s.append(self.right.inorder())

    return ' '.join(s).strip()

  # returns True if success, False otherwise  
  def insert( self, new_value ):
    # TODO ?? what happens if we are inserting strings
    if not new_value and new_value != 0:
      return False

    if new_value == self.value:
      return False

    # need to find the right location in terms of binary tree ordering
    if new_value  self.value:
      if not self.right:
        self.right = TreeNode(new_value)
        return True
      return self.right.insert( new_value )

    return False

  @staticmethod    
  def check(node):
    if not node:
      return
    if not isinstance(node, TreeNode):
      raise TypeError('only instances of TreeNode are allowed')      

  def __repr__(self):
    return '(' + repr(self.left) + ',' + \
      str(self.value) + ',' + repr(self.right) + ')'

Solution

Firstly, the official python style guide recommends 4-space indentation, and you are using 2. (No big deal but following the style guide is helpful)

class TreeNode(object):
  """ applicable to all tree nodes including root node """
  def __init__(self, value, left=None, right=None,root=False):


Your code never passes a left/right parameter. There are also doesn't seem to be a good reason why you would pass left/right parameters in the construction of the tree. I assume you would use this class by constructing an empty tree and inserting values into it, not by composing trees. As a result, I suggest eliminating left/right.

Also, you have a root parameter which is never used. It looks like something that you should eliminate.

Additionally, you cannot create an empty tree with this method.

self.value = value 
    TreeNode.check(left)
    TreeNode.check(right)
    self.left = left
    self.right = right
    self.root = root

  def inorder(self):
    s = list()


Use s = [], also don't use single letter variable names.

if not self.left:


When checking for None, recommened practice is to do it like: if self.left is None:

s.append('')  
    else:
      s.append(self.left.inorder())

    s.append(str(self.value))

    if not self.right:
      s.append('')
    else:
      s.append(self.right.inorder())

    return ' '.join(s).strip()


The fact that you are trying to construct a string is kinda wierd. I'd expect this method to produce either an iterator or a list containing all the elements.

# returns True if success, False otherwise  
  def insert( self, new_value ):
    # TODO ?? what happens if we are inserting strings
    if not new_value and new_value != 0:
      return False


This is why you should "is None"

if new_value == self.value:
      return False

    # need to find the right location in terms of binary tree ordering
    if new_value  self.value:


You've already established that this expression must be true, just use else

if not self.right:
        self.right = TreeNode(new_value)
        return True
      return self.right.insert( new_value )


The previous branch had this inside an else. You should at least be consistent between branches

return False

  @staticmethod    
  def check(node):
    if not node:
      return
    if not isinstance(node, TreeNode):
      raise TypeError('only instances of TreeNode are allowed')


Explicitly checking types is frowned upon. All you manage is converting an AttributeError into a TypeError. However, a check method could usefully verify the invariants of a data structure. (Just only do it while debugging).

def __repr__(self):
    return '(' + repr(self.left) + ',' + \
      str(self.value) + ',' + repr(self.right) + ')'


TreeNode seems to be a collection class, one that holds elements. The internal structure shouldn't matter to the world. However, repr spills the internal structure for the world to see. I'd suggest calling inorder to produce all of the elements inside the string and show that. Also repr's value should make it clear what class is being repr, by including "TreeMap" somewhere in its output.

Code Snippets

class TreeNode(object):
  """ applicable to all tree nodes including root node """
  def __init__(self, value, left=None, right=None,root=False):
self.value = value 
    TreeNode.check(left)
    TreeNode.check(right)
    self.left = left
    self.right = right
    self.root = root

  def inorder(self):
    s = list()
if not self.left:
s.append('')  
    else:
      s.append(self.left.inorder())

    s.append(str(self.value))

    if not self.right:
      s.append('')
    else:
      s.append(self.right.inorder())

    return ' '.join(s).strip()
# returns True if success, False otherwise  
  def insert( self, new_value ):
    # TODO ?? what happens if we are inserting strings
    if not new_value and new_value != 0:
      return False

Context

StackExchange Code Review Q#2280, answer score: 6

Revisions (0)

No revisions yet.