HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Activating items in some randomized arrays

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
arraysrandomizedactivatingitemssome

Problem

I've got a list with about 120 items in IE and I have some items I want to activate. I compare, build and randomize some Arrays on the fly. The for loop in the run() function is really slow, and i tried to add a setTimeout(), like i've read somewhere on stackoverflow.

It does not seem to improve the performance, and if the requested list of items to be activated is too large, IE starts to give the slow script error.

Can i optimize this to something that'll work?

```
for(index = 0; index < 120; index++){
wrapper.tempSlot.push(index);
}
Array.prototype.shuffle = function shuffle(){
var tempSlot;
var randomNumber;
for(var i =0; i != this.length; i++){
randomNumber = Math.floor(Math.random() * this.length);
tempSlot = this[i];
this[i] = this[randomNumber];
this[randomNumber] = tempSlot;
}
}

Array.prototype.compare = function(testArr) {
if (this.length != testArr.length) return false;

for (var i = 0; i < testArr.length; i++) {
if (this[i].compare) {
if (!this[i].compare(testArr[i])) return false;
}
if (this[i] !== testArr[i]) return false;
}
return true;
}

function checkAgainstEl(val){
val = $($item.select).eq(val).data('personid')
return val in oc([array, with, to, be, activated, numbers]);
}

var index = 0;
var length = wall.globals.highlight.selection.length;
var run = function(){
for(;index < length; index++){

wrapper.tempSlot.shuffle();
var pop = wrapper.tempSlot.pop();
var value = wall.globals.highlight.selection[index];
var newItem = $('#wallItem' + value);

wrapper.objectsStr = wrapper.objectsStr + '#wallItem' + value + ', ';

while(checkAgainstEl(pop)){
wrapper.tempSlot.shuffle()
pop = wrapper.tempSlot.pop();
}

beforeItem = $($item.select).eq(pop);

newItem.insertBefore(beforeItem);
$($container.select

Solution

-
Interacting with the DOM is one of the slowest operations in JavaScript. I'd suggest selecting all your elements before the loop, then referring to the cached elements inside.

-
On the same token, do not perform any DOM manipulation in a loop if you can help it. Since it doesn't look like you can, another approach is to detach all DOM elements you're going to rearrange before the loop, perform the manipulation inside the loop, then re-attach them to the DOM at the end.

-
A reverse while loop would be faster than the for.

-
If you're using a version of jQuery prior to 1.6, the .data() method carries a lot of overhead. In checkAgainstEl(), use $.data($($item.select).get(val), 'personId') instead.

-
It looks like you're re-wrapping jQuery objects in new jQuery objects unnecessarily, and in multiple places. Like in #4. I'm assuming variables prefixed with a $ are jQuery objects; if they are, no need to wrap them in $() again.

Your biggest problem is the DOM interaction inside a loop. Cut that out any way you can and you'll be golden.

Context

StackExchange Code Review Q#2283, answer score: 5

Revisions (0)

No revisions yet.