8
\$\begingroup\$

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?

\$\endgroup\$
1
  • \$\begingroup\$ 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. \$\endgroup\$
    – Dagg
    Sep 23 '14 at 17:35
8
\$\begingroup\$

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.

\$\endgroup\$
4
  • \$\begingroup\$ 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!! \$\endgroup\$
    – Jim
    Sep 23 '14 at 13:37
  • 1
    \$\begingroup\$ 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 \$\endgroup\$
    – Claudiu
    Sep 23 '14 at 15:56
  • 1
    \$\begingroup\$ @Claudiu My assumption is that MySQL needs 1 or 0 \$\endgroup\$
    – konijn
    Sep 23 '14 at 16:20
  • \$\begingroup\$ @Claudiu I need to save a value in MySQL so that when I call the same page again the box will get checked. \$\endgroup\$
    – Jim
    Sep 23 '14 at 18:53
2
\$\begingroup\$

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.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ 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() \$\endgroup\$
    – Claudiu
    Sep 23 '14 at 19:14
  • \$\begingroup\$ @Claudiu that's true from an "internal" perspective, ie. "according to the JS semantics, 'object' means anything derived from Object". From an "external" perspective we might say "according to OOP, 'object' means encapsulated data which I can call methods on", in which case the automatic conversion doesn't matter too much: (4).toString() is a method call, so 4 acts like an object according to OOP. \$\endgroup\$
    – Warbo
    Sep 24 '14 at 8:10
  • \$\begingroup\$ Well it's good to be precise. For example it matters if you try to use them as mutable objects, e.g. z=4; z.cool=200; alert(z.cool) won't behave as you expect if you expect z to be an object \$\endgroup\$
    – Claudiu
    Sep 24 '14 at 14:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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