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