Javascript code help - newbie

I’m trying to have a specific input field to be enabled onload, only if a specific radio button is checked… after some hours, I’ve come with something like this:

The form id is “pedidoOnline”:


function activaInputOnload()
{

    //stores on an array all the fieldsets on our document:
    fieldsetArray = document.getElementsByTagName("fieldset");
*
    //for each occurence of fieldset:
    for (var i = 0; i < fieldsetArray.length; i++)
    {
        //stores the fieldset name atribute value into a variable radioName - so I hope. :s
        var radioName = document.pedidoOnline.fieldsetArray[i].id;

        //saves all the radio buttons of the actual fieldset
        //note: the fieldset id is equal to the radio name on a given fieldset.
        var radiosArray = document.pedidoOnline.fieldsetArray[i].getElementByName[radioName];

        //there will be only one 'outroTxt' input per fieldset. Hope getElementById doesn't return an array, it will be a 1 element array. :s'
        var inputFieldOutro = document.pedidoOnline.fieldsetArray[i].getElementById(match(/^outroTxt/));

        //for all radios on this fieldset
        for (var j=0; j < radiosArray.length; j++)
        {

            //if the fieldset radios starts with a string outro and, is checked:
            if ( (radiosArray[j].id.match(/^outro/)) && (radiosArray[j].checked) )
            {
                
                //the input field should be active (cross-browser active :s)
                inputFieldOutro.disabled=false;
                inputFieldOutro.focus=true;
                inputFieldOutro.click=true;

            }
        }

    }
}

However, I’m having an error:
document.pedidoOnline.fieldsetArray is undefined

on this line:

var fieldsetName = document.pedidoOnline.fieldsetArray[i].id; 

The error is a little thing compared with the issues all over this code, I realise that, and I hope you can give me some lights here… :s

Best regards,
Márcio
ps- I don’t need this to be necessarily on load, but it needs to be active if a server side validation returns errors. So I thought, onLoad. But if there are other ways to make js to communicate with php, please let me know.

Hello, no I’ve not forget it. It has been here on my firefox tab for long time I realize that. :slight_smile:

I really want to thank your time and detail on explain it.

Here is the final code, if you hated, then you will hated even more :wink:
I’m trying to program like this. Thanks a lot for your expertise;


"use strict";

var i;
var fieldsetArray;
var radioName;
var radiosArray;
var inputsArray;
var input;
var k;
var j;

function activaInputOnload()
{
    //stores on an array all the fieldsets on our document:
    fieldsetArray = document.getElementsByTagName("fieldset");
 
    //for each occurence of fieldset:
    for (i = 0; i < fieldsetArray.length; i = i + 1)
    {
        //stores the fieldset name atribute value into a variable radioName
        // ++ fieldsetArray[i] is a DOMNode, so you can request it's ID directly. No need to do more searching
        radioName = fieldsetArray[i].id;
 
        //saves all the radio buttons of the actual fieldset
        //note: the fieldset id is equal to the radio name on a given fieldset.
        // ++  fieldsetArray[i].getElementsByName is not function, document.getElementsByName() is
        radiosArray = document.getElementsByName(radioName);
 
        //there will be only one 'outroTxt' input per fieldset. Hope getElementById doesn't return an array, it will be a 1 element array. :s'
        // ++ You're use of the match function was incorrect as .match can only work on a string, and not stand alone.
        // ++ Since you want to find the input whose ID starts with "txtOutro", we first need to find all inputs in the fieldset:
        inputsArray = document.getElementById(radioName).getElementsByTagName('input');
 
        // ++ And now loop through all the fields in the fieldset
        for (k = 0; k < inputsArray.length; k = k + 1)
        {
            // ++ If the name of one them starts with "txtOutro", we found the one we need
            if (inputsArray[k].name.match(/^txtOutro/))
            {
                // ++ so we save it to the variable "input" ...
                input = inputsArray[k];
                // ++ and stop the loop. No need searching for something that has already been found :)
                break;
            }
        }
 
        // ++ It might be that we didn't find an input. In that case we should return now,
        // ++ otherwise the function might give an error "input is undefined"
        if (!input) {
            return;
        }
 
        //for all radios on this fieldset
        for (j = 0; j < radiosArray.length; j = j + 1)
        {
            //if the fieldset radios starts with a string outro and, is checked:
            if ((radiosArray[j].id.match(/^outro/)) && (radiosArray[j].checked))
            {
                //the input field should be active (cross-browser active :s)
                input.disabled = false;
                input.focus = true;
                input.click = true;
                // ++ Stop the loop. No need searching for something that has already been found :)
                break;
            }
        }
    }
}

You can replace


var i;
var fieldsetArray;
var radioName;
var radiosArray;
var inputsArray;
var input;
var k;
var j;

with


var i, fieldsetArray, radioName, radiosArray, inputsArray, input, k, j;

if you like :slight_smile:

Furthermore, if you only use these variables inside this function, you can (and should) put this in the function body


function activaInputOnload()
{
  var i, fieldsetArray, radioName, radiosArray, inputsArray, input, k, j;
  // rest of code
}

If the variables are used in multiple functions you should leave them where they are now.
For more information on why you should do this, click here.

So it’s not really a problem. Is a best practice. Right?
And like all best practices, they are not detachable of programmers habits.

I believe that, if the programmer understands the problem in place here:

  • the variables scope and the way javascript deals with it;

Then, either using one var statement or several, is it at no matter at all, if and only if, we can understand what is the problem related to this at the first place.

But we are not machines, and even if we know that something is wrong, and we are used to it, that is not a necessary condition to tell us that, THAT wrong will never happen, it could happen.

So, at the end, your advice is pertinent.

We can advocate that knowing something, is not only having the conceptual understanding of X and all is possibilities but, also, the fact of doing it always right… but this pertinent philosophical question I must leave it for later.

Thanks a lot. :slight_smile:

Márcio

It’s to help reduce the possibility of problems. If more than one is allowed, that opens the opportunity for them to appear at places other than at the start. That is the problem that is trying to be prevented in the first place. By enforcing them to a single var statement, those goals are achieved and the opportunity for bad habits to creep in are reduced.

Ok… I understand and I will do it.

Done it:


"use strict";

function activaInputOnload()
{
    var fieldsetArray, i, k, input, j, inputsArray, radiosArray, radioName;
    
    //stores on an array all the fieldsets on our document:
    fieldsetArray = document.getElementsByTagName("fieldset");
 
    
    //for each occurence of fieldset:
    for (i = 0; i < fieldsetArray.length; i = i + 1)
    {
        //stores the fieldset name atribute value into a variable radioName
        // ++ fieldsetArray[i] is a DOMNode, so you can request it's ID directly. No need to do more searching
        radioName = fieldsetArray[i].id;
 
        //saves all the radio buttons of the actual fieldset
        //note: the fieldset id is equal to the radio name on a given fieldset.
        // ++  fieldsetArray[i].getElementsByName is not function, document.getElementsByName() is
        radiosArray = document.getElementsByName(radioName);
 
        //there will be only one 'outroTxt' input per fieldset. Hope getElementById doesn't return an array, it will be a 1 element array. :s'
        // ++ You're use of the match function was incorrect as .match can only work on a string, and not stand alone.
        // ++ Since you want to find the input whose ID starts with "txtOutro", we first need to find all inputs in the fieldset:
        inputsArray = document.getElementById(radioName).getElementsByTagName('input');
 
        // ++ And now loop through all the fields in the fieldset
        for (k = 0; k < inputsArray.length; k = k + 1)
        {
            // ++ If the name of one them starts with "txtOutro", we found the one we need
            if (inputsArray[k].name.match(/^txtOutro/))
            {
                // ++ so we save it to the variable "input" ...
                input = inputsArray[k];
                // ++ and stop the loop. No need searching for something that has already been found :)
                break;
            }
        }
 
        // ++ It might be that we didn't find an input. In that case we should return now,
        // ++ otherwise the function might give an error "input is undefined"
        if (!input) {
            return;
        }
 
        //for all radios on this fieldset
        for (j = 0; j < radiosArray.length; j = j + 1)
        {
            //if the fieldset radios starts with a string outro and, is checked:
            if ((radiosArray[j].id.match(/^outro/)) && (radiosArray[j].checked))
            {
                //the input field should be active (cross-browser active :s)
                input.disabled = false;
                input.focus = true;
                input.click = true;
                // ++ Stop the loop. No need searching for something that has already been found :)
                break;
            }
        }
    }
}

Is just to have a place were the final code should rest. :slight_smile:

One last question, that I can’t figure it out:

We see often expressed that:

It is recommended that a single var statement be used per function.

What it isn’t explain to me, newbie, is the why should we have only one var statement per function ? What are the main reasons for this?

They should be at the top of our block scope element in javascript, the function, - that I get it. But why it’s better to have:
var a, b, c, d;
then:
var a;
var b;

etc ?

It must be something more then personal readability taste no?

Thanks a lot,
Márcio

That was a stupid topic header, and I’m sorry. I was yet editing it, and suddenly, I realize I forgot to change the topic summary. My bad.

The issue still applies. Sorry once again.

M.

I know this doesn’t answer your question, but have you considered using a javascript framework?

For example, using jQuery, what you want can be achieved with the following snippet:


$(document).ready(function() {

  $("fieldset").each( function() {
    if ($("input[id^='outro']:checked", this).size() > 0) {

      $("textarea[id^='outroText']", this).attr('disabled','disabled').focus().click();

    }
  });

});

See how much shorter and more readable that is? :slight_smile:

I do agree. I would love to, believe me.
But how can I jump to a ‘something’ framework, if I don’t really know how to deal with ‘something’? I mean, what frame work would that be for me?

More then only js or jquery or “what else we could put in here” issue, what I have is a logic and syntax issue here. A help towards those would be nice. :smiley:

I think you need to make it


 var radiosArray = document.pedidoOnline[fieldsetArray[i]].getElementByName[radioName];

No luck. :frowning:
Same error as before:
document.pedidoOnline[fieldsetArray[i]] is undefined

regrds,
Márcio

Where are you running the JavaScript from. The code needs to run after the radio button it refers to has loaded into the browser. The easiest way to do that is to call the code from the bottom of the page just before the </body> tag.

I’m calling it on the header, on a external js file.
I assume the header is loaded before the body.

Anyway, I will called on just before the body end tag.

I will post back the results in a shortwhile,

Thanks a lot,
Márcio

And, once again, sorry for this awfull topic title. if someone could change it… :s

I forgot. I was calling this external .js on the body bottom already.
It must something to do with the code then.

Here it is again, with the changes:


function activaInputOnload()
{

    //stores on an array all the fieldsets on our document:
    fieldsetArray = document.getElementsByTagName("fieldset");
*
    //for each occurence of fieldset:
    for (var i = 0; i < fieldsetArray.length; i++)
    {
        //stores the fieldset name atribute value into a variable radioName - so I hope. :s
        var radioName = document.pedidoOnline[fieldsetArray[i]].id;

        //saves all the radio buttons of the actual fieldset
        //note: the fieldset id is equal to the radio name on a given fieldset.
        var radiosArray = document.pedidoOnline[fieldsetArray[i]].getElementByName[radioName];

        //there will be only one 'outroTxt' input per fieldset. Hope getElementById doesn't return an array, it will be a 1 element array. :s'
        var inputFieldOutro = document.pedidoOnline[fieldsetArray[i]].getElementById(match(/^outroTxt/));

        //for all radios on this fieldset
        for (var j=0; j < radiosArray.length; j++)
        {

            //if the fieldset radios starts with a string outro and, is checked:
            if ( (radiosArray[j].id.match(/^outro/)) && (radiosArray[j].checked) )
            {

                //the input field should be active (cross-browser active :s)
                inputFieldOutro.disabled=false;
                inputFieldOutro.focus=true;
                inputFieldOutro.click=true;

            }
        }

    }
}

If I put this on jslint, with the good parts turned off, I get:
Implied global: fieldsetArray 5,8,11,15,18, document 5,11,15,18, match 18

But I have no clue about the meaning.

:frowning:

Regards,
Márcio

Could you post the HTML of one fieldset this javascript is supposed to work on?
Without it I’m rather clueless as to what the code is supposed to do in the first place …

Before all an important note on the code:
getElementByName doesn’t exist (so I believe).

getElementsByName exists.
Credit to Kor for finding it. So, we have a getElementById singular, and others will be plural, and that depends if we want to grab several elements or only one?

getElementById will return the ID of that element as string?
getElementsByName will return an ARRAY of string elements names?

Sure. Np. Thanks a lot.

  
<fieldset id="cursos">
 <legend>Cursos</legend>
    <input type="radio" onclick="desactivaInput('txtOutroCurso')" id="cursoModeloFotografico" name="cursos" value="" />
<label id="labelCursoModeloFotografico" for="cursoModeloFotografico">Modelo Fotográfico</label>
<br />

<input type="radio" onclick="desactivaInput('txtOutroCurso')" id="cursoMaquilhagem" name="cursos" value="" />
<label id="labelCursoMaquilhagem" for="cursoMaquilhagem">Maquilhagem</label>
<br />

<input type="radio" onclick="desactivaInput('txtOutroCurso')" id="tvCinema" name="cursos" value="" />
<label id="labelTvCinema" for="tvCinema">TV e cinema</label>
<br />

<input type="radio" onclick="activaInput('txtOutroCurso')" id="outroCurso" name="cursos" value="" />
<label id="labelOutroCurso" for="outroCurso">Outro</label>
<input type="text" id="txtOutroCurso" disabled="disabled" name="txtOutroCurso" value="" title="Por favor, indique que outro Curso deseja."/>
<br />
</fieldset>

The server side part has been removed obviously.

If it helps, on the beginning:

I’m trying to have a specific input field to be enabled onload, only if a specific radio button is checked

Thanks,
Márcio

Okay, so the following works

Comment lines starting with // ++ are comments added be me


function activaInputOnload()
{
	//stores on an array all the fieldsets on our document:
	fieldsetArray = document.getElementsByTagName("fieldset");

	//for each occurence of fieldset:
	for (var i = 0; i < fieldsetArray.length; i++)
	{
		//stores the fieldset name atribute value into a variable radioName
		// ++ fieldsetArray[i] is a DOMNode, so you can request it's ID directly. No need to do more searching
		var radioName = fieldsetArray[i].id;

		//saves all the radio buttons of the actual fieldset
		//note: the fieldset id is equal to the radio name on a given fieldset.
		// ++  fieldsetArray[i].getElementsByName is not function, document.getElementsByName() is
		var radiosArray = document.getElementsByName(radioName);

		//there will be only one 'outroTxt' input per fieldset. Hope getElementById doesn't return an array, it will be a 1 element array. :s'
		// ++ You're use of the match function was incorrect as .match can only work on a string, and not stand alone.
		// ++ Since you want to find the input whose ID starts with "txtOutro", we first need to find all inputs in the fieldset:
		var inputsArray = document.getElementById(radioName).getElementsByTagName('input');

		// ++ And now loop through all the fields in the fieldset
		var input;
		for (var k = 0; k < inputsArray.length; k++)
		{
			// ++ If the name of one them starts with "txtOutro", we found the one we need
			if (inputsArray[k].name.match(/^txtOutro/))
			{
				// ++ so we save it to the variable "input" ...
				input = inputsArray[k];
				// ++ and stop the loop. No need searching for something that has already been found :)
				break;
			}
		}

		// ++ It might be that we didn't find an input. In that case we should return now,
		// ++ otherwise the function might give an error "input is undefined"
		if (!input)
			return;

		//for all radios on this fieldset
		for (var j=0; j < radiosArray.length; j++)
		{
			//if the fieldset radios starts with a string outro and, is checked:
			if ( (radiosArray[j].id.match(/^outro/)) && (radiosArray[j].checked) )
			{
				//the input field should be active (cross-browser active :s)
				input.disabled=false;
				input.focus=true;
				input.click=true;
				// ++ Stop the loop. No need searching for something that has already been found :)
				break;
			}
		}
	}
}

BTW, only tested in FF (3.6.3)

PS. Coding this way makes me remember why I absolutely hated javascript before I used jQuery :wink: