Learning JS, trying to figure out createelement

[Ben Hallert]

Hi there,

I haven't done a lot of Javascript programming, so I'm not having great
luck debugging something I put together, and I was hoping someone here
could give it a quick lookover and maybe see the obvious mistake I'm
making.

My challenge is, I've been asked to make a change to an internal tool.
It has a screen with a tree control and a series of buttons at the top.
The buttons at the top control which form is submitted, and are
enabled/disabled based on what type of things are selected in the tree
below.

It currently uses radio buttons, but I've changed the items that are
parents to be checkboxes instead so I can have multiselect. To
maintain compatibillity with the other forms that expect a single
variable (named testclass_id) to be posted, I've made the following
action to be executed on submit:

function dealWithMultiSelect ()
{
/*This function should parse through all tcl set checkboxes to see if
there are more then one
//selected. If so, it needs to create a new hidden field with the
same name for each so we can
//send multiple instances of testclass_id to fulfillments review.
//Suggested flow, go through all of the checkboxes and see if there
are more then one active.
//In a sub loop, for each found, add a new hidden field.
//We'll modify set_val to make sure that there isn't a combination of
tcs and tcls set*/
num = document.tc.id.length;
for (var i = 0; i < num; i++)
{
if (document.tc.id.checked == true && document.tc.id.type ==
"checkbox")
{
//if it's not the FIRST tcl selected, then create a new hidden
element named testclass_id
if (TotalTCLSelected)
{
var hidden = document.createElement('input');
hidden.setAttribute('type','hidden');
hidden.setAttribute('value',document.tc.id.value);
hidden.setAttribute('name','testclass_id');
document.getElementById(document.appendChild(hidden));
}
TotalTCLSelected++;
}
}
}

As you can see, I'm trying to use createelement to add more hidden
variables of the same name (in this case, 'testclass_id') so that I'll
be emulating a multi-select GET on the next screen. The intention is
that it loops through all the inputs in the form and, if the item is
checked AND is of the type 'checkbox', it adds a new hidden input with
the value of that item. End result, it should submit multiple
instances of 'testclass_id'. I increment TotalTCLSelected so that it
doesn't start doing that until the second one it encounters on purpose,
that's also to maintain compatibillity with other forms.

Can anyone offer some suggestions? My main theory is that the problem
has to do with one or more of the following:
1. Can I really keep redefining the var hidden in the loop? And does
it get cleared each time?
2. Am I messing up the object inheritance by doing
document.createelement instead of something like
document.tc.createelement? (where tc is the name of the form being
parsed)
3. I didn't define TotalTCLSelected anywhere. It isn't erroring out,
but could this be silently killing it?
4. Other?

Thanks!

 

[Martin Honnen]

Ben Hallert wrote:

var hidden = document.createElement('input');
hidden.setAttribute('type','hidden');
hidden.setAttribute('value',document.tc.id.value);
hidden.setAttribute('name','testclass_id');
document.getElementById(document.appendChild(hidden));

The last line is somehow nonsense, you should call appendChild on the
element you want to append the input to, probably the form element or a
descendant of the form e.g.
document.forms.formName.appendChild(hidden);
The document.getElementById is not needed at all.
Also consider to set value and defaultValue of the input, otherwise some
browsers will not have the value set for the newly created input:
hidden.value = hidden.defaultValue =
document.tc.id.value;

 

[RobG]

Ben Hallert wrote:
[...]

num = document.tc.id.length;

If this is intended to get all the input elements of a form with
id="tc", you should consider getting all the form elements and then
use that:

var ele = document.forms['tc'].elements;

and then you could change:

for (var i = 0; i < num; i++)
{
if (document.tc.id.checked == true && document.tc.id.type ==
"checkbox")

to

for (var i = 0; i < ele.length; i++) {
if (ele.type == "checkbox" && ele.checked ) {

If you test if it's checked first, then the second test is more or less
redundant since only a checkbox can be checked. But you also depend on
non-checkboxes returning evaluating to false, which probably should
happen but may not.

{
//if it's not the FIRST tcl selected, then create a new hidden
element named testclass_id
if (TotalTCLSelected)

You haven't declared TotalTCLSelected. Do so near the start of your
script (before the for loop), and change the if statement:

var TotalTCLSelected = 0; // set to zero
...
if (TotalTCLSelected == 0) // or <1 or !> 0 whatever
{ ... }
TotalTCLSelected++;

If you are not going to use the value of TotalTCLSelected, make it a
boolean:

var TotalTCLSelected = false;
if (TotalTCLSelected == 0)
{ ... }
TotalTCLSelected = true;

Of course you may then want to change the name to 'pastFirstTCL' or
similar.

{
var hidden = document.createElement('input');
hidden.setAttribute('type','hidden');
hidden.setAttribute('value',document.tc.id.value);

You really need better names for variables, change "hidden" the
variable name to hiddenInput or something otherwise it is very
confusing for anyone trying to maintain your code.

Why not (if you've used my suggestion above and renamed 'hidden')

hiddenInput.value = ele.value;

hidden.setAttribute('name','testclass_id');

This will give all your hidden inputs the same name, which whilst
invalid HTML will likely not cause any problems at the client end
(unless you want to refer to them by id). Consider:

hiddenInput.name = 'testclass_id' + i;

document.getElementById(document.appendChild(hidden));
}
TotalTCLSelected++;
}

I think your appendChild error has been well covered. To make life
much easier, why not include the hidden inputs in the HTML?

[...]

1. Can I really keep redefining the var hidden in the loop? And does
it get cleared each time?

It's OK.

2. Am I messing up the object inheritance by doing
document.createelement instead of something like
document.tc.createelement? (where tc is the name of the form being
parsed)

No. You create the element, then add it to a collection.

3. I didn't define TotalTCLSelected anywhere. It isn't erroring out,
but could this be silently killing it?

Very likely (it doesn't work for me). Just declare it before using it
in the if statement.

4. Other?

Of course I haven't tested any of the above very much since I only have
your snippet and no HTML. If you have any issues post your modified
code & I'll have another go.

 

[RobG]

RobG wrote:
[...]

if (TotalTCLSelected == 0) // or <1 or !> 0 whatever

That should have been:

if (TotalTCLSelected != 0) // or <1 or !> 0 whatever