0
\$\begingroup\$

This is a follow-up on my previous post.

I have made modifications based on the feedback received on the previous post, hence I request another review on the modified code if ok.

Note: review purpose is just to check good practices (code / performance wise) and see if anything's missing.

I created a jsfiddle here: https://jsfiddle.net/yc6z3v13/

/**
 * 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 
 * 
 * @author Kim Gysen 
 */ 

/**
 * @typedef Node 
 * @property{number} _cid The internal id used by zp-Tree
 * @property{?Node} prev The previous Node in the tree 
 * @property{?Node} next The next Node in the tree 
 * @property{?Node} parent The node's parent Node 
 * @property{?Array} children The node's childe nodes 
 * @property{number} level The node's depth level 
 * @property{boolean} isLeaf The node is (not) a leaf
 */

var CONSTRUCT_MODE = Object.freeze({ 
    DEEP: { value: 0, desc: "Construct all children" }, 
    SHALLOW: { value:1, desc: "Construct only top level" }
}); 

/**
 * Tree constructor function 
 * @constructor
 * @param {type} Array 
 * @returns {type} Tree 
 */
function Tree( tree ){ 
    tree = tree || []; 
    if( !_isArray( tree ) ) throw "Array is required!"; 

    this.tree = tree; 
    this.index = {}; 
    this._cid = 1; 

    this._build( tree ); 
} 

var proto = Tree.prototype; 

/**
 * Decorate the tree and build the index 
 * @param {Array} Nodes 
 * @param {Node} parent 
 * @returns {undefined}
 */
proto._build = function( tree ){ 
    _constructTree.call( this, tree ); 
}; 

/**
 * 
 * @param {number} _cid
 * @returns {Node} node 
 */
proto.findNode = function( _cid ){
    var node = this.index[ _cid ] || null; 
    return node; 
}; 

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

    var parentNode = this.findNode( parent_cid ); 
    if( parent_cid && !parentNode ) throw "Node insertion failed: Provided parent node doesn't exist. Pass 'null' if you want to add a base Node"; 

    var nodes = []; 
    _constructTree.call( this, [ node ], null, CONSTRUCT_MODE.DEEP ); 

    if( !parentNode ){ 

        if( !_indexWithinTree( this.tree, idx ) ) { 
            this.tree.push( node ); 

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

        }; 

        nodes = this.tree; 

    } else { 
        var nodes; 
        if( !_hasChildren( parentNode ) ){ 
            idx = 0; 
            parentNode.children = []; 

        }
        nodes = parentNode.children; 
        nodes.splice( idx, 0, node ); 
    } 

    _constructTree.apply( this, [ nodes, parentNode, CONSTRUCT_MODE.DEEP ] ); //DEEP required for syncing childNode.level

    return node; 
}; 

/**
 * 
 * @param {number} node__cid
 * @returns {undefined}
 */
proto.removeNode = function( node__cid ){ 

    var node = this.findNode( node__cid ); 
    if ( node ){ 
        var nodes = _hasParent( node ) ? node.parent.children : this.tree; 
        nodes.splice( node.idx, 1 ); 

        _removeNodeFromIndex.call( this, node ); 
        if( node.parent) _constructTree.apply( this, [ nodes, node.parent, CONSTRUCT_MODE.SHALLOW ] ); 

    }
}; 

function _constructTree( tree, parent, constructMode = CONSTRUCT_MODE.DEEP ){ 

    tree.forEach( function (node, idx) { 
        if(!node._cid) node._cid = this._cid++; 
        node.parent = parent; 
        node.level = _hasParent( node ) ? parent.level + 1 : 0; 
        node.prev = idx > 0 ? tree[ idx - 1 ] : null; 
        node.next = _indexWithinTree( tree, idx + 1 ) ? tree[ idx + 1 ] : null; 
        node.idx = idx; 
        node.isLeaf = !_hasChildren( node ); 
        node.isBase = !_hasParent( node ); 
        this.index[ node._cid ] = node; 

        if( constructMode === CONSTRUCT_MODE.DEEP ) { 
            if( _hasChildren( node ) ) _constructTree.apply( this, [ node.children, node, CONSTRUCT_MODE.DEEP ] ); 
        }

    }, this); 
}; 

function _removeNodeFromIndex( node ){ 
    delete this.index[ node._cid ]; 

    if( _hasChildren( node ) ){
        node.children.forEach( function( childNode, idx ){ 
            delete this.index[ childNode._cid ]; 

            if( _hasChildren( childNode ) ) _removeNodeFromIndex.call( this, childNode ); 

        }, this); 
    }
} 

function _indexWithinTree( tree, idx ) { 
    return !( idx >= tree.length || idx < 0 ); 
} 

function _hasParent( node ){ 
    if( node.parent ) return true; 
    return false; 
}

function _hasChildren( node ){ 
    if( node.children && node.children.length ) return true; 
    return  false; 
} 

function _isArray( arr ){
    return ( arr.constructor === Array ); 
} 
\$\endgroup\$
1
\$\begingroup\$

This revision brings a huge improvement in readability over your previous submission, but there are still a couple of minor things that attract otherwise undue attention.

1) The _hasChildren() and _hasParent() functions have an if(cond) return true else return false structure. This can be refactored to simply return cond, as you've done for the other boolean functions.

2) in _constructTree() you have a condition: constructMode === CONSTRUCT_MODE.DEEP. If you write an is() method on the DEEP and SHALLOW objects you could change this to something like: CONSTRUCT_MODE.DEEP.is(constructMode).

3) _constructTree() is doing too much. The loop itself should be in the _build method -- only the target function should have been extracted.

Our aim here is to avoid indentation -- actually, it's not literally that; rather, we're trying to have each method do things at one level, and every time there's a change of level -- a nesting -- we represent that by a function. That way we end up with code that reads like a book, explaining itself in abstractions that eventually become more and more concrete.

It's a good goal to have a single statement (function call) as the target of every iteration (loop) or selection (if). If you find yourself needing braces around things then you should consider putting those things together in a function with a meaningful name.

This can be useful in places like insertNode(), which has a big if statement, that could read as simple as:

if(!_hasParent(node)) 
    _createRoot(node, tree, idx);
else
    _createChild(node, tree, idx);
\$\endgroup\$
  • \$\begingroup\$ Looks promising. My hands are tied today but let me have a closer look tomorrow, and keep you posted! \$\endgroup\$ – Trace May 5 '16 at 1:57
  • \$\begingroup\$ In case you'd have a few minutes...: codereview.stackexchange.com/questions/127614/… \$\endgroup\$ – Trace May 5 '16 at 17:33

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.