2

I have written the following codes, the purpose of these codes is to generate an <ul> from some javascript Node objects:

<html>

<body>
<div id='content'></div>
<script type='text/javascript'>

            function node(name,parent){
                this.name=name;
                this.parent=parent;
                this.level=function(){
                    if(this.parent==null)
                        return 0;
                    return this.parent.level()+1;                    
                }
                this.childs=new Array();
            }

            //create a rootNode, having no parent
            var rootNode=new node('AAA',null);

            //create node1 and add it to the child of rootNode
            var node1=new node('BBB',rootNode);
            rootNode.childs.push(node1);

            //create node2 and add it to the child of rootNode
            var node2=new node('CCC',rootNode);
            rootNode.childs.push(node2);

            //create node3 and add it to the child of node1
            var node3=new node('DDD',node1);
            node1.childs.push(node3);

            //create node4 and add it to the child of node1
            var node4=new node('EEE',node1);
            node1.childs.push(node4);

            //create node5 and add it to the child of node2
            var node5=new node('FFF',node2);
            node2.childs.push(node5);

            function getTreeHTML(node,html){

                if(node.level()==0)
                    html+='<ul>';

                html+='<li>';
                html+=node.name;
                if(node.childs.length > 0){
                    html+='<ul>';
                    for(var i in node.childs){
                        html+=getTreeHTML(node.childs[i],html);
                    }
                    html+='</ul>';
                }
                html+='</li>';

                if(node.level()==0)
                    html+='</ul>';
                return html;
            }

            var treeHTML=getTreeHTML(rootNode,'');
            document.getElementById('content').innerHTML=treeHTML;
</script>
</body>

</html>

Under rootNode, there are two immediate child nodes, for these two child nodes, one of them should have two childs children and another should have one child. I suspect there is a logical bug in the getTreeHTML() function but I just can't figure out what it is, feel free to paste the codes on http://htmledit.squarefree.com/, then you can quickly see what I mean.

Many thanks to you all.

2
  • FYI, plural of child is children, not childs :) Commented Dec 31, 2010 at 8:57
  • @Psytronic Thanks. I will use children next time. :) Commented Dec 31, 2010 at 9:19

2 Answers 2

4

The problem is here:

html+=getTreeHTML(node.childs[i],html);

You're ending up adding to the HTML string twice, because within the function you append to the HTML string passed in, but then you use an append operation with what it returns as well. If you change it to either of these, it works fine:

html=getTreeHTML(node.childs[i],html);
html+=getTreeHTML(node.childs[i],'');

Off-topic: Some suggestions/recommendations/observations if you're interested; none of the below is meant to be critical, only helpful:

  1. Rather than concatenating strings, it generally works out faster if you build up a bunch of strings in an Array and then use a join('') to put them all together into one string when you're done.
  2. Standard practice (which you're free to ignore!) is to make constructor functions like node initially capped (Node), to differentiate them from non-constructor functions.
  3. Each of your node objects will get its own copy of the level function, which isn't necessary in your code. They can all share one level function, like this:

    function node(name,parent){
        this.name=name;
        this.parent=parent;
        this.childs=new Array();
    }
    node.prototype.level = function() {
        if(this.parent==null)
            return 0;
        return this.parent.level()+1;                    
    }
    
  4. In English, the plural of "child" is "children" (it's irregular).

  5. Using for..in to loop through array indexes, as in your for(var i in node.childs){, is delicate and technically incorrect unless you take a couple of precautions in the loop; it will fail if you do anything interesting with your arrays like add more meta-data to them (arrays are just objects, you can add arbitrary properties to them; and libraries sometimes add properties to Array.prototype to smooth out browser differences, which will muck up a naive for..in loop). Details here: Myths and realities of for..in
  6. You can write this.childs=new Array(); as this.childs=[]; if you like; they have exactly the same effect.
  7. Regardless of where you put your var statement in your code, it takes effect at the top of the function (or the top of the global scope, if the var is global). I think I'm a bit on my own with this, but I don't like having my code be a bit misleading to code maintainers, so I always put the vars up top. More here Poor misunderstood var. So for instance:

    // This series of statements
    doSomething();
    doSomethingElse();
    var a = new Foo();
    doAnotherThing();
    
    
    // ...is *exactly* identical to:
    var a;
    doSomething();
    doSomethingElse();
    a = new Foo();
    doAnotherThing();
    
  8. Recommend a function on node that creates a child and hooks it up, so you avoid the possibility of the parent being set but then the child added to the wrong childs array (or not added at all).

  9. Using null to mean "no parent" is fine, but undefined is actually a bit easier to work with.
  10. You can use the power of JavaScript's Curiously-Powerful OR Operator (||) in some places. You see this a lot in JavaScript code. if statements are totally fine, but I thought it worth mentioning because it's such common practice.
  11. Braces really are your friends, even when they're not necessary.

Taking all of that and a couple of minor things together:

var rootNode, node1, node2;

function Node(name, parent){
    this.name = name;
    this.parent = parent; // undefined if none
    this.children = [];
}
Node.prototype.level = function(){
    return this.parent ? this.parent.level() + 1 : 0;
};
Node.prototype.addChild = function(name) {
    var child;

    child = new Node(name, this);
    this.children.push(child);
    return child;
};

//create a rootNode, having no parent
rootNode = new Node('AAA');

//create node1 and add it to the child of rootNode
node1 = rootNode.addChild('BBB');

//create node2 and add it to the child of rootNode
node2 = rootNode.addChild('CCC');

//create node3 and add it to the child of node1
node1.addChild('DDD');

//create node4 and add it to the child of node1
node1.addChild('EEE');

//create node5 and add it to the child of node2
node2.addChild('FFF');

function getTreeHTML(node, html) {
    var i;

    html = html || []; // In case they don't give it

    if(node.level()==0) {
        html.push('<ul>');
    }

    html.push('<li>');
    html.push(node.name);
    if (node.children.length > 0) {
        html.push('<ul>');
        for (i = 0; i < node.children.length; ++i) {
            getTreeHTML(node.children[i], html);
        }
        html.push('</ul>');
    }
    html.push('</li>');

    if (node.level() == 0) {
        html.push('</ul>');
    }

    return html;
}

var treeHTML = getTreeHTML(rootNode).join('');
document.getElementById('content').innerHTML=treeHTML;
Sign up to request clarification or add additional context in comments.

6 Comments

Or remove the html-parameter completely, construct the html into a local variable and returning that. It would be somewhat "cleaner".
On a different issue, shouldn't the code be html+=getTreeHTML(i,''); and not html+=getTreeHTML(node.childs[i],''); I mean, each i will be a node type itself, so why are we using i as an index. I am little bit confused here.
@Gunner: No, the code's right. for..in doesn't loop through the array entries, it loops through the property names (in this case, array indexes) of the object. So i will be 0, then 1, etc.
@T.J. Crowder: Thanks for answer and all the self tagged off-topic comments. I wish I could up vote your post a second time :)
@T.J. Crowder It's really amazing to get an answer like this, which is very very informative. Thank you so much.
|
1

Remove the html parameter all together:

function getTreeHTML(node){
    var html = "";
    if(node.level()==0)
        html+='<ul>';

    html+='<li>';
    html+=node.name;
    if(node.childs.length > 0){
        html+='<ul>';
        for(var i in node.childs){
            html+=getTreeHTML(node.childs[i]);
        }
        html+='</ul>';
    }
    html+='</li>';

    if(node.level()==0)
        html+='</ul>';
    return html;
}

3 Comments

There's a logical bug in my brain. Thanks for your answer, it's a useful one.
I'd retain it, but make it an array, so you get the ability to build everything up in an array and then do a join('') at the end, which is faster on nearly every (possibly every) JavaScript implementation I've run into than string concatenation. FWIW.
@T.J indeed, I'd also convert the whole thing to Factory Design pattern, and make GetTreeHtml a function of the Node object ;)

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.