Javascript Problem

[cavey]

Can anyone see why my Javascript isn't working? It just checks for empty text boxes and other form elements, and it works well on another page I did, but I bet I just have a stupid typo I don't see... I do way more PHP than JS so maybe I am missing something. It passes as true even if all of the boxes are empty, and creates a blank customer account.

<script language="JavaScript" type="text/javascript">
  //<!--
     function checkit ( addsubscriber )
		{
		if (document.addsubscriber.company.value =='') 
                &&  ((document.addsubscriber.firstname.value =='') 
                || (document.addsubscriber.lastname.value =='')) 
                  {
		    alert('Please provide a company and/or a first and last name.');
		    document.addsubscriber.firstname.focus();
		    return false;
		  }
		else if (document.addsubscriber.address1.value =='') 
                  {
		    alert('Please provide a street address.)');
		    document.addsubscriber.address1.focus();
		    return false;
		  }
		else if (document.addsubscriber.city.value =='') 
                  {
		    alert('Please provide a city.');
		    document.addsubscriber.city.focus();
		    return false;
		  }
		else if   

(document.addsubscriber.country.options[document.addsubscriber.country.selectedIndex].value == 'United States')
		    && (document.addsubscriber.state.options[document.addsubscriber.state.selectedIndex].value == '') {
		  alert('Please provide a state.');
		  document.addsubscriber.state.focus();
		  return false;
		}
		else if (document.addsubscriber.country.options[document.addsubscriber.country.selectedIndex].value == 'Canada') && (document.addsubscriber.province.value =='') 
                {
		   alert('Please provide a province.');
		   document.addsubscriber.province.focus();
		   return false;
		}
		else if ((document.addsubscriber.country.options[document.addsubscriber.country.selectedIndex].value == 'Canada')
		|| (document.addsubscriber.country.options[document.addsubscriber.country.selectedIndex].value == 'United States')) && (document.addsubscriber.zipcode.value =='') {
		   alert('Please provide a zip code.');
		   document.addsubscriber.zipcode.focus();
		   return false;
		}
		else if (document.addsubscriber.country.options[document.addsubscriber.country.selectedIndex].value == '') {
		   alert('Please enter a country.');
		   document.addsubscriber.country.focus();
		   return false;
		}

return true;
 }
 //-->
  </script>

The form

<form action="scripts/addsubscriber_script.php" method="post" name="addsubscriber" id="addsubscriber" onSubmit="return checkit(this)">

<input size="40" name="company" type="text" tabindex="1"/>

<input size="40" name="firstname" type="text" tabindex="2"/>

<input size="40" name="lastname" type="text" tabindex="3"/>

<input size="40" name="city" type="text" tabindex="4"/>

etc.

etc.

etc.

</form>

[JPnyc]

Because the last statement is return true. No matter what comes before, it will always return true, because you told it to. The return true must be part of an else statement.

[cavey]

JPnyc wrote:

Because the last statement is return true. No matter what comes before, it will always return true, because you told it to. The return true must be part of an else statement.

That is not true at all in JavaScript. You do not need the else, it is implied. The code below works 100% perfectly, and it has the same clause at the end:

<script language="JavaScript" type="text/javascript">
  <!--
  function checkit ( addsubscriber )
    {
        if (addsubscriber.firstname.value =='') {
                alert('Please provide a first name');
                addsubscriber.firstname.focus();
                return false;
        }
	    else if (addsubscriber.email.value =='') {
                alert('Please provide your email address (See Privacy Statment)');
                addsubscriber.email.focus();
                return false;
        }
        else if (addsubscriber.saddress1.value =='') {
                alert('Please provide your street address');
                addsubscriber.saddress1.focus();
                return false;
        }
        else if (addsubscriber.spostalcode.value =='') {
                alert('Please enter your postal code');
                addsubscriber.spostalcode.focus();
                return false;
        }
        else if (addsubscriber.scity.value =='') {
                alert('Please enter your city');
                addsubscriber.scity.focus();
                return false;
        }
	    else if (addsubscriber.scountry.options[addsubscriber.scountry.selectedIndex].value == '') {
                alert('Please enter your country');
                addsubscriber.scountry.focus();
                return false;
        }
	    else if (addsubscriber.cardtype.options[addsubscriber.cardtype.selectedIndex].value == '') {
                alert('Please enter your credit card type');
                addsubscriber.cardtype.focus();
                return false;
        }
		else if (addsubscriber.cardnumber.value =='') {
                alert('Please enter your credit card number');
                addsubscriber.cardnumber.focus();
                return false;
        }
		else if (addsubscriber.cardexpmon.options[addsubscriber.cardexpmon.selectedIndex].value == '') {
                alert('Please enter your credit card expiration month');
                addsubscriber.cardexpmon.focus();
                return false;
        }
		else if (addsubscriber.cardexpyear.options[addsubscriber.cardexpyear.selectedIndex].value == '') {
                alert('Please enter your credit card expiration year');
                addsubscriber.cardexpyear.focus();
                return false;
        }
		else if (addsubscriber.cardsc.value =='') {
                alert('Please enter your credit card authorization code');
                addsubscriber.cardsc.focus();
                return false;
        }

    return true;
  }
  //-->
  </script>

i think it has something to do with the parenthesis around my && and || operators. I am pretty sure that they all need enclosed in outsie parens, I'll try that now that I think of it.

[mpb001]

He is using early returns so it never gets to the end.

If the javascript is knackered the form submits by default. Install firebug, turn javascript errors on. Wrap in a try catch

http://www.w3schools.com/js/js_try_catch.asp

Try catch won't stop syntax errors like these but it will stop referencing anything that does not exist etc from breaking it. The database should also be guarded by the php, javascript is just a convenience method for the user to stop round trips.

You are also parsing a reference to the form with the onsubmit=checkit( this ) then hard calling it on the inside. Notice the example does not use the document keyword.

Yours 
 if (document.addsubscriber.company.value =='') 
        &&  ((document.addsubscriber.firstname.value =='') 
        || (document.addsubscriber.lastname.value =='')) 
          {
    alert('Please provide a company and/or a first and last name.');
    document.addsubscriber.firstname.focus();
    return false;
  }

Theirs 
if (addsubscriber.firstname.value =='') {
            alert('Please provide a first name');
            addsubscriber.firstname.focus();
            return false;
    }

Though your immediate problem is your brackets are out of sync.

You have to wrap the whole clause

eg 
if (document.addsubscriber.company.value =='') 
    &&  ((document.addsubscriber.firstname.value =='') || (document.addsubscriber.lastname.value =='')) 

should be 

if ((document.addsubscriber.company.value =='') 
    &&  ((document.addsubscriber.firstname.value =='') || (document.addsubscriber.lastname.value =='')) ) 

There are a few like that. Firebug in firefox shows you them all.

[cavey]

The PHP / SQL code is spot on, I am pretty good with that stuff. I used:

$lastname = mysql_real_escape_string($_POST['lastname']);

to pre-screen injections, and cleaned up the phone number with:

$onlynums = preg_replace('/[^0-9]/','',$phone);

  if (strlen($onlynums)==10) { 
     $areacode = substr($onlynums, 0,3);
     $exchange = substr($onlynums,3,3);
     $number = substr($onlynums,6,4);
     $phone = $areacode . "-" . $exchange . "-" . $number;
     }
  else {
    go back etc... 
    }

I just never use client side scripting if I can avoid it, as many of our customers are rednecks at a transmission shop at the end of a dirt road and will probably have Windows 95 on an i486 with no javascript support. Now that I am working on an intranet that forces Firefox on an all mac network, it isn't an issue.

And WTF is knackered? LOL...

[JPnyc]

All you need to change is the last line

else { return true; }

[mpb001]

He is using early/multiple returns so he does not need an else. If and if else are optional.

It's in java but the logic is the same

http://www.javalobby.org/forums/thread.jspa?forumID=61&threadID=16937

The main problem is there is 4-5 if statements written without a () wrapping the condition.

eg
if (x==x) && (y==y)

instead of

if ((x=x) && (y==y))

I know because I checked it and added the braces to remove the errors in firebug.