List Box Crash When Several Items Added

[JuliaRBarton]

I have looked everywhere I can for a solution to this problem but
nothing so far has worked.

I have two list boxes in my form. The first one (selAvailable) is
populated from an access database. The second (selSelected) is
populated by selecting items in selAvailable and clicking the add
button. In some situations there can be hundreds or thousands of items
in the selAvailable list box. If a user selects a few hundred or
clicks the Add All button the selSelected list box will begin to
populate until the whole page freezes then crashes.

I tried making the list box selSelected invisible before added items
to it but that did not help.

I know it seems that no user would ever want to have a list box with
so many items but in the context of my webpage it is needed.

Below is the Add All code that adds all of the items from the
selAvailable list box to the selSelected list box. I greatly
appreciate anyones help, thank you very much in advance.

~Julia

function AllAdd()
{
var i
var count
var selectedText
var selectedValue
var selAvailableLength = frmSelect.selAvailable.length
var selSelectedLength = frmSelect.selSelected.length
var newoption


var count = selSelectedLength + 0;

for (i = 0; i < selAvailableLength; i++)
{
selectedText = frmSelect.selAvailable.options.text;
selectedValue = frmSelect.selAvailable.options.value;
newoption = new Option(selectedText, selectedValue, false, false);
frmSelect.selSelected.options[count] = newoption;
count = count + 1;
frmSelect.selAvailable.options = null;
selAvailableLength = frmSelect.selAvailable.length;
i--;
}

}

 

[g4toloc0]

I have looked everywhere I can for a solution to this problem but
nothing so far has worked.

mmm.. have you tried with different browsers/OS?
What browser are you currently using?

About the code..

i-- inside the for loop is pure evil..
anyway.. i suppose that your code works so you can try:

use "display" property to hide the destination combo object (that's
really different than using the visibility prop)
try to delay the insert for loop using a setTimeout call.

hope this helps.
Ciao.
Seba

 

[JuliaRBarton]

Thank you I will try those suggestions,

The most there will be in a list box will be hundreds OR thousands,
not hundreds of thousands, I agree that anything over 5,000 would be
ridiculous which is about the most the website would ever populate,

thanks

 

[JuliaRBarton]

mmm.. have you tried with different browsers/OS?
What browser are you currently using?

About the code..

i-- inside the for loop is pure evil..
anyway.. i suppose that your code works so you can try:

use "display" property to hide the destination combo object (that's
really different than using the visibility prop)
try to delay the insert for loop using a setTimeout call.

hope this helps.
Ciao.
Seba

The i--; was a quick fix for adding of items without skipping any, why
is it pure evil? Is it just bad practice or does use more memory? I am
a novice here and appreciate your help, thanks!

Julia

 

[JuliaRBarton]

Hey, thanks for the suggestion, I ended up calling the add function
recursively by the setTimeout method with an interval of 1, just
enough to keep it from freezing up, thanks!

 

[RobG]

The i--; was a quick fix for adding of items without skipping any, why
is it pure evil? Is it just bad practice or does use more memory?

Because it is causing your browser to crash. You are incrementing i
in the for statement, then decrementing it inside the for block. It
is initially set to zero, decremented to -1 at the bottom of the
block, then incremented back to zero at the start of the next loop -
it will never get beyond zero, you are adding option[0] a zillion
times until the browser crashes.

The options collection is live, so if you *remove* option then
option[i+1] you will skip every second option. However, you are
copying so that isn't an issue.

Just get rid of that line.

Incidentally, if you don't need to copy the options, try just moving
them instead (where you will have to account for skipping options):

var fromSel = frmSelect.selAvailable;
var toSel = frmSelect.selSelected;
var i = 0;
var len = fromSel.length;
while (len--) {
toSel.options = fromSel.options[0];
i++;
}


Untested, but the idea is to simply assign the options from one select
to the other. When you remove fromSel.options[0], they all 'shuffle
up' so what was option[1] is now option[0].

 

[g4toloc0]

Hey, thanks for the suggestion, I ended up calling the add function
recursively by the setTimeout method with an interval of 1, just
enough to keep it from freezing up, thanks!

I am happy it is working now.. as a suggestion think about this..
when you start a very intensive and long loop, your browser will just
sit down and loop..
it won't run the screen refresh cycle and it won't listen to any mouse
or keyboard event.

Your browser (my little experience is limited to javascript on FF and
IE) will try to inform you that something bad is happening and will
try to bring up an alert window allowing the user to abort the
script.
[this is so easy to try out.. just write something like "while (1)" as
a little demo]

But in certain situations the 'Hang detector alert' is not raised
(this control has been maily implemented for infinite loops
detection..) in addtion, if your loop uses a lot of memory some
problems may occur.
If the trash collector is not able to deallocate memory your script
will be out of control and in a short time your browser will crash.

This explains why using a timer solves the problem. By calling
setTimeout you allow the browser to proccess events and deallocate
memory while dealing with the insertion loop.

Ok, i think that's enough.

Ciao
Seba

 

[g4toloc0]

The i--; was a quick fix for adding of items without skipping any, why
is it pure evil? Is it just bad practice or does use more memory?

Well, RobG and David Golightly answers are far more better than mine,
but i need to do some practice (i really hate not being able to speak/
write fluent English).
(note: I am sure that what i am going to say has been already (better)
discussed here..)

And now.. about that "i--"
I think we cant talk a little about good programming practices..

In my opinion talking about "good" and "bad" code is a very subjective
argument, i think it is not different from talking about "my Firefox
is good your IE is bad", "Perl rocks, PHP sucks" etc., etc. (argh...
this is really going beyond about what i want to say..)

Anyway.. there are some "universal" programming practices you can
apply to any programming language:

About for loops
---
Do not touch the loop counter inside the loop itself. It can bring to
unexpected results.
If you are modifying the loop counter this mean that you don't need a
for loop ("while" and "do while" will suit better)

Of course i am talking about 'normal' for loops.. (you can find any
sort of programming genius doing the weirdest things out there)

In your code is easy to see that a for loop it is not the right
choice...
You are saying the javascript interpreter to increment "i" (that's why
you are using a for loop) and then you are using i--.
Of course you are free to do whatever you want if you just need your
code working.. but doing things like this one greatly reduce code
readability (weird code is hard to debug)

Dom Interaction
---
in your loop you are referencing frmSelect lot of times..
You can greatly reduce browser work and execution time by using local
lookup variables. It is a good practice to always minimize the scope
chain of objects.

e.g.

var x=frmSelect.selAvailable; //more suitable var name needed
//x is now a lookup var, use x instead of
//frmSelect.selAvailable inside your loop

Ok.. i think this is enough for now.

Ciao
Seba