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

Determining if a check box has been checked or not

Submitted by: @import:stackexchange-codereview··
0
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.

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 ($("#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 to

guides.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.