patternjavascriptMinor
Determining if a check box has been checked or not
Viewed 0 times
determiningbeenhascheckedcheckboxnot
Problem
I have a block of code that checks to see if a check box is checked or not, and assigns a value to a variable for saving to a MySQL table. There are approximately 8 of these boxes.
I am using something similar for another set of check boxes later on in the project as well. As it sits, this works, but it is ugly. I feel that there must be a better way to write this that will be easier to maintain, and without having to repeat it later on. Should I be using a class or objects?
if ($("#amOld").is(":checked")) {
var amOldGuide=1;
} else {
var amOldGuide=0;
}
if ($("#folOld").is(":checked")) {
var folOldGuide=1;
} else {
var folOldGuide=0;
}
if ($("#tichOld").is(":checked")) {
var tichOldGuide=1;
} else {
var tichOldGuide=0;
}
if ($("#bookOld").is(":checked")) {
var bookOldGuide=1;
} else {
var bookOldGuide=0;
}
if ($("#nebOld").is(":checked")) {
var nebOldGuide=1;
} else {
var nebldGuide=0;
}
if ($("#sterOld").is(":checked")) {
var sterOldGuide=1;
} else {
var sterOldGuide=0;
}
if ($("#byteOld").is(":checked")) {
var byteOldGuide=1;
} else {
var byteOldGuide=0;
}
if ($("#ingOld").is(":checked")) {
var ingOldGuide=1;
} else {
var ingOldGuide=0;
}I am using something similar for another set of check boxes later on in the project as well. As it sits, this works, but it is ugly. I feel that there must be a better way to write this that will be easier to maintain, and without having to repeat it later on. Should I be using a class or objects?
Solution
A common question, your question is not so much ugly as it is repetitive. Because of that it violate DRY.
You basically have copy pasted the same structure over and over :
If you turn these
Or, if you do not wish to expose the list, you could go for something more functional (hat tip to @Warbo):
I also threw in a ternary for good measure since I don't like one liners in curly braces.
You can then access
If you need to refresh the values inside
Finally, there is a performance hit you should be aware of, I am not caching any of the jQuery lookups. You could have
You basically have copy pasted the same structure over and over :
if ($("#XXXOld").is(":checked")) {
var XXXOldGuide=1;
} else {
var XXXOldGuide=0;
}If you turn these
vars into the properties of a class, then you could do these assignments in a loop over strings. Something like this:function Guides(){
this.update();
}
Guides.prototype.list = ['amOld','folOld','tichOld','bookOld','nebOld','sterOld','byteOld','ingOld'];
Guides.prototype.update = function(){
for( var i = 0 ; i < this.list.length ; i++ ){
var guide = this.list[i];
this[ guide + 'Guide' ] = $("#" + guide ).is(":checked") ? 1 : 0;
}
}Or, if you do not wish to expose the list, you could go for something more functional (hat tip to @Warbo):
function Guides(){
this.update();
}
Guides.prototype.update = function(){
var list = ['amOld','folOld','tichOld','bookOld','nebOld','sterOld','byteOld','ingOld'],
self = this;
list.forEach( function( guide ){
self[ guide + 'Guide' ] = $("#" + guide ).is(":checked") ? 1 : 0;
});
}I also threw in a ternary for good measure since I don't like one liners in curly braces.
You can then access
bookOldGuide like this:var guides = new Guides();
console.log( guides.bookOldGuide );If you need to refresh the values inside
guides ( because you are about to save data for example ), you simply need toguides.update();Finally, there is a performance hit you should be aware of, I am not caching any of the jQuery lookups. You could have
update create a cache of the lookups if performance becomes an issue.Code Snippets
if ($("#XXXOld").is(":checked")) {
var XXXOldGuide=1;
} else {
var XXXOldGuide=0;
}function Guides(){
this.update();
}
Guides.prototype.list = ['amOld','folOld','tichOld','bookOld','nebOld','sterOld','byteOld','ingOld'];
Guides.prototype.update = function(){
for( var i = 0 ; i < this.list.length ; i++ ){
var guide = this.list[i];
this[ guide + 'Guide' ] = $("#" + guide ).is(":checked") ? 1 : 0;
}
}function Guides(){
this.update();
}
Guides.prototype.update = function(){
var list = ['amOld','folOld','tichOld','bookOld','nebOld','sterOld','byteOld','ingOld'],
self = this;
list.forEach( function( guide ){
self[ guide + 'Guide' ] = $("#" + guide ).is(":checked") ? 1 : 0;
});
}var guides = new Guides();
console.log( guides.bookOldGuide );guides.update();Context
StackExchange Code Review Q#63661, answer score: 8
Revisions (0)
No revisions yet.