views:

52

answers:

2

Ok, I have a validation script that checks everything on the form - but it flags the phone number fields as wrong regardless of whats in there. I've tried it a couple different ways and I cant figure out what I am doing wrong.

The part of the script that validates is...

    if (testPattern(phone1, /^\d{3}$/)== false) { // checking phone length
        valid = false;
    }
    if (testPattern(phone2, /^\d{3}$/)== false) {
        valid = false;
    }
    if (testPattern(phone3, /^\d{4}$/)== false) {
        valid = false;
    }

The function code is...

function testPattern(field, reg2) {
    var trueOrfalse = reg2.test(field)
    if (trueOrfalse == false) {
        field.style.backgroundColor="yellow";  // if false, change colors and return false
        field.style.color="red";
        return false;
    }
    else {
        field.style.backgroundColor="white"; // if true, change colors and return true
        field.style.color="black";
        return true;
    }
}
+3  A: 

Maybe

var trueOrfalse = reg2.test(field)

should be

var trueOrfalse = reg2.test(field.value)

Added:

Also, remember that you don't have to compare to true or false when evaluating in a boolean context. (Use the value itself or the negation). And it is better to name variables after their meaning, not "trueorfalse" Here's my re-write:

if (!testPattern(phone1, /^\d{3}$/)) { // checking phone length
    valid = false;
}
if (!testPattern(phone2, /^\d{3}$/)) {
    valid = false;
}
if (!testPattern(phone3, /^\d{4}$/)) {
    valid = false;
}



function testPattern(field, reg2) {
  var good = reg2.test(field.value);
  if (good) {
      field.style.backgroundColor="white"; // if good, change colors
      field.style.color="black";
  }
  else {
      field.style.backgroundColor="yellow";  // if bad, change colors
      field.style.color="red";
  }
  return(good);
}
Larry K
+1 for specifying that you should name variables after their meaning.
Cam
A: 

Not an actual answer to your question, since there's nothing inherently wrong with the snippets you posted, but this was too large for a comment.

Your code is really redundant!

You could express the whole first part as:

valid = testPattern(phone1, /^\d{3}$/) &&
        testPattern(phone2, /^\d{3}$/) &&
        testPattern(phone3, /^\d{4}$/)

And the function code as:

function testPattern(field, reg2) {
    var test_result = reg2.test(field)

    if (test_result) {
        field.style.backgroundColor = "white";
        field.style.color = "black";
    } else {
        field.style.backgroundColor = "yellow";
        field.style.color = "red";
    }

    return test_result;
}

Or even more concise:

function testPattern(field, reg2) {
    var test_result = reg2.test(field)

    field.style.backgroundColor = test_result ? "white" : "yellow";
    field.style.color = test_result ? "black" : "red";

    return test_result;
}

Isn't that much easier to read?

Max Shawabkeh