Hours of operation input form

Question

I need some comments on the code that I "assembled". I want to know if it is efficient because I intend to use it on a live site.

jsFiddle

HTML:

<div id="hourForm">
<div id="Monday-Friday" class="day"></div>
<div id="Saturday" class="day"></div>
<div id="Sunday" class="day"></div>
</div>

jQuery:

$('.day').each(function() {
 var day = $(this).attr('id');
 $(this).append('<div id="label">' + day + ': </div>')
 .append('<select id="' + day + 'FromH" name="' + day + 'FromH" class="hour from"></select>')
 .append('<select id="' + day + 'FromM" name="' + day + 'FromM" class="min from"></select>')
 .append(' to <select id="' + day + 'ToH" name="' + day + 'ToH" class="hour to"></select>')
 .append('<select id="' + day + 'ToM" name="' + day + 'ToM" class="min to"></select>')
 .append(' <input type="checkbox" id="closed_' + day + '" name="closed" value="closed" class="closed" /> <label for="closed_' + day + '">Closed</label>');
});
$('.hour').each(function() {
 for (var h = 0; h < 24; h++) {
 $(this).append('<option value="' + h + '">' + h + '</option>');
 }
 $(this).filter('.from').val('6');
 $(this).filter('.to').val('22');
});
$('.min').each(function() {
 var min = [':00', ':15', ':30', ':45'];
 for (var m = 0; m < min.length; m++) {
 $(this).append('<option value="' + min[m] + '">' + min[m] + '</option>');
 }
 $(this).val(':00');
 $(this).filter('.from').val(':30');
});
$('input').change(function() {
 if ($(this).filter(':checked').val() == "closed") {
 $(this).siblings('select').attr('disabled', true);
 } else {
 $(this).siblings('select').attr('disabled', false);
 }
});
$('#Sunday .closed').val(["closed"]).siblings('select').attr('disabled', true);
function displayVals() {
 var MondayFridayFromHValues = $("#Monday-FridayFromH").val();
 var MondayFridayFromMValue = $("#Monday-FridayFromM").val();
 var MondayFridayToHValue = $("#Monday-FridayToH").val();
 var MondayFridayToMValue = $("#Monday-FridayToM").val();
 var MondayFridayClosedValue = $("#closed_Monday-Friday").filter(":checked").val();
 if (MondayFridayClosedValue == "closed") {
 var MondayFridayOpen = "Closed";
 }
 else {
 var MondayFridayOpen = MondayFridayFromHValues + MondayFridayFromMValue + "-" + MondayFridayToHValue + MondayFridayToMValue;
 }
 var SaturdayFromHValues = $("#SaturdayFromH").val();
 var SaturdayFromMValue = $("#SaturdayFromM").val();
 var SaturdayToHValue = $("#SaturdayToH").val();
 var SaturdayToMValue = $("#SaturdayToM").val();
 var SaturdayClosedValue = $('#closed_Saturday').filter(':checked').val();
 if (SaturdayClosedValue == "closed") {
 var SaturdayOpen = "Closed";
 }
 else {
 var SaturdayOpen = SaturdayFromHValues + SaturdayFromMValue + "-" + SaturdayToHValue + SaturdayToMValue;
 }
 var SundayFromHValues = $("#SundayFromH").val();
 var SundayFromMValue = $("#SundayFromM").val();
 var SundayToHValue = $("#SundayToH").val();
 var SundayToMValue = $("#SundayToM").val();
 var SundayClosedValue = $('#closed_Sunday').filter(':checked').val();
 if (SundayClosedValue == "closed") {
 var SundayOpen = "Closed";
 }
 else {
 var SundayOpen = SundayFromHValues + SundayFromMValue + "-" + SundayToHValue + SundayToMValue;
 }
 if (MondayFridayOpen == SaturdayOpen) {
 if (MondayFridayOpen == SundayOpen) {
 $("input:text[name='entry.7.group.other_option_']").val("Monday-Sunday: " + MondayFridayOpen);
 }
 else {
 $("input:text[name='entry.7.group.other_option_']").val("Monday-Saturday: " + MondayFridayOpen + ", Sunday: " + SundayOpen);
 }
 }
 else if (MondayFridayOpen != SaturdayOpen && SaturdayOpen == SundayOpen) {
 $("input:text[name='entry.7.group.other_option_']").val("Monday-Friday: " + MondayFridayOpen + ", Saturday-Sunday: " + SaturdayOpen);
 }
 else {
 $("input:text[name='entry.7.group.other_option_']").val("Monday-Friday: " + MondayFridayOpen + ", Saturday: " + SaturdayOpen + ", Sunday: " + SundayOpen);
 }
}
$("div#hourForm select").change(function() {
 $("input:radio[name='entry.7.group']:nth(3)").attr("checked", true);
});
$(":checkbox[name='closed']").change(function() {
 $("input:radio[name='entry.7.group']:nth(3)").attr("checked", true);
});
$("input:text[name='entry.7.group.other_option_']").click(function () {
 $("input:radio[name='entry.7.group']:nth(3)").attr("checked", true);
});
$("div#hourForm select").change(displayVals);
$(":checkbox[name='closed']").change(displayVals);
displayVals();​

• RoToRa: Hmm, at least the first part of your code ("building" select elements) shouldn't be done in JavaScript like that, but should be part of the static HTML, possibly generated using a server-side script.
• m1r0: @RoToRa - Can you help me do it?
• RoToRa: @m1r0 Help doing what? Generate the HTML server-side? That would depend on the language and templating engine you use. And it would be off topic here.

Answer 1

Some quick tips;

1. Look at caching $(this). You're regularly re-creating a jQuery object that doesn't change.

2. Adhere to the common JavaScript coding conventions; normally a variable that starts with a capital letter denotes a constructor (that should be invoked with new). Change all your SaturdayFromHValues etc variable names to saturdayFromHValues

3. As mentioned by @priteaes, you should save the HTML you're constructing to a string and write it to the element once. DOM manipulation is slow, and the browser will be re-paining the window with each append() you're using;

   $(this).append('<div id="label">' + day + ': </div>')
    .append('<select id="' + day + 'FromH" name="' + day + 'FromH" class="hour from"></select>')
    .append('<select id="' + day + 'FromM" name="' + day + 'FromM" class="min from"></select>')
   

   Becomes:

   var html = '';
   html += '<div id="label">' + day + ': </div>';
   html += '<select id="' + day + 'FromH" name="' + day + 'FromH" class="hour from"></select>';
   html += '<select id="' + day + 'FromM" name="' + day + 'FromM" class="min from"></select>';
   $(this).append(html);
   

• m1r0: Great answer Matt, thank you! Do you mean something like this: jsfiddle.net/m1r0/Ace6h/27 . How can I add the other each to the append(html)?
• Matt: @m1ro: I wouldn't worry toooo much about getting each each into exactly 1 append(). What I'd change next would be the appends() in the for loops as well: jsfiddle.net/Ace6h/29
• m1r0: Here is the final form: jsfiddle.net/m1r0/Ace6h/31 . I learned a lot. Thanks again for helping me out.

Answer 2

In the first and second each, I would build the entire HTML first (as a string), and then append() once.

Update:

I prefer something like this:

$('.day').each(function() { 
 var day = $(this).attr('id'); 
 var html = '<div id="label">' + day + ': </div>' +
 '<select id="' + day + 'FromH" name="' + day + 'FromH" class="hour from"></select>' +
 '<select id="' + day + 'FromM" name="' + day + 'FromM" class="min from"></select>' + 
 ' to <select id="' + day + 'ToH" name="' + day + 'ToH" class="hour to"></select>' +
 '<select id="' + day + 'ToM" name="' + day + 'ToM" class="min to"></select>' +
 ' <input type="checkbox" id="closed_' + day + '" name="closed" value="closed" class="closed" /> <label for="closed_' + 
 day + '">Closed</label>';
 $(this).append(html);
});