Help improve this Javascript code?

Question

EDIT2:
I omitted the line that updates the UI "urrentNode.set_checked(checked);", and it run amazingly fast. Apparently this is the reason of that performance bottleneck, what do you guys suggest?

Hello SO,

In short, I'm dealing with Telerik's RadTreeView, and I want enable checking all the child nodes if the user checked the parent node. Simple enough! OK here's my code that handles OnClientNodeChecked event of the TreeView:

function UpdateAllChildren(nodes, checked) {

            var i;
            for (i = 0; i < nodes.get_count(); i++) {
                var currentNode = nodes.getNode(i);
                currentNode.set_checked(checked);                    
                if (currentNode.get_nodes().get_count() > 0) {
                   UpdateAllChildren(currentNode.get_nodes(), checked);                   
                }
            }
        }       

        function ClientNodeChecked(sender, eventArgs) {
            var node = eventArgs.get_node();         
            UpdateAllChildren(node.get_nodes(), node.get_checked());
        }

And here's the TreeView's markup:

<telerik:RadTreeView ID="RadTreeView1" runat="server" CheckBoxes="True" OnClientNodeChecked="ClientNodeChecked"></telerik:RadTreeView>

The tree contains quite a lot of nodes, and this is causing my targeted browser (ehm, that's IE7) to really slow down while running it. Furthermore IE7 displays an error message asking me to stop the page from running scripts as it's might make my computer not responsive (yeah, scary enough). So what do you guys propose to optimize this code?
EDIT: Runs pretty fast in firefox and chrome of course. Needles to say, hah?

Thanks in advance

Answer 1

For sure save your objects for reuse:

var i;
for (i = 0, n=nodes.get_count(); i < n; i++) {
  var currentNode = nodes.getNode(i);
  currentNode.set_checked(checked);
  var curNodes = currentNode.get_nodes();
  if (curNodes.get_count() > 0) {
    UpdateAllChildren(curNodes, checked);                   
  }
}

Answer 2

Avoid repeating calls to the objects. Get the value in a variable and reuse the variable. You don't need to check if the node collection is empty before calling the function, as the function already does that (in the loop condition).

function UpdateAllChildren(nodes, checked) {
  var cnt = nodes.get_count();
  for (var i = 0; i < cnt; i++) {
    var currentNode = nodes.getNode(i);
    currentNode.set_checked(checked);
    UpdateAllChildren(currentNode.get_nodes(), checked);                   
  }
}

Answer 3

Profile it in a proper browser to see where it's spending most of its time.

Like @mplungjan says, reuse objects.

It seems unlikely the get_count call is expensive, but if it is you can drop the check before recursing down.

If the library is slow you could possibly cheat and simulate click events yourself.

Answer 4

It's a guess, but I believe that set_checked calls ClientNodeChecked. This is why it's called many more times than it needs to be.

The easiest way to make sure of this is to run it in FF with Firebug, and use console.log to see how it's called exactly.

Answer 5

OK, apparently the problem is that the call to set_checked() is the most expensive part. I omitted it from the code and the recursive call went pretty fast. I also modified the ScriptMode property on the script manager to be "release" and this helped get rid of IE's annoying dialog, but the performance is still as slow. Case closed.