current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 7 down vote favorite
1

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?

share|improve this question
    
Why are you using IDs to select form inputs? You can use name attributes and then use the form API like somevalue = +document.someform.somecheckbox.checked. –  Dagg 3 hours ago

2 Answers 2

up vote 8 down vote accepted

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.

share|improve this answer
    
That works great!!! I am going to implement in the other area that I copied and pasted code. Thank you very much for the help!! –  Jim 7 hours ago
1  
I'd say it should just be $("#" + guide).is(":checked"), without the ? 1 : 0. It's redundant: you have the correct truthy or falsy value already –  Claudiu 5 hours ago
1  
@Claudiu My assumption is that MySQL needs 1 or 0 –  konijn 4 hours ago
    
@Claudiu I need to save a value in MySQL so that when I call the same page again the box will get checked. –  Jim 2 hours ago
up vote 1 down vote

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??

There are a few ways to think about this. Technically speaking, all Javascript values are objects (even numbers, strings, etc.), so there's no way to write code without using objects. Classes are different: they're not built-in like in some other languages, they're more like a "design pattern" which you may or may not choose to follow.

In terms of your situation, @konijn is dead right that your code is violating "DRY". However, I would avoid for-loops since they're a procedural way to solve the problem (the solution depends on steps being executed in the correct order), which also breaks encapsulation (this.list must expose its internals, so that the loop can extract the elements; this forces us to create a whole new Guides object to hide them again).

I think a nicer way to think of your problem is as a map, turning a set of values (DOM element IDs) into a corresponding set of values (boolean check-box results) based on a transformation function (look up the DOM element with that ID and see if it's checked). This changes the interface a little, since we're dealing with sets of values (actually, arrays):

['am', 'fol', 'tich', 'book', 'neb', 'ster', 'byte', 'ing'].map(
  function(id) {
    return $('#' + id + 'Old').is(':checked');
  });

The return value is an array of booleans in the same order as the strings were.

An alternative which retains the names without splitting up the "sets", is to use an object as a key:value store:

var results = {};
['am', 'fol', 'tich', 'book', 'neb', 'ster', 'byte', 'ing'].map(
  function(id) {
    results[id + 'OldGuide'] = $('#' + id + 'Old').is(':checked');
  });

Here the anonymous function will have access to results due to lexical scope.

share|improve this answer
1  
Actually not everything is an object in JavaScript. Strings and numbers are not objects, but they get converted into String and Number objects respectively when you try to access them as objects, eg (4).toString() is the same as (new Number(4)).toString() –  Claudiu 2 hours ago

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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