3
\$\begingroup\$

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;
  }
}
\$\endgroup\$
7
\$\begingroup\$

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

\$\endgroup\$
11
  • \$\begingroup\$ 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. \$\endgroup\$
    – srgbnd
    Aug 23 '16 at 12:19
  • \$\begingroup\$ 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 \$\endgroup\$
    – Vogel612
    Aug 23 '16 at 12:50
  • \$\begingroup\$ Now it looks good. And what about the variables scope? Where is it better to declare them? \$\endgroup\$
    – srgbnd
    Aug 23 '16 at 13:04
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ Aug 23 '16 at 14:19
  • 1
    \$\begingroup\$ @IsmaelMiguel sounds reasonable. Edited the answer. Thanks :) \$\endgroup\$
    – Vogel612
    Aug 23 '16 at 14:40
0
\$\begingroup\$

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;
  }
}
\$\endgroup\$
2
  • 4
    \$\begingroup\$ You just provided an alternative implementation without reviewing the code. Please add a review to comply with the sites standards. \$\endgroup\$
    – Mast
    Aug 23 '16 at 11:09
  • \$\begingroup\$ See meta question meta.codereview.stackexchange.com/q/1763/31503 for help \$\endgroup\$
    – rolfl
    Aug 23 '16 at 12: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.