current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 3 down vote favorite
1

I wrote a recursive function to uncheck all menu nodes. It works well, but I need to know if it is right to leave empty block inside if (nodes[i].children.length === 0) base case.

Is there a better design choice?

The function:

function uncheck_all_tree (nodes) {
  var i, n, n_dom_el;

  for (i = 0; i < nodes.length; i++) {
    if (nodes[i].children.length === 0) {
      // recursion bottom;
    } else {
      uncheck_all_tree(nodes[i].children); 
    }

    // pop nodes from recursion stack
    n = tree.jstree('get_node', nodes[i]);
    n_dom_el = document.getElementById(n.id + '_anchor');
    if (n_dom_el !== null) {
      console.log(n.id + '_anchor');
      n_dom_el.className = 'jstree-anchor';
    }
    n.state.checked = false;
  }
}
share|improve this question
up vote 7 down vote accepted

More things that can be improved:

Variable declarations

To allow for easier understanding you should declare variables as close as possible to their usage (even though it's semantically no different)

Additionally IIRC javascript variables are usually camelCase and not snake_case. Furthermore it's useful to write out variable names instead of shortening them as much as possible. This reduces cognitive load and makes the code easier to read.

General case

Simply through the way your code works, I'm dead sure that you don't even need that if-statement you have there. It's superfluous, because it's already working for the general case. If your nodes have 0 elements, the for-loop is not executed and the "recursion bottom" is reached.

"Final" result:

function uncheck_all_tree (nodes) {
    for (var i = 0, length = nodes.length; i < length; i++) {
        uncheck_all_tree(nodes[i].children); 
        var node = tree.jstree('get_node', nodes[i]);
        var nDomElement = document.getElementById(n.id + '_anchor');
        if (nDomElement !== null) {
            console.log(node.id + '_anchor');
            nDomElement.className = 'jstree-anchor';
        }
        node.state.checked = false;
    }
}

In addition to that Ismael Miguel has suggested in a comment to use Array.forEach.call(nodes[i].children, function(node) { [...] }); to further improve performance. More information on how that works can be found at the MDN Documentation

share|improve this answer
    
jslint suggest me to declare variables outside of the loop declaration and body. As far as I remember, it is because in JS, a variable is inside the function scope, not inside for () {} loop scope. Am I not right? Plus, I don't see the recursion (the function calling itself) in your final example. – trex Aug 23 at 12:19
    
Whoops I skipped that line.. I did mention that it makes no semantic difference for the variable declarations. This hints at the block scoping you mention – Vogel612 Aug 23 at 12:50
    
Now it looks good. And what about the variables scope? Where is it better to declare them? – trex Aug 23 at 13:04
1  
Just 3 small things: 1- You should store the length on a variable on your for loop: for (var i = 0; i < nodes.length; i++). It would look like this: for (var i = 0, length = nodes.length; i < length; i++). This makes transversing a lot faster. 2- The variable n should be called node. 3- You have double-quotes on n.id + "_anchor" while you have single-quotes through the code. Other than that, you can use Array.forEach.call(nodes[i].children, function(){ [...] }) instead of a for loop. – Ismael Miguel Aug 23 at 14:19
1  
@IsmaelMiguel sounds reasonable. Edited the answer. Thanks :) – Vogel612 Aug 23 at 14:40
up vote 0 down vote

I found the way, don't know why I didn't see it the first time :-)

There is no need for the empty if () {} block. The recursive call is needed only if there are children nodes nodes[i].children.length > 0. And all nodes are unchecked when we are moving back from recursion.

Final result:

function uncheck_all_tree (nodes) {
  var i, node, nodeDomEl;

  for (i = 0; i < nodes.length; i++) {
    if (nodes[i].children.length > 0) {
      uncheck_all_tree(nodes[i].children); 
    }
    // recursion bottom;
    // pop nodes from recursion stack
    node = tree.jstree('get_node', nodes[i]);
    nodeDomEl = document.getElementById(node.id + '_anchor');
    if (nodeDomEl !== null) {
      nodeDomEl.className = 'jstree-anchor';
    }
    node.state.checked = false;
  }
}
share|improve this answer
4  
You just provided an alternative implementation without reviewing the code. Please add a review to comply with the sites standards. – Mast Aug 23 at 11:09
    
See meta question meta.codereview.stackexchange.com/q/1763/31503 for help – rolfl Aug 23 at 12:12
1  
If you have a new question, please ask it by clicking the Ask Question button. Include a link to this question if it helps provide context. - From Review – rolfl Aug 23 at 12:13

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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