Streamlining my javascript with a function

Question

i have a series of select lists, that i am using to populate text boxes with ids. so you click a select option and another text box is filled with its id.

with just one select/id pair this works fine, but i have multiples, and the only thing that changes is the id of the select and input.. in fact just the ending changes, the inputs all start with featredproductid and the select ids all start with recipesproduct and then both end with the category.

i know that listing this over and over for each category is not the way to do it. i think i need to make an array of the categories var cats = ['olive oil', "grains", "pasta"] and then use a forEach function? maybe?

here is the clunky code

window.addEvent('domready', function() {
    $('recipesproductoliveoil').addEvent('change', function(e){
       pidselected = this.options[this.selectedIndex].getProperty('value') ;
   $("featuredproductidoliveoil").setProperties({
       value: pidselected}); ;
    });
       $('recipesproductgrains').addEvent('change', function(e){
           pidselected = this.options[this.selectedIndex].getProperty('value') ;
       $("featuredproductidgrains").setProperties({
           value: pidselected}); ;
        });
      $('recipesproductpasta').addEvent('change', function(e){
          pidselected = this.options[this.selectedIndex].getProperty('value') ;
      $("featuredproductidpasta").setProperties({
          value: pidselected}); ;
       });
    $('recipesproductpantry').addEvent('change', function(e){
        pidselected = this.options[this.selectedIndex].getProperty('value') ;
    $("featuredproductidpantry").setProperties({
        value: pidselected}); ;
     });

});

keep in mind this is mootools 1.1 (no i cant update it sorry). i am sure this is kind of basic, something i seem to have wrapping my brain around. but i am quite sure doing it as above is not really good...

JS supports higher-order functions (functions that take & return functions). Note that the answers all follow a common pattern: factor out the part of the function than varies, turning it into a parameter of a higher order function. — outis, May 06 '10 at 01:19
You should declare `pidselected` as local (`var pidselected` within the listener) so you don't use a global and pollute the global namespace. — outis, May 06 '10 at 01:19

score 1 · Accepted Answer · answered May 06 '10 at 00:59

1

You're close. This is how you could do it:

var cats = ['oliveoil', 'grains', 'pasta'];
for (i in cats) {
    addChangeFunction(cats[i]);
}

function addChangeFunction(name) {
    $('recipesproduct' + name).addEvent('change', function(e) {
        pidselected = this.options[this.selectedIndex].getProperty('value');
        $('featuredproductid' + name).setProperties({
            value: pidselected
        });
    });
}

answered May 06 '10 at 00:59

BalusC

1,082,665
372
3,610
3,555

why limit based upon id? a much more semantic approach is to go at it via a selector and work with an array, like `var addChangeEvent(elements) { elements.each(function(el) { el.addEvent(...)`, either add class="food" to them or go `addChangeEvent($("formid").getElements("select"));`. In 1.2 you could apply the event directly on top of the selector a'la `$("formid").getElements("select").addEvent()'` – Dimitar Christoff May 06 '10 at 14:57
@Dimitar: There are indeed more ways to Rome. This one is just the shortest. Feel free to post your own way as answer. – BalusC May 06 '10 at 15:00
thanks, this is just what i meant. so in this cats[i] is passed to the function as name. right? – liz May 06 '10 at 15:09
@dimitar yes you are correct, if i had control of the initial output, or the mootools version, but i dont--there are no hooks in the code and i cant add any. this seemed to be the best course in this scenario. – liz May 06 '10 at 15:12
1

then don't use `for i in cats` for sure. this is mootools. in mootools, arrays are prototyped, using this kind of loop will also try to use all the prototyped array methods! use simply `['oliveoil', 'grains', 'pasta'].each(function(el) { something(el); })` - here are the bled prototypes: http://www.jsfiddle.net/BxhAK/ – Dimitar Christoff May 06 '10 at 15:44
@Dimitar: I don't do mootools, but if that's possible, then I would indeed just use `cats.each()` instead of `for (i in cats)`. @liz: I'd appreciate an upvote as well though. Accepted answers with 0 votes doesn't look right. – BalusC May 06 '10 at 16:25
@BalusC fair enough on mootools use - but treating an array like an object and not checking for `hasOwnProperty` after is a bad practice on any framework :) – Dimitar Christoff May 07 '10 at 09:57

score 0 · Answer 2 · answered May 06 '10 at 00:58

Something like this might help:

 function bindSelectFeature(select, featured) {
    $(select).addEvent('change', function(e){
        pidselected = this.options[this.selectedIndex].getProperty('value') ;
        $(featured).setProperties({
          value: pidselected
        });
     });
  });
  bindSelectFeature('recipesproductpasta','featuredproductidpasta');

Streamlining my javascript with a function

2 Answers2

Linked