patternjavascriptMinor
Setting periodicity in the week in Google Calendar
Viewed 0 times
thegoogleperiodicitysettingweekcalendar
Problem
I am trying to write a generic JavaScript subroutine to set the periodicity in the week:
I would like to improve this to make it more generic and readable if possible.
My q
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 ofattr().
- See the first point.
- You're declaring
var selectDay = function(idValue, element), yourelementargument is probably a typo? You don't need it.
- See the first point.
- Use
switchinstead of all thoseif. 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.