Code reviews

JavaScript
1

If you are looking for a code review or suggestions on how to improve on code you’ve written then please prefix the summary of your thread with [Code Review]. That will alert our resident experts to comment on the code you have written.

Please do not use the Code review title for normal bug hunting, but rather when you have accomplished something and wonder if you could have done it better.

It is preferable to post the snippet of code in question as well as a link to the site to help in working out what issues need to be addressed. Don’t just link to the whole site and say review this code, be specific about what you want reviewed.

Please use the highlight function with the appropriate syntax.
To highlight your code as javascript, select your code, and then from the Select Syntax… dropdown list, choose JavaScript.

That will wrap your code in tags like this:

[noparse]


var someValue = 0;
var anotherValue = 'something else';
doSomething(someValue, anotherValue);

[/noparse]

Which causes your code to be displayed like this:


var someValue = 0;
var anotherValue = 'something else';
doSomething(someValue, anotherValue);

Highlighting your code with the appropriate syntax makes it much easier for people to read and understand your code, which in turn makes it easier for them to help with your code.

Be aware that you may get conflicting and opposing views from different people. Ultimately it will be your task to sift through the information and make a decision based on what you have read. There is usually no one perfect way to do something, although there are established best practices.

Remember this is just about reviewing the code only, as website Design elements and Content reviews already have their own forums.

If this facility proves useful then these reviews may get their own dedicated forum at a later date.

2

The following is a good example of an effective review.

[QUOTE=minusten;4851389]here is a code i recently did

I would be very interested to see possible improvements!:)[/quote]

Here are some alternative improvements, which I’ll take step by step through the process of how they are achieved.

It’s easy to tell that there is a lot of duplication throughout your script.
Let’s remove some of that duplication, so that it’ll be easier to see where the improvements can occur.

Before I get started, I’m goin to run the code through the Online JavaScript Beautifier, so that indenting and other presentational aspects are cleaned up. That will help to make it easier to interpret what the code is doing.

Default Values

The first thing to deal with are where the field value is being checked for a certain string. Doing that forces you to update multiple things whenever you want to update the text. That causes you to not do such updates, which can lead to bad things.

Web pages automatically provide the default text in a property called defaultValue, so we can instead check that the field value equals the fields defaultValue.

Before:

if (document.Forders.fName.value.length == 0 ||
    document.Forders.fName.value == [color="red"]"Please enter your name"[/color]) {

After:

if (document.Forders.fName.value.length == 0 ||
    document.Forders.fName.value == [color="blue"]document.Forders.fName.defaultValue[/color]) {

Similarly with select lists, we can check if the selectedIndex is 0, without needing to know the value that’s there. Whenever the selectedIndex is 0, that refers to the first one that’s selected at the top of the list.

Before:

if (document.Forders.aState.value [COLOR="Red"]== "000"[/COLOR]) {

After:

if (document.Forders.aState.selectedIndex [COLOR="blue"]=== 0[/COLOR]) {

Those updates to remove the default field value strings should be done throughout all of your code.

Passing by Reference

The next form of duplication involves the field names.
In each function, the full field name is referred to over and over again. Take a look at We should assign that common field name to a variable, so that we can use that instead:

Look at for example the nBlank() function:


function nBlank() {
    if ([color="blue"]document.Forders.fName[/color].value.length ==0 ||
        [color="blue"]document.Forders.fName[/color].value == [color="blue"]document.Forders.fName[/color].defaultValue) {
        alert("Please enter your name");
        return false;
    }
    if ([color="blue"]document.Forders.address[/color].value.length == 0 ||
        [color="blue"]document.Forders.address[/color].value == [color="blue"]document.Forders.address[/color].defaultValue) {
        alert("Please enter your address");
        return false;
    }
    if ([color="blue"]document.Forders.suburb[/color].value.length == 0 ||
        [color="blue"]document.Forders.suburb[/color].value == [color="blue"]document.Forders.suburb[/color].defaultValue) {
        alert("Please enter your suburb");
        return false;
    }
}

When we assign those names to a variable called field, the code becomes much more consistent, making it easier for us to improve on.


function nBlank() {
    [color="green"]var field;[/color]
    [color="green"]field = document.Forders.fName;[/color]
    if ([color="green"]field[/color].value.length ==0 ||
        [color="green"]field[/color].value == [color="green"]field[/color].defaultValue) {
        alert("Please enter your name");
        return false;
    }
    [color="green"]field = document.Forders.address;[/color]
    if ([color="green"]field[/color].value.length == 0 ||
        [color="green"]field[/color].value == [color="green"]field[/color].defaultValue) {
        alert("Please enter your address");
        return false;
    }
    [color="green"]field = document.Forders.suburb;[/color]
    if ([color="green"]field[/color].value.length == 0 ||
        [color="green"]field[/color].value == [color="green"]field[/color].defaultValue) {
        alert("Please enter your suburb");
        return false;
    }
}

The above changes have helped us to pinpoint large amounts of duplication within the function, and have also made it easier to remove that duplication.

So what can we do about that? If the field variable is passed in to the form, along with the error message, then you won’t need those three nearly identical pieces of code in there.


function nBlank([color="green"]field, error[/color]) {
    if (field.value.length == 0 || field.value == field.defaultValue) {
        alert([color="green"]error[/color]);
        return false;
    }
}

And when we call nBlank() from the validateForders() function, we can also replace document.Forders with just form


function validateForders(form) {
    ...
    if (nBlank([color="blue"]form.fName, "Please enter your name"[/color]) === false) {
        isValid = false;
    }
    if (nBlank([color="blue"]form.address, "Please enter your address"[/color]) === false) {
        isValid = false;
    }
    if (nBlank([color="blue"]form.suburb, "Please enter your suburb"[/color]) === false) {
        isValid = false;
    }
    ...
}

The good news is that the massive amount of duplication in the nBlank() function is now solved.
The not-so-good news is that now that we’ve removed the duplication from the nBlank() function, there is now duplication in the validateForders() function.

Arrays to the Rescue

Fortunately for us, arrays are made to handle just such an issue. That, combined with associative lists, means that we can solve all our problems.

All we need to do is to put fName, address and suburb in an array, along with their error message. We can then pass that info to a function that will loop through them, and pass them to the nBlank() function for us.

This is also a good time to decide how to group the validation info. If it were books on a shelf, you could group them by size, or by title, or by color, or by category.

With this validation info, two of the common groupings are:

[list][]by type of validation, where all appropriate fields are validated at the same time for being blank
[]or by type of field, where each field has several different types of validation requirements that they need to pass[/list]

I’ll follow on from here using the first type where groups of fields are validated for being blank, or for being a number, and so on. While there can be some duplication of error messages with this, it’s closer to how you’ve coded your validation script so it should be easier for you to follow.

The field and error message can be put together in an array, where each item of the array handles a different field/error message. Each array item could store the field and message as values of another nested array, but that then goes against the idea that array items should be similar to each other.
So instead, an associative array can be used to store these different pieces of information instead.

We can also make good use of the form variable that is in the validateForders function too:


var notBlanks = [
    {field: form.fName, error: "Please enter your name"},
    {field: form.address, error: "Please enter your address"},
    {field: form.suburb, error: "Please enter your suburb"}
];

We can now loop through each of those items, and assume that things are valid until the nBlank() function tells us that things aren’t.


function validateNotBlanks(fieldData) {
    var field,
        error,
        i;
    for (i = 0; i < fieldData.length; i += 1) {
        field = fieldData[i].field;
        error = fieldData[i].error;
        if (nBlank(field, error) === false) {
            return false;
        }
    }
}

That means that the validateForders() function just needs this code for validating the nBlank() fields:


var notBlanks = [
    {field: form.fName, error: "Please enter your name"},
    {field: form.address, error: "Please enter your address"},
    {field: form.suburb, error: "Please enter your suburb"}
];
if (validateNotBlanks(notBlanks) === false) {
    isValid = false;
}

There is more than just notBlanks to validate though, so it will be useful if we can use the same looping function to validate the other ones. We can do that by passing a reference of nBlank to a validate function, so that the field and the error message can then be passed along to nBlank.


function [color="green"]validate[/color](fieldsToCheck[color="green"], func[/color]) {
    var field,
        error,
        i;
    for (i = 0; i < fieldsToCheck.length; i += 1) {
        field = fieldData[i].field;
        error = fieldData[i].error;
        if ([color="green"]func[/color](field, error) === false) {
            return false;
        }
    }
}

Now we can pass to the validateForders() function a reference to the nBlank function:


var notBlanks = [
    {field: form.fName, error: "Please enter your name"},
    {field: form.address, error: "Please enter your address"},
    {field: form.suburb, error: "Please enter your suburb"}
];
if ([color="green"]validate[/color](notBlanks[color="green"], nBlank[/color]) === false) {
    isValid = false;
}

That code is now easy to understand, and it’s easy to keep it updated too.

Now that we have the above base on which to build upon, we can start to look at the other functions.

Checking Default Labels

Performing similar updates to the cDefault() function gives us:


function cDefault(field, error) {
    if (field.selectedIndex === 0) {
        alert(error);
        return false;
    }
}

There was one set of validation in there that was commented out, and the good news is that you can still keep that in the array list too, if you still want it to be available, but not yet active:


var notDefaultDropdowns = [
    {field: form.aState, error: "Please select a State"},
    {field: form.cCard, error: "Please choose your card type"},
    {field: form.emonth, error: "Please select card expiry month"},
    {field: form.eyear, error: "Please select card expiry year"},
    {field: form.basket, error: "Please make a basket choice"} /*,
    {field: form.quan, error: "You have chosen 1 basket"}*/
];
if (validate(notDefaultDropdowns, cDefault) === false) {
    isValid = false;
}

That was pretty much the same as for the notBlanks() function, which is a nice and useful thing.

Validating Numbers

The numVal() function introduces an interesting issue. Some fields of the code within it are doing more checks than with other fields.


var field = document.Forders.postcode;
if ([color="red"]isBlank(field.value) || [/color]isNum(field.value) || field.value.length != 4) {
 ...
var field = document.Forders.homeP;
if ([color="red"]isBlank(field.value) || [/color]isNum(field.value) || field.value.length != 10[color="red"] ||
    field.value == field.defaultValue[/color]) {
 ...
var field = document.Forders.workP;
if (isNum(field.value) || field.value.length != 10) {

All of those red bits are already handled by the nBlank() function, so we can add them to the notBlanks check validation.


var notBlanks = [
    {field: form.fName, error: "Please enter your name"},
    {field: form.address, error: "Please enter your address"},
    {field: form.suburb, error: "Please enter your suburb"},
    [color="blue"]{field: form.postcode, error: "Please enter a valid Post Code"},
    {field: form.homeP, error: "Please enter a valid Home phone number"}[/color]
];

That now leaves the numVal function more consistent, and capable of being condensed with less repetition.
Some of the lengths that are checked though are different in size. We can deal with that by passing to the function the size that we want. How do we pass in the size?

Additional Data Required

When validating your page, you will sometimes find that you need to provide more info than just the type of validation and an error message. You might have min/max/size/range needs to take care of, or you may need to pass on other instructions relating to the form.

To help deal with all of these different needs, we can have a third generic argument called data. That way the validate() function can pass any extra needed info to the function that needs it.


function validate(fieldsToCheck, func) {
    var field,
        [color="green"]data,[/color]
        error,
        i;
    for (i = 0; i < fieldsToCheck.length; i += 1) {
        field = fieldData[i].field;
        [color="green"]data = fieldData[i].data;[/color]
        error = fieldData[i].error;
        if (func(field, error[color="green"], data[/color]) === false) {
            return false;
        }
    }
}

function numVal(field, error[color="green"], data[/color]) {
    if (isNum(field.value) || field.value.length != [color="green"]data.size[/color]) {
        alert(error);
        field.focus();
        return false;
    }
}

The numeric values can now be validated, with their size requirements passed along in the data section.


var numericValues = [
    {field: form.postcode, [color="green"]data: {size: 4},[/color] error: "Please enter a valid Post Code"},
    {field: form.homeP, [color="green"]data: {size: 10},[/color] error: "Please enter a valid Home phone number"},
    {field: form.workP, [color="green"]data: {size: 10},[/color] error: "Please enter a valid Work phone number"},
    {field: form.faxP, [color="green"]data: {size: 10},[/color] error: "Please enter a valid fax number"}
];
if (validate(numericValues, numVal) === false) {
    isValid = false;
}

Currently the numVal() function only handles the size property, but it wouldn’t be difficult at all to extend that to handle things like min, max and range too.

Setting the Focus

You might have noticed too that the numVal() function focuses the field on an error, while the nBlank() function does not. How can we have it check the postcode and homeP, and have those fields still gain the focus?

We can add another piece of data to say whether we want the field to be focussed, or not. Since more than just the nBlank() function might want to focus the field, we should check for that focus part from the validate function itself:


function validate(fieldsToCheck, func) {
    ...
        if (func(field, error, data) === false) {
            [color="green"]if (data.focus === true) {
                field.focus();
            }[/color]
            return false;
        }
    ...
}

Now we can easily tell the postcode and homeP fields to set their focus when the error occurs, as well as all of the numeric fields too.


var notBlanks = [
    {field: form.fName, error: "Please enter your name"},
    {field: form.address, error: "Please enter your address"},
    {field: form.suburb, error: "Please enter your suburb"},
    {field: form.postcode, [color="green"]data: {focus: true},[/color] error: "Please enter a valid Post Code"},
    {field: form.homeP, [color="green"]data: {focus: true},[/color] error: "Please enter a valid Home phone number"}
];
var numericValues = [
    {field: form.postcode, data: {size: 4[color="green"], focus: true[/color]}, error: "Please enter a valid Post Code"},
    {field: form.homeP, data: {size: 10[color="green"], focus: true[/color]}, error: "Please enter a valid Home phone number"},
    {field: form.workP, data: {size: 10[color="green"], focus: true[/color]}, error: "Please enter a valid Work phone number"},
    {field: form.faxP, data: {size: 10[color="green"], focus: true[/color]}, error: "Please enter a valid fax number"}
];

Of course, it’s entirely up to you to decide if you always want the error field to gain focus. If that’s the case, you don’t need the data focus part at all. You can just remove that if statement from the validate function so that the field will always focus after showing the error message.

Regardless of that, after the numVal() function has the focus parts removed from it, that now leaves it looking remarkably similar to the cBlank() and nDefault() functions:


function nBlank(field, error) {
    if (field.value.length == 0 || field.value == field.defaultValue) {
        alert(error);
        return false;
    }
}
function cDefault(field, error) {
    if (field.selectedIndex === 0) {
        alert(error);
        return false;
    }
}
function numVal(field, error, data) {
    if (isNum(field.value) || field.value.length != data.size) {
        alert(error);
        return false;
    }
}

Error Messages

Those three functions help us to see that the error alert is a common thing that can be brought up to the validate() function.

If we bring the error alerting to the validate function, those three functions will only need to return true or false, so they can be reduced to the following:


function nBlank(field) {
    return (field.value.length == 0 || field.value == field.defaultValue);
}
function cDefault(field) {
    return (field.selectedIndex === 0);
}
function numVal(field, data) {
    return (isNum(field.value) || field.value.length != data.size);
}

With the error alert now occurring in the validate() function:


function validate(fieldsToCheck, func) {
    ...
    if (func(field, error) === false) {
        [color="green"]alert(error);[/color]
        ...
    }
    ...
}

So, numVal() is now tidied up, and since the isBlank() function was only used by numVal(), the isBlank() function can be removed completely.

That leaves four different functions to go: valEmail(), radioVal(), delDrops() and checkText()

Validating Email

With the valEmail() you can just pass the field in there, and you’re nearly done:

I won’t get in to all of the different ways to validate emails. To validate them properly can get complex and scary. There is however the issue of those x’s.


function valEmail([color="green"]field[/color]) {
    var [color="red"]x[/color] = [color="green"]field[/color].value
    var at = [color="red"]x[/color].indexOf("@");
    var dot = [color="red"]x[/color].lastIndexOf(".");
    return (at < 1 || dot < at + 2 || dot + 2 >= [color="red"]x[/color].length);
}

That x variable needs to be improved since it doesn’t say much on its own. What is x. Does it mark the spot, is it a fashion accessory, or is it a random variable name because nothing else seemed appropriate.

Let’s have the variable name give us at least some idea or what it contains, by calling it email.


function valEmail(field) {
    var [color="green"]email[/color] = field.value
    var at = [color="green"]email[/color].indexOf("@");
    var dot = [color="green"]email[/color].lastIndexOf(".");
    return (at < 1 || dot < at + 2 || dot + 2 >= [color="green"]email[/color].length);
}

The final thing that I would tidy up in the valEmail() is the comparison.
Instead of checking for three different fail conditions:
(a || b || c)

We can instead check that three pass conditions aren’t met:
!(!a && !b && !c)

Does that sound crazy? Bear with me.

Inverting a less than comparison just means checking for greater than or equal. We can also get rid of that invert at the start too, by checking if the whole lot equals to false. That also causes it to use the same structure as all of our other validation functions. Happy joy!

So after inverting everything we have:

(at [color="blue"]>=[/color] 1 [color="blue"]&&[/color] dot [color="blue"]>=[/color] at + 2 [color="blue"]&&[/color] dot + 2 [color="blue"]<[/color] str.length) [color="blue"]=== false[/color]

If we now move the variables to the left of the comparison, and the numbers to the right, we end up with this:

(at >= 1 && dot [color="green"]- at[/color] >= 2 && [color="green"]str.length -[/color] dot [color="green"]>= 2[/color]) === false

That can be much easier to understand. When it’s easier to tell at a glance what it being checked for, it makes it that much harder for bugs to remain in the code.

You could also replace it with a regular expression, such as:

/^.{1,}@.{1,}\\..{2,}$/.exec(email)

which does exactly the same job as the comparison, but easily understanding what a regular expression does can be something that even experts have trouble with.

With the above changes in place, the valEmail() function ends up being:


function valEmail(field) {
    var email = field.value
    var at = email.indexOf("@");
    var dot = email.lastIndexOf(".");
    return ((at >= 1 && dot - at >= 2 && email.length - dot >= 2) === false);
}

And it’s called with:


var emailValues = [
    {field: form.eMail, error: "Not a valid e-mail address"}
];
if (validate(emailValues, valEmail) === false) {
    isValid = false;
}

Depending on Dependencies

The radioVal() function has a lot of duplication there too, that we can remove:

What we are starting with:

function radioVal() {
    if (document.Forders.Group1[1].checked) {
        var field = document.Forders.dStreet;
        if (field.value.length == 0 || field.value == field.defaultValue) {
            alert("Please enter your delivery address");
            return false;
        }
        var field = document.Forders.dSuburb;
        if (field.value.length == 0 || field.value == field.defaultValue) {
            alert("Please enter your delivery suburb or town");
            return false;
        }
        var field = document.Forders.Dpostcode;
        if (field.value.length != 4 || field.value == field.defaultValue || isNaN(field.value)) {
            alert("enter valid delivery postcode");
            return false;
        }
    }
}

I think that the radioVal() function has moved far away from validating radio buttons. Instead of validating radio buttons, it’s just ensuring that the normal validation of some fields depends on that radio button being selected.

That dependency requirement can be added as data to the field info, which can then be used by our standard validation routine.


var notBlanks = [
    ...
    {field: form.dStreet, [color="green"]data: {dependsOn: form.Group1[1]},[/color] error: "Please enter your delivery address"},
    {field: form.dSuburb, [color="green"]data: {dependsOn: form.Group1[1]},[/color] error: "Please enter your delivery suburb or town"},
    {field: form.Dpostcode, [color="green"]data: {dependsOn: form.Group1[1]},[/color] error: "Please enter valid delivery postcode"}
];
var numericValues = [
    ...
    {field: form.Dpostcode, [color="green"]data: {dependsOn: form.Group1[1], size: 4},[/color] error: "Please enter a valid delivery postcode"}
];

Now we just need the validate function to look for that dependsOn condition, and only do the validation when that dependsOn field is active. To help keep things nice and simple, we can hand off that task to a separate function:


function validate(fieldsToCheck, func) {
    ...
    if ([color="green"]checkDependency(data.dependsOn) &&[/color] func(field, error) === false) {
        alert(error);
        if (data.focus === true) {
            field.focus();
        }
        return false;
    }
    ...
}

So if the checkDependency() function returns true, the validate() function will carry on and validate the field. If checkDependency() returns false, the validate() function just skips that field and moves on with the next.

As you can see here, the things that it checks for are very basic, but quite useful.


function checkDependency(field) {
    if (field.type === 'checkbox') {
        return field.checked;
    }
    if (field.type === 'radio') {
        return field.selected;
    }
    if (field.type === 'text') {
        return (field.value.length > 0 && field.value !== field.defaultValue);
    }
}

Now the radioVal() function can be deleted.

The delDrops() function has a similar situation, so we can handle it now with only these additions:


var notDefaultDropdowns = [
    ...
    {field: form.dState, [color="green"]data: {dependsOn: form.Group1[1]},[/color] error: "Please select a state for delivery"},
    {field: form.Dday, [color="green"]data: {dependsOn: form.Group1[1]},[/color] error: "Please select a date for delivery"},
    {field: form.Dmonth, [color="green"]data: {dependsOn: form.Group1[1]},[/color] error: "Please select a month for delivery"},
    {field: form.Dyear, [color="green"]data: {dependsOn: form.Group1[1]},[/color] error: "Please select a year for delivery"}
];

And now the delDrops() function can also be removed.

That brings us to the last function: checkText(), to which we can apply the same dependency solution.


var notBlanks = [
    ...
    {field: form.message, [color="green"]data: {dependsOn: form.card},[/color] error: "Please enter your personal message in the text area"}
];

Bringing things Together

So now the validateForders() function contains two main things. It contains a series of arrays at the start:


var notBlanks = [
    ...
];
var numericValues = [
    ...
];
var notDefaultDropdowns = [
    ...
];
var emailValues = [
    ...
];

And a series of validating calls below them:



if (validate(notBlanks, nBlank) === false) {
    isValid = false;
}
if (validate(numericValues, numVal) === false) {
    isValid = false;
}
if (validate(notDefaultDropdowns, cDefault) === false) {
    isValid = false;
}
if (validate(emailValues, valEmail) === false) {
    isValid = false;
}

Wouldn’t it be great if we didn’t need those duplicate validation calls?

What we can do about that is to have a final array called fieldsToValidate, in which we associate each array with the function to validate it with.


groupsToValidate = [
    {group: notBlanks, validator: nBlank},
    {group: numericValues, validator: numVal},
    {group: notDefaultDropdowns, validator: cDefault},
    {group: emailValues, validator: valEmail},
    
];

This is also a good time to rename the functions to other names, if you want to make them consistent and expressive of what they do.

Now we can just loop through each of those groups, making the same call to the validate function.


for (i = 0; i < groupsToValidate.length; i += 1) {
    if (validate(groupsToValidate.group, groupsToValidate.validator) === false) {
        isValid = false;
    }
}

That will show an error message for each type of validation.

If you only want to show one error message and then stop, that can be easily done too, by breaking out of the loop when an error is detected.


for (i = 0; i < groupsToValidate.length; i += 1) {
    if (validate(groupsToValidate.group, groupsToValidate.validator) === false) {
        isValid = false;
        [color="green"]break;[/color]
    }
}

The resulting code from all of these updates is:


function validateForders(form) {
    var isValid = true,
        notBlanks, numericValues, notDefaultDropdowns, emailValues,
        groupsToValidate,
        i;
    
    notBlanks = [
        {field: form.fName, error: "Please enter your name"},
        {field: form.address, error: "Please enter your address"},
        {field: form.suburb, error: "Please enter your suburb"},
        {field: form.postcode, data: {focus: true}, error: "Please enter a valid Post Code"},
        {field: form.homeP, data: {focus: true}, error: "Please enter a valid Home phone number"},
        {field: form.dStreet, data: {dependsOn: form.Group1[1]}, error: "Please enter your delivery address"},
        {field: form.dSuburb, data: {dependsOn: form.Group1[1]}, error: "Please enter your delivery suburb or town"},
        {field: form.Dpostcode, data: {dependsOn: form.Group1[1]}, error: "Please enter valid delivery postcode"},
        {field: form.message, data: {dependsOn: form.card}, error: "Please enter your personal message in the text area"}
    ];
    numericValues = [
        {field: form.postcode, data: {size: 4, focus: true}, error: "Please enter a valid Post Code"},
        {field: form.homeP, data: {size: 10, focus: true}, error: "Please enter a valid Home phone number"},
        {field: form.workP, data: {size: 10, focus: true}, error: "Please enter a valid Work phone number"},
        {field: form.faxP, data: {size: 10, focus: true}, error: "Please enter a valid fax number"},
        {field: form.Dpostcode, data: {dependsOn: form.Group1[1], size: 4}, error: "Please enter a valid delivery postcode"}
    ];
    notDefaultDropdowns = [
        {field: form.aState, error: "Please select a State"},
        {field: form.cCard, error: "Please choose your card type"},
        {field: form.emonth, error: "Please select card expiry month"},
        {field: form.eyear, error: "Please select card expiry year"},
        {field: form.basket, error: "Please make a basket choice"},
        /*{field: form.quan, error: "You have chosen 1 basket"},*/
        {field: form.dState, data: {dependsOn: form.Group1[1]}, error: "Please select a state for delivery"},
        {field: form.Dday, data: {dependsOn: form.Group1[1]}, error: "Please select a date for delivery"},
        {field: form.Dmonth, data: {dependsOn: form.Group1[1]}, error: "Please select a month for delivery"},
        {field: form.Dyear, data: {dependsOn: form.Group1[1]}, error: "Please select a year for delivery"}
    ];
    emailValues = [
        {field: form.eMail, error: "Not a valid e-mail address"}
    ];
    groupsToValidate = [
        {group: notBlanks, validator: nBlank},
        {group: numericValues, validator: numVal},
        {group: notDefaultDropdowns, validator: cDefault},
        {group: emailValues, validator: valEmail},
        
    ];
    
    for (i = 0; i < groupsToValidate.length; i += 1) {
        if (validate(groupsToValidate.group, groupsToValidate.validator) === false) {
            valid = false;
            break;
        }
    }
    
    return isValid;
}
function validate(fieldsToCheck, func) {
    var field,
        data,
        error,
        i;
    for (i = 0; i < fieldsToCheck.length; i += 1) {
        field = fieldData[i].field;
        data = fieldData[i].data;
        error = fieldData[i].error;
        if (checkDependency(data.dependsOn) && func(field, error, data) === false) {
            alert(error);
            if (data.focus === true) {
                field.focus();
            }
            return false;
        }
    }
}
function checkDependency(field) {
    if (field.type === 'checkbox') {
        return field.checked;
    }
    if (field.type === 'radio') {
        return field.selected;
    }
    if (field.type === 'text') {
        return (field.value.length > 0 && field.value !== field.defaultValue);
    }
}
function nBlank(field) {
    return (field.value.length == 0 || field.value == field.defaultValue);
}
function cDefault(field) {
    return (field.selectedIndex === 0);
}
function numVal(field, data) {
    return (isNum(field.value) || field.value.length != data.size);
}
function valEmail(field) {
    var email = field.value
    var at = email.indexOf("@");
    var dot = email.lastIndexOf(".");
    return ((at >= 1 && dot - at >= 2 && str.length - dot >= 2) === false);
}
3

Surprised no comments. Top tutorial!!

You mentioned in Help Wanted about having stickies for common problems, and the above is an ideal candidate.

There’s something similar to this covered in Javascript Design Patterns ‘The Strategy Pattern’.

Will certainly be considering the [code review]. Exactly what I’ve been looking for.

Thanks

RLM