patternpythonMinor
Binary tree in Python
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.
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)
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.
Use s = [], also don't use single letter variable names.
When checking for None, recommened practice is to do it like: if self.left is None:
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.
This is why you should "is None"
You've already established that this expression must be true, just use else
The previous branch had this inside an else. You should at least be consistent between branches
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).
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.
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 FalseThis 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 FalseContext
StackExchange Code Review Q#2280, answer score: 6
Revisions (0)
No revisions yet.