Support for an interactive form with multiple tabs

Question

I just failed in a JavaScript test and I would really appreciate some comments of how I can improve so I can keep learning.

The test was about making a form interactive, where the code should do things like changing to the next tab while showing not visible content, validate email, etc.

I have received the following feedback to consider:

[-] caching (no jquery selector is cached)----
[-] performance optimization (nope, usage of jquery each instead of a native loop)---
[-] reusable code (hardly, as commented above)---
[+] clean code and good structure---

[ ] Extra points for applied design pattern

$("<link rel='stylesheet' href='css/ui-lightness/jquery-ui-1.10.1.custom.css' type='text/css' media='screen' />").insertAfter("[type='text/css']");   // CSS de Jquery UI

function isValidEmailAddress(emailAddress) {
    var pattern = new RegExp(/^((([a-z]|\d|[!#\$%&'\*\+\-\/=\?\^_`{\|}~]|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF])+(\.([a-z]|\d|[!#\$%&'\*\+\-\/=\?\^_`{\|}~]|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF])+)*)|((\x22)((((\x20|\x09)*(\x0d\x0a))?(\x20|\x09)+)?(([\x01-\x08\x0b\x0c\x0e-\x1f\x7f]|\x21|[\x23-\x5b]|[\x5d-\x7e]|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF])|(\\([\x01-\x09\x0b\x0c\x0d-\x7f]|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF]))))*(((\x20|\x09)*(\x0d\x0a))?(\x20|\x09)+)?(\x22)))@((([a-z]|\d|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF])|(([a-z]|\d|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF])([a-z]|\d|-|\.|_|~|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF])*([a-z]|\d|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF])))\.)+(([a-z]|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF])|(([a-z]|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF])([a-z]|\d|-|\.|_|~|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF])*([a-z]|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF])))\.?$/i);
    return pattern.test(emailAddress);
};


function setTabOn(stepNumber) {
    stepNumber = parseInt(stepNumber);
    $("#step" + stepNumber + "_tab").addClass("on");
    $("#step" + stepNumber + "_tab").click(function() {
        $(".step").hide();
        $("#step" + stepNumber).show();
        $("#steps a.active").removeClass("active");
    }); 
}

function nextStep(stepNumber) {
    stepNumber = parseInt(stepNumber) + 1;
    $(".step").hide();
    $("ol#steps li a").removeClass("active");
    $("#step" + stepNumber).show();
    $("#step" + stepNumber + "_tab").addClass("active");
}

$(document).ready(function() {

    $(".back").click(function() {
        var idBack = $(this).attr("id");
        var idBack = idBack.match(/\d/) - 1;
        $(".step").hide();
        $("#step" + idBack).show(); 
    });

    $("#step2_confirm_button").click(function() {
        $("#step2_confirm").dialog("destroy");
        nextStep(2);
        setTabOn(2);
    });

    $("#step2_cancel_button").click(function() {
        $("#step2_confirm").dialog("destroy");
    });

    $(".step").hide();
    $("#step1").show();
    $("#step1_tab").addClass("active");
    $("#step2_confirm").hide();

    $("#step1_next").click(function() { 

            var flagFirstStepError = 0;
            var flagFirstStepEmpty = 0;
            var flagFirstStepEmail = 0;

            $("#step1_errors").html("");

            $(".required").each(function() {    
                if ($(this).val() == "") {
                    flagFirstStepError = 1;
                    return false
                }
                else {
                    flagFirstStepEmpty = 1;
                }

            }); // end each                                                      

            if (flagFirstStepError == 1) {
                $("#step1_errors").append("We are sorry, but you need to enter all the required fields in order to proceed. The following information is missing: <br /><ol>");
                $(".required").each(function() {                                                             
                    if ($(this).val() == "") {
                         var labelId = $(this).attr("id");
                         var y = $("#step1 label[for=" + labelId + "]").html();
                         $("#step1_errors").append("<li>You must fill the " + y + " field</li>");
                    }
                }); // end each
                $("#step1_errors").append("</ol>");
            }

        var userEmail = $("#email").val();
        if (!isValidEmailAddress(userEmail)) {
                $("#step1_errors").append("<p> You must provide a valid email adress");
                flagFirstStepEmail = 0;
        }
        else {
            flagFirstStepEmail = 1;
        }

        if (flagFirstStepEmail + flagFirstStepEmail == 2) {
            nextStep(1);
            setTabOn(1);
        }

    });  //end click                                                         


        $("#step2_next").click(function() { 
                $("#confirm_list li").remove();
                $("#share_list li label").find("input:checked").each(function() {   
                    var x = $(this).val();                                                                                                          
                    $("#confirm_list").append("<li>" + x + "</li>");
                });

            $("#step2_confirm").dialog();

        });


});

Community · Accepted Answer · 2017-05-23 12:40:48Z

Initial notes

[-] caching (no jquery selector is cached)

You should be creating variables that store the elements you're selecting with jQuery. The reason for this is that when you call $("#step" + stepNumber + "_tab") for example, jQuery will determine what type of selector it is (id), and them call document.getElementById("step" + stepNumber + "_tab"). This is particularly important when you're selecting multiple elements.
[-] performance optimization (nope, usage of jquery each instead of a native loop)

So you used $.each, what is the problem with that? Well it is doing a tonne of other stuff just so you can have some nice convenient syntax. For systems you're trying to optimise you should probably stay away from it, prefer this:
```
var required = $(".required");
for (var i = 0; i < required.length; i++) {
    // object is required[i]
}
```
As you can see from this jsPeft, a native for loop is much faster than $.each. You can see the source code for each at this StackOverflow post.
[-] reusable code (hardly, as commented above)

The main issue here is that your document ready is a whopping 85 lines long. You should be splitting it up into logical groupings and placing it in functions. This is something that a lot of new programmers have trouble with and I never really understood until I saw a great example. Consider a function of 85 lines of code vs:
```
$(document).ready(function () {
    setupItemA();
    setupItemB();
    setupItemC();
    setupItemD();
});
```
This is much more readable, modular and reusable.
[+] clean code and good structure

This ties in a lot with above point 'reusable code' in my opinion. I'm assume your teacher was marking this point on consistency, indentation, appropriate calls, etc. which it seems to do pretty well.
[] Extra points for applied design pattern

This would depend on what design patterns you have been learning in your course.

Code walkthrough

I'll now go through my implementation, line-by-line.

// Ideally this should go in your HTML page, I assume it wasn't an option though.
$("<link rel='stylesheet' href='css/ui-lightness/jquery-ui-1.10.1.custom.css' type='text/css' media='screen' />").insertAfter("[type='text/css']");   // CSS de Jquery UI

// This regex wouldn't be acceptable in a lot of companies as it's huge and difficult
// to read. A link to the source would be good in this situation.
function isValidEmailAddress(emailAddress) {
    var pattern = new RegExp(/^((([a-z]|\d|[!#\$%&'\*\+\-\/=\?\^_`{\|}~]|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF])+(\.([a-z]|\d|[!#\$%&'\*\+\-\/=\?\^_`{\|}~]|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF])+)*)|((\x22)((((\x20|\x09)*(\x0d\x0a))?(\x20|\x09)+)?(([\x01-\x08\x0b\x0c\x0e-\x1f\x7f]|\x21|[\x23-\x5b]|[\x5d-\x7e]|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF])|(\\([\x01-\x09\x0b\x0c\x0d-\x7f]|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF]))))*(((\x20|\x09)*(\x0d\x0a))?(\x20|\x09)+)?(\x22)))@((([a-z]|\d|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF])|(([a-z]|\d|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF])([a-z]|\d|-|\.|_|~|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF])*([a-z]|\d|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF])))\.)+(([a-z]|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF])|(([a-z]|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF])([a-z]|\d|-|\.|_|~|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF])*([a-z]|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF])))\.?$/i);
    return pattern.test(emailAddress);
};

// If your spacing is more consistent, your code will look cleaner. More than one 
// line in a row can make code look messy.

function setTabOn(stepNumber) {
    // This conversion is redundant, you're converting an int to an int and then using
    // it in a string. JavaScript does conversions like this implicitly anyhow
    //stepNumber = parseInt(stepNumber);

    // Caching
    var tab = $("#step" + stepNumber + "_tab");
    tab.addClass("on");
    tab.click(function() {
        $(".step").hide();
        $("#step" + stepNumber).show();
        $("#steps a.active").removeClass("active");
    }); 
}

function nextStep(stepNumber) {
    stepNumber = parseInt(stepNumber) + 1;
    $(".step").hide();
    // You don't need to include the ol in 'ol#steps', it makes your code more specific
    // and thus less reusable.
    $("#steps li a").removeClass("active");
    $("#step" + stepNumber).show();
    $("#step" + stepNumber + "_tab").addClass("active");
}

$(document).ready(function() {
    // Your indentation went a little funny in your document ready

    $(".back").click(backClick);
    $("#step2_confirm_button").click(step2Confirm);
    $("#step2_cancel_button").click(step2Cancel);
    $("#step1_next").click(step1NextClick);
    $("#step2_next").click(step2NextClick);

    // Shuffled around in better order
    $(".step").hide();
    $("#step1").show();
    $("#step1_tab").addClass("active");
    $("#step2_confirm").hide();
});

function step2Confirm() {
    $("#step2_confirm").dialog("destroy");
    nextStep(2);
    setTabOn(2);
}

function step2Cancel() {
    $("#step2_confirm").dialog("destroy");
}

function backClick = function() {
    // You defined idBank twice here?
    //var idBack = $(this).attr("id");
    //var idBack = idBack.match(/\d/) - 1;
    // Use this.id instead of the jQuery alternative, it's much faster
    var idBack = this.id
    // I'm not sure what your regex was trying to do
    $(".step").hide();
    $("#step" + idBack).show(); 
};

function step1NextClick() { 
    var flagFirstStepError = 0;
    var flagFirstStepEmpty = 0;
    var flagFirstStepEmail = 0;

    $("#step1_errors").html("");

    var required = $(".required");
    for (var i = 0; i < required.length; i++) {
        if ($(this).val() == "") {
            flagFirstStepError = 1;
            // Missing semi-colon
            return false;
        // Place } else { all on same line
        } else {
            flagFirstStepEmpty = 1;
        }
    }

    if (flagFirstStepError == 1) {
        showError(this, required);
    }

    var userEmail = $("#email").val();
    if (!isValidEmailAddress(userEmail)) {
        // Include a closing tag, address spelt wrong ;)
        $("#step1_errors").append("<p>You must provide a valid email address</p>");
        flagFirstStepEmail = 0;
    } else {
        flagFirstStepEmail = 1;
    }

    // Never do an operation of the left-hand side of an if
    if (flagFirstStepEmail  == 1 && flagFirstStepEmail == 1) {
        nextStep(1);
        setTabOn(1);
    }
}

function showError(sender, required) {
    // Only append the string once, keep in a variable
    var message = "We are sorry, but you need to enter all the required fields in order to proceed. The following information is missing: <br /><ol>";
    for (var i = 0; i < required.length; i++) {                                                      
        if ($(sender).val() == "") {
            var y = $("#step1 label[for=" + sender.id + "]").html();
            message += "<li>You must fill the " + y + " field</li>";
        }
    }
    message += "</ol>";
    $("#step1_errors").append(message);
}

// I moved your events down here to reduce indentation, the size of document ready, and
// improve readability
function step2NextClick() { 
    $("#confirm_list li").remove();
    $("#share_list li label").find("input:checked").each(function() {   
        var x = $(this).val();                                                                                                          
        $("#confirm_list").append("<li>" + x + "</li>");
    });
    $("#step2_confirm").dialog();
};

Ryan Erickson · Accepted Answer · 2013-03-22 02:52:38Z

[-] caching (no jquery selector is cached)----
Your code:

$("#step" + stepNumber + "_tab").addClass("on");
$("#step" + stepNumber + "_tab").click(function() { ...

Revised:

var stepTab = $("#step" + stepNumber + "_tab");
stepTab.addClass("on");
stepTab.click(function() { ...

Storing the results of a selector will make sure you don't have to search the DOM for the "#step" + stepNumber + "_tab" element each time.

[-] performance optimization (nope, usage of jquery each instead of a native loop)---
Original:

$(".required").each(function() {    
    if ($(this).val() == "") {
        flagFirstStepError = 1;
        return false
    }
    else {
        flagFirstStepEmpty = 1;
    }

});

Revised:

var requiredElems = $(".required"), length = requireElems.length;
for(var i = 0; i < length; i++){
    var requiredElem = $(requiredElems[i]);   
    if (requiredElem.val() == "") {
        flagFirstStepError = 1;
        return false
    }
    else {
        flagFirstStepEmpty = 1;
    }
}

The performance can be significantly slower using $.each because it needs to create a function and scope for each iteration where as the generic for loop has a lot less overhead and should be used in simple cases like this.

Webritos · Accepted Answer · 2013-03-24 02:28:11Z

This is an optimized version that I just made, if someone finds it useful

window.userProgress = 0;
var stepContent;
var stepTab;    
var backButtons;
var nextButtons;
var step2Confirm;

// CSS of Jquery UI
$("<link rel='stylesheet' href='css/ui-lightness/jquery-ui-1.10.1.custom.css' type='text/css' media='screen' />").insertAfter("[type='text/css']");   

// Email validation
function isValidEmailAddress(stepErrors) {
    var userEmail = $("#email").val();
    var flagStepEmail = 0;

    var pattern = new RegExp(/^((([a-z]|\d|[!#\$%&'\*\+\-\/=\?\^_`{\|}~]|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF])+(\.([a-z]|\d|[!#\$%&'\*\+\-\/=\?\^_`{\|}~]|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF])+)*)|((\x22)((((\x20|\x09)*(\x0d\x0a))?(\x20|\x09)+)?(([\x01-\x08\x0b\x0c\x0e-\x1f\x7f]|\x21|[\x23-\x5b]|[\x5d-\x7e]|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF])|(\\([\x01-\x09\x0b\x0c\x0d-\x7f]|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF]))))*(((\x20|\x09)*(\x0d\x0a))?(\x20|\x09)+)?(\x22)))@((([a-z]|\d|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF])|(([a-z]|\d|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF])([a-z]|\d|-|\.|_|~|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF])*([a-z]|\d|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF])))\.)+(([a-z]|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF])|(([a-z]|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF])([a-z]|\d|-|\.|_|~|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF])*([a-z]|[\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF])))\.?$/i);

    if (!pattern.test(userEmail)) {
        var message = "<p> You must provide a valid email address</p>"; 
        printStepMessages(message, stepErrors);
        flagStepEmail = 1;
    }    
    return flagStepEmail;
};

// Set into the variable "userProgress" the reached step by the user
function changeUserProgress(button) {
    userProgress = parseInt(button.id.match(/\d/)) + 1;
}

// Checks whether the user is allowed to move further
function checkUserPermision(stepNumber) {
    if (isNaN(stepNumber)) {
        stepNumber = parseInt(stepNumber.id.match(/\d/)) + 1;
    }
    if (stepNumber <= userProgress ) {
        userMovement(stepNumber);
    }
}

// Hides and shows step content
function userMovement(stepNumber) {
    var contentToShow;
    changeTab(stepNumber);
    stepContent.hide();
    for (var i = 0; i < stepContent.length; i++) {
        contentToShow = stepContent[i];
        if(contentToShow.id == "step" + stepNumber) {
            contentToShow.style.display='block';    
        }
        }
}

// Sets the "active" and "on" classes on tabs
function changeTab(stepNumber) {
    var stepTabNumber;
    stepTab.removeClass("active");
    stepTab.removeClass("on");
    for (var i = 0; i < stepTab.length; i++) {
  var stepTabNumber = stepTab[i];
    var stepTabNumberId =  stepTabNumber.id.match(/\d/);
    if(stepTabNumberId == stepNumber) {
        stepTabNumber = $(stepTabNumber);
        stepTabNumber.addClass("active");   
    }
    else if(stepTabNumberId <= userProgress) {
        stepTabNumber = $(stepTabNumber);
        stepTabNumber.addClass("on");   
    }
    }   
}

// Shows messages to the user
function printStepMessages(message, stepMessageContainer, messageIntro, requiredElements) {
    if(messageIntro === undefined) {
      messageIntro = '';
  }
    if(requiredElements === undefined) {
    requiredElements = '';
  }
    stepMessageContainer.prepend(messageIntro);
    stepMessageContainer.append(message);
}       

// Checks if the required fields in Step 1 are filled and sets warning messages if not
function firstTabCheck(requiredFirstStepElements, step1Errors) {
    var message = "";
    var fieldElement;
    var firstStepEmpty = 0;
    var messageIntro = "We are sorry, but you need to enter all the required fields in order to proceed. The following information is missing: <br /><ol>";
    for (var i = 0; i < requiredFirstStepElements.length; i++) {
        var requiredFirstStepElement = requiredFirstStepElements[i];
        if (requiredFirstStepElement.value == "") {
            fieldElement = $("#step1 label[for=" + requiredFirstStepElement.id + "]").html();
            message += "<li>You must fill the " + fieldElement + " field</li>";
            firstStepEmpty += 1;
        }
    }
    message += "</ol>";
    if (firstStepEmpty > 0) {
        printStepMessages(message, step1Errors, messageIntro, requiredFirstStepElements);
    }   
    return firstStepEmpty;
}

// Sets message of confirmation for social networks 
function secondTabCheck(shareListLabel, socialConfirmList) {
    messaje = "<li>" + shareListLabel + "</li>";
    printStepMessages(messaje, socialConfirmList);
}

$(document).ready(function() {

    stepContent = $(".step");
    stepTab = $("#steps").find("a");    
    backButtons = $(".back");
    nextButtons = $(".next");
    step2Confirm = $("#content #step2_confirm");
    var requiredFirstStepElements = $(".required"); 
    var socialConfirmList = $("#confirm_list");
    var step1Errors = $("#step1_errors");
    var shareListLabels = $("#share_list li label");

    stepContent.hide();
    step2Confirm.hide();
    $("#content #step1").show();
    $("#content #step1_tab").addClass("active");

    $("#step1_next").click(function() { 
        step1Errors.html("");
        var firstStepEmpty = firstTabCheck(requiredFirstStepElements, step1Errors);
        var firstStepEmail = isValidEmailAddress(step1Errors);
        if (firstStepEmpty == 0 && firstStepEmail  == 0) {
        changeUserProgress(this);
        }
    }); 

    $("#step2_next").click(function() {
        var shareListLabel;
        if(userProgress < 3) {                                                                  
            $("#confirm_list li").remove();
            var shareListLabelsCheck = shareListLabels.find("input:checked");   
            for (var i = 0; i < shareListLabelsCheck.length; i++) {     
                shareListLabel = $(shareListLabelsCheck[i]).val();
                secondTabCheck(shareListLabel, socialConfirmList);
            }       
            if(socialConfirmList.children().length > 0) {
                step2Confirm.dialog();
            }
            else {
                changeUserProgress(this);
            }
        }
    });

    $("#step2_confirm_button").click(function() {
        step2Confirm.dialog("destroy");
        changeUserProgress(this);
        checkUserPermision(this);
    });

    $("#step2_cancel_button").click(function() {
        step2Confirm.dialog("destroy");
    });

    stepTab.click(function() {                                           
        var idElement = parseInt(this.id.match(/\d/));  
        checkUserPermision(idElement);
    });

    nextButtons.click(function() {                                           
        var idElement = parseInt(this.id.match(/\d/)) + 1;  
        checkUserPermision(idElement);
    });

    backButtons.click(function() {
        var idElement = parseInt(this.id.match(/\d/)) - 1;
        userMovement(idElement);
    });

});

Stack Exchange Network

Support for an interactive form with multiple tabs

3 Answers 3

Initial notes

Code walkthrough

Your Answer

Not the answer you're looking for? Browse other questions tagged
javascript
performance
jquery
form
or ask your own question.

Hot Network Questions

Support for an interactive form with multiple tabs

3 Answers 3

Initial notes

Code walkthrough

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged javascriptperformancejqueryform or ask your own question.

Related

Hot Network Questions

Not the answer you're looking for? Browse other questions tagged
javascript
performance
jquery
form
or ask your own question.