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

Try to implement comments tree for Django

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

Problem

I've tried to create an implementation of a tree for comments. I want use it in Django to store comments. Please tell me how to implement it much simpler, without recursion, to find child nodes.

class commentTree(object):
def __init__(self, id, data, parentId = 0, childrens = None):
    "Represents one leaf Tree"
    self.id = id
    self.parentId = parentId
    self.title = data['title']
    self.body = data['body']
    self.childrens = []
    self.isLeaf = True if len(self.childrens) > 0 else False
    if childrens is not None:
        for children in childrens:
            self.childrens.append(children)

def addComment(self, parentId, data):
    root = 1 if self.isLeaf == False else 0
    comment = commentTree(parentId + root + len(self.childrens), data, self.id)
    if parentId == self.id:
        self.childrens.append(comment)
    else:
        node = self.findById(parentId)
        node.childrens.append(comment)

def getParent(self, node):
    "Gets parent node"
    if self.parent is not None:
        return self.parent

def getSize(self, node):
    "Returns size of childrens length + node"
    return len(node.childrens) + 1

def findById(self, id):
    def searchInChildren(id, childNodes):
        for children in childNodes:
            if children.id == id:
                return children
            else:
                searchInChildren(id, children)
    if id == self.id:
        return self
    else:
       result = searchInChildren(id, self.childrens)
       if result.id == id:
           return result
       else: return None

def getSiblings(self, node):
    "Get nodes that have one parent"
    nodes = []

    def getAllChildrens(self, child):
        for children in node.childrens:
            nodes.append(children)
            if children.isLeaf:
                getAllChildrens(self, children)

    getAllChildrens(self, node)
    return nodes

Solution

A few comments regarding style:

-
As pointed out by @jonrsharpe

  • Please have a look at PEP8



  • Use for spaces for each indentation level



-
Naming

  • Use capitalized words for class names (commentTree -> CommentTree)



  • Use underscores for methods (findById -> find_by_id)



-
Do not use spaces around the equal sign to set default values for arguments (parentId = 0 -> parent_id=0)

-
Review hardcoded booleans since usually they are not needed:

self.isLeaf = True if len(self.childrens) > 0 else False
self.is_leaf = len(self.children) > 0  # True if ... else False not needed

root = 1 if self.isLeaf == False else 0
root = 1 if not self.is_leaf else 0  # False not needed
root = int(not self.is_leaf)  # boolean to int conversion


-
Do not use builtin name functions for arguments (id)

-
Note that children is already plural

Regarding the code:

-
addComment method

  • What is the goal of the new commentTree object id calculation? Shouldn't be easier to get a new id from the database itself?



  • What is the benefit of being able to add a node as a child of a node that is not the node for which the method was called. This makes the API confusing in my opinion.



-
getParent method

  • What is the expected returned value when self.parent is None?



  • If the expected value is None, then the whole method implementation could be return self.parent



-
getSize method

  • Why is there a node parameter to this method? Shouldn't return the length of the tree object itself?



  • This method implementation should be recursive. Note that each child may have children on its own.



-
findById method

  • Why don't you like the recursive implementation? Recursion is what makes them implementation easier and I don't think you need to handle more levels than allowed by the interpreter



  • In the recursion the base case, the one for a leaf node, is missing



-
I'd try something like this:

def find_by_id(self, comment_id):
    """Find node by id."""
    if self.node_id == node_id:
        return self

    if self.is_leaf:
        return None

    for child in self.chidren:
        node = child.find_by_id(id)
        if node:
            return node


-
getSiblings method

  • Why is there a node parameter to this method? Shouldn't return the siblings of the current node?



  • There is no need to implement any recursion since siblings are the nodes at the same level of the tree.



-
I'd try something like this:

def get_siblings(self):
    """Get sibling nodes."""

    # Get parent (not implemented, but using find_by_id on the root node should work)
    parent = ...

    return [node for node in parent.children if node.id != self.id]


I hope this helps.

Code Snippets

self.isLeaf = True if len(self.childrens) > 0 else False
self.is_leaf = len(self.children) > 0  # True if ... else False not needed

root = 1 if self.isLeaf == False else 0
root = 1 if not self.is_leaf else 0  # False not needed
root = int(not self.is_leaf)  # boolean to int conversion
def find_by_id(self, comment_id):
    """Find node by id."""
    if self.node_id == node_id:
        return self

    if self.is_leaf:
        return None

    for child in self.chidren:
        node = child.find_by_id(id)
        if node:
            return node
def get_siblings(self):
    """Get sibling nodes."""

    # Get parent (not implemented, but using find_by_id on the root node should work)
    parent = ...

    return [node for node in parent.children if node.id != self.id]

Context

StackExchange Code Review Q#58839, answer score: 2

Revisions (0)

No revisions yet.