Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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.left:
        self.left = TreeNode(new_value) 
        return True
      else:
        return self.left.insert( new_value )
    elif 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) + ')'

Thank you

share|improve this question

1 Answer

up vote 2 down vote accepted

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:
      if not self.left:
        self.left = TreeNode(new_value) 
        return True
      else:
        return self.left.insert( new_value )
    elif 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.

share|improve this answer
Whats wrong with single letter variable names? Especially for something so used as the s in this case. Do you dislike i as well? – mathepic May 7 '11 at 20:58
Single letter variable names are acceptable if the abbreviation is common. Otherwise it just makes the code harder to read. s doesn't strike me as a common abbreviation for what you are doing. Of course, that's subjective, so if you find it common enough, that's ok. – Winston Ewert May 7 '11 at 21:31
My own policy is to almost entirely avoid single letter variable names. Python features properly used eliminate many of the variables which would typically be candidates for such short names. I always use longer names to make sure I don't slip into being lazy and stop looking for a better way. – Winston Ewert May 7 '11 at 21:33
There are tons of code written by supposedly expert pythonista which uses isinstance. When is it kosher to use it ? – Jacques René Mesrine May 8 '11 at 6:28
@Jacques, that question is much too large to answer here. You should probably open another question on that. Short answer: everything always depends. – Winston Ewert May 8 '11 at 13:56

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.