2
\$\begingroup\$

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();​
\$\endgroup\$
3
  • \$\begingroup\$ 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. \$\endgroup\$
    – RoToRa
    Apr 2 '12 at 10:19
  • \$\begingroup\$ @RoToRa - Can you help me do it? \$\endgroup\$
    – m1r0
    Apr 2 '12 at 12:25
  • \$\begingroup\$ @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. \$\endgroup\$
    – RoToRa
    Apr 5 '12 at 14:49
2
\$\begingroup\$

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);
    
\$\endgroup\$
3
  • \$\begingroup\$ 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)? \$\endgroup\$
    – m1r0
    Apr 2 '12 at 19:09
  • \$\begingroup\$ @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 \$\endgroup\$
    – Matt
    Apr 3 '12 at 10:28
  • \$\begingroup\$ Here is the final form: jsfiddle.net/m1r0/Ace6h/31 . I learned a lot. Thanks again for helping me out. \$\endgroup\$
    – m1r0
    Apr 3 '12 at 18:39
1
\$\begingroup\$

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);
});
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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