[Javascript] Checkbox challenge

[Paul Novitski]

dev,

May I suggest some improvements to your code?

If that were my code I'd replace:
         onClick='CheckMax(5,window.document.forms[0].MyBest)'
with:
         onClick='CheckMax(5,this)'
which passes only the current object (the checkbox) to the function.  Once 
in CheckMax() you can easily expand from the current input object to the 
array of same-named tags:

function CheckMax(maxNo,obj)
{
         var numChecked = 0
         var bolPop=false

         // get array of objects in the current form with the same name as obj
         var aObjects = obj.form[obj.name]

         for(var x = 0; x < aObjects.length; x++)
etc.


Making this change appeals to me for a few reasons:

- "CheckMax(5,this)" is shorter and less redundant.

- It makes me a little uncomfortable to build an array of objects 
("forms[0].MyBest") on the fly as you're calling a function.  If a bug 
crept into your code it might be harder to track down if it triggered an 
error during a function call.  I suggest that your code will be cleaner & 
easier to manage if you pass simple values & objects to your functions, and 
then let the functions perform more complicated processes on those data 
where you can monitor them more easily.

- Replacing:
         window.document.forms[0].MyBest
with:
         obj.form[obj.name]
appeals to me because the latter doesn't assume anything.  "forms[0]" 
assumes that the checkboxes will always be in the first form on the page, 
so the code will break if you ever add a prior form region to your page -- 
say, for example, if you insert a search control into your page chrome that 
has its own form tag.  Also, "forms[0].MyBest" hard-codes the object name, 
so if (when) you changed names you'd have to do it twice for each 
checkbox.  In contrast, "obj.form" always refers to the form enclosing the 
current object, and "obj.name" always refers to the current object's name, 
and these will work even if you add more form tags to your page or rename 
your checkboxes.

Your test:
         if (numChecked >= maxNo)
should probably be:
         if (numChecked > maxNo)
if you want to stop when you exceed the maximum, not when you reach it.

Last, I don't think there's any desirable result from your final "return 
true" statement -- in my experience, the onclick event doesn't behave any 
differently if the function it calls returns true, false, or nothing at 
all.  If the code doesn't advance your cause, rip it out~

Cheers,
Paul



At 04:58 PM 5/6/2004, dev at qroute.net wrote:
>I did it this way; it worked. Thank you guys !...
>
><script>
>function  CheckMax(maxNo,obj)
>{
>  var numChecked = 0
>
>  var bolPop=false;
>  for(var x = 0; x < obj.length; x++){
>   if(obj[x].checked){
>    numChecked++
>    if (numChecked>=maxNo) {
>     bolPop=true
>     obj[x].checked=false
>    }
>   }
>  }
>
>  if (bolPop)
>  {
>   alert('You have already selected '+maxNo+' items. ')
>  }
>
>  return true;
>}
></script>
>
>I call them like this
>
><td width="25%"><span class="cssMainSmall"><input
>onClick='CheckMax(5,window.document.forms[0].MyBest)' type="checkbox"
>name="MyBest" value="Intelligent">Intelligent</span></font></td>
>
><td width="25%"><span class="cssMainSmall"><input
>onClick='CheckMax(5,window.document.forms[0].MyBest)' type="checkbox"
>name="MyBest" value="Thoughtful">Thoughtful</span></font></td>
>
><td width="25%"><span class="cssMainSmall"><input
>onClick='CheckMax(5,window.document.forms[0].MyBest)' type="checkbox"
>name="MyBest" value="Sweet">Sweet</span></font></td>
>  </tr><tr>
><td width="25%"><span class="cssMainSmall"><input
>onClick='CheckMax(5,window.document.forms[0].MyBest)' type="checkbox"
>name="MyBest" value="Passionate">Passionate</span></font></td>