HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Setting periodicity in the week in Google Calendar

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thegoogleperiodicitysettingweekcalendar

Problem

I am trying to write a generic JavaScript subroutine to set the periodicity in the week:

var items = {"0": "none", "1":"Daily", "2": "Every weekday (Monday to Friday)", "3": "Weekly"};


I would like to improve this to make it more generic and readable if possible.

var callback = selectDay(idValue);​
createSelect(items, idValue, callback);


var createSelect = function (items, idValue) {
    var selElem = document.createElement("select");
    $.each(items, function (key, value) {
        var ov = document.createElement("option");
        ov.value = key;
        ov.appendChild(document.createTextNode(value));
        selElem.appendChild(ov);
    });
    $(idValue).prepend(selElem);
};


var selectDay = function (idValue, element) {
    var element = $(idValue);
    var childElements = idValue + '_';
    element.find('select').change(function () {
        if($(this).val() === '0') {
            element.find('label').hide();
            $(this).closest('div').find('input').attr("checked", false);
        }
        if($(this).val() === '1') {
            element.find('label').hide();
            $(this).closest('div').find('input').attr("checked", true);
        }
        if($(this).val() === '2') {
            element.find('label').show();
            for (var i = 0; i < 5; i += 1) {
                $(childElements + i).attr("checked", true);
            }
            for (var i = 5; i < 7; i += 1) {
                $(childElements + i).attr("checked", false);
            }
        }
        if($(this).val() === '3') {
            element.find('label').show();
            $(this).closest('div').find('input').attr("checked", false);
            $(childElements + (new Date()).getDay()).attr("checked", true);
        }
    });
};


var items = {"0": "none", "1":"Daily", "2": "Every weekday (Monday to Friday)", "3": "Weekly"};
var idValue = '#contest_data_updatePeriodicity_days';
createSelect(items, idValue);
selectDay(idValue);​


My q

Solution


  • Use prop() instead of attr().



  • See the first point.



  • You're declaring var selectDay = function(idValue, element), your element argument is probably a typo? You don't need it.



  • See the first point.



  • Use switch instead of all those if. Or at least cache the $(this).val() result.



  • See the first point.



  • You don't need element.find(), you could use the context (somehow cleaner).



Overall, here is some code to give you the idea:

var selectDay = function (idValue) {
    $('select', idValue).change(function() {
        switch ($(this).val()) {
            case '0':
                $('label', idValue).hide();

                // Use prop(), not attr().
                $(this).closest('div').find('input').prop('checked', true);
                break;
        }
    });
};


Now you may notice something: this doesn't have exactly the same behavior as your code. Guess why :-)

A friend of mine says that whenever you use a switch case, you can actually use a mapping object. This might be another way to do it. The mapping object would be something like:

var mapping = {
    '0': {
        'label': 'hide',
        'checked': false
    },
    '1': {
        'label': 'hide',
        'checked': true
    }
};


And then you code accordingly to this object.

Code Snippets

var selectDay = function (idValue) {
    $('select', idValue).change(function() {
        switch ($(this).val()) {
            case '0':
                $('label', idValue).hide();

                // Use prop(), not attr().
                $(this).closest('div').find('input').prop('checked', true);
                break;
        }
    });
};
var mapping = {
    '0': {
        'label': 'hide',
        'checked': false
    },
    '1': {
        'label': 'hide',
        'checked': true
    }
};

Context

StackExchange Code Review Q#16439, answer score: 4

Revisions (0)

No revisions yet.