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();
 });
}); 

Answer 1

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();
};

Answer 2

[-] 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.

Answer 3

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);
 });
});