1
\$\begingroup\$

For my project I have rewritten a small lib for a tree structure that was inspired by wangzuo's js-tree.

The reason was largely because I prefer to work with an array structure rather than nested objects.
Could you advise me if you see any bad practices?
I will work with React.js for an important project and if you notice anything bad, I'd like some feedback early.
Also if you have advice for me for improvements; things I should or could do better, I'd like to know.

I did some very basic tests that seem to work well so far.

/**
 * zp-Tree 
 * This Tree constructor is essentially a decorator for an array "tree of nodes" passed in. 
 * 
 * Purpose: 
 *  - Enter tree as arrays of nested objects Format 
 *  - Easily find / insert / delete Nodes 
 *  - Find parent / child / next / previous Nodes 
 *  - Respect nodes order: array will provide natural ranking
 *  
 * Inspired by js-tree: https://github.com/wangzuo/js-tree/blob/master/index.js 
 */ 

/**
 * Constructor 
 * @param {type} tree
 * @returns {Tree}
 */
function Tree( tree ){                                                          
    if( !tree.constructor === Array ) throw "Array is required!";               //Array is required 

    this.tree = tree; 
    this.index = {};                                                            //Index to quick-find nodes by key 
    this.cid = 0;                                                               //Count id's (used as auto-increment node id's) 

    this._build( tree );                                                        //Build (decorate) the tree 
} 

var proto = Tree.prototype; 

/**
 * Decorate the tree and build the index 
 * @param {type} tree, parentNode
 * @returns {undefined}
 */
proto._build = function( tree, parent, deep = true ){ 

    tree.forEach( function (node, idx) { 
        if(!node.cid) node.cid = this.cid++;                                    //Node cid 
        node.parent = parent;                                                   //If parent == null -> base node 
        node.level = parent ? parent.level + 1 : 0;                             //Node level 
        node.prev = idx > 0 ? tree[ idx - 1 ] : null;                           //Link to previous node 
        node.next = idx < tree.length ? tree[ idx + 1 ] : null;                 //Link next node 
        node.idx = idx;                                                         //Keep reference to current position 
        this.index[ node.cid ] = node;                                          //Add to index 

        if( deep ) {                                                            //Optimize for performance 
            if( node.children && node.children.length ) this._build( node.children, node ); 
        }

    }, this); 
}; 

/**
 * 
 * @param {type} cid
 * @returns {node}
 */
proto.findNode = function( cid ){
    var node = this.index[ cid ];                                               //Get node from the index 
    if( node ) return node; 
}; 

/**
 * Insert node into the tree 
 * @param {type} node
 * @param {type} parent_cid
 * @param {type} idx
 * @returns {node}
 */
proto.insertNode = function ( node, parent_cid, idx ){                          //Insert node @index below parent  

    var parentNode = this.findNode( parent_cid ); 

    if( !parentNode ){                                                          //No parent means base node 

        if( idx >= this.tree.length ) {
            this.tree.push( node );                                             //Push to the end 

        } else { 
            this.tree.splice( idx, 0, node ); 
        }; 

        this._build( this.tree, null, false ); 

    } else { 
        if( !parentNode.children ){ 
            idx = 0; 
            parentNode.children = []; 
        } 
        parentNode.children.splice( idx, 0, node );                             //Insert node into children array 
        this._build( parentNode.children, parentNode, false );                  //Rebuild 
    } 
    return node; 
}; 
/**
 * 
 * @param {type} node
 * @returns {undefined}
 */
proto.removeNode = function( node_cid ){ 
    var node = this.findNode( node_cid ); 
    if ( node ){ 
        var nodes = node.parent ? node.parent.children : this.tree;             //Parent node or null (when base node) 
        nodes.splice( node.idx, 1);                                             //Remove node from Tree 
        delete this.index[node_cid];                                            //Remove node from Index 
        if( node.parent) this._build( nodes, node.parent, false );              //Rebuild tree 

    }
}; 

Edit: next day, fixing a number of bugs.
My question mainly regards style / performance issues, not details.
Thanks

\$\endgroup\$
  • \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Simon Forsberg May 4 '16 at 17:10
  • \$\begingroup\$ @SimonForsberg Ok got it. This answer was very helpful anyway, I'll continue bug fixing / checking for feedback elsewhere. \$\endgroup\$ – Trace May 4 '16 at 17:27
  • 1
    \$\begingroup\$ You are welcome to post a follow-up question here on Code Review, if you wish to receive more suggestions. \$\endgroup\$ – Simon Forsberg May 4 '16 at 17:30
  • \$\begingroup\$ @SimonForsberg Thanks, maybe I will follow your suggestion. \$\endgroup\$ – Trace May 4 '16 at 17:45
  • \$\begingroup\$ Follow-up: codereview.stackexchange.com/questions/127511/… \$\endgroup\$ – Trace May 4 '16 at 18:23
2
\$\begingroup\$

It's pretty good actually, but requires more thought than necessary when reading it.

1) Remove the comments -- at best they are pointless (not the method docs, they are fine), and at worst confusing.

2) Try extracting the conditions (all of them) into either methods or variables with meaningful names. For example, a hasChildren(node) method might reduce some duplication and make the code more readable. This applies to all conditionals, in if statements and in ternary operators ?.

3) Deep shouldn't be treated as a boolean -- it should be an enum of some variety with values like deep and shallow.

4) Extract some functions. For example, in the _build() method you've got a big embedded function (closure) as the target of the loop. You could extract that into a private method with a meaningful name.

\$\endgroup\$
  • \$\begingroup\$ I have made structural changes based on your input. Could you please check if this is better? Thanks \$\endgroup\$ – Trace May 4 '16 at 16:38
  • \$\begingroup\$ @KimGysen Please see the comment I just did on your question. We have a way of doing iterative reviews and it does not involve continuously editing questions and answers. \$\endgroup\$ – Simon Forsberg May 4 '16 at 17:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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