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.

I've simplified this for posting, since there are going to be more than 4 options sets for this function to cycle through:

function redundantSee() {

  var optionSet1 = $('.wrapper:eq(0)'),
      optionSet2 = $('.wrapper:eq(1)');

  optionSet1.find('.options').each(function(){
      var self = $(this),
          input = self.find('input'),
          title = self.find('.title').text(),
          value = input.val(),
          source = self.find('img').attr('src'),
          id = input.attr('id'),
          listItem = $('<li/>', {'value': value, 'id': id }),
          imageElement = $('<img/>', {'src': source, 'title': title});

      $('div.corresponding1').append(nameListItem);
      listItem.append(imageElement);
  });

  optionSet2.find('.options').each(function(){
      var self = $(this),
          input = self.find('input'),
          title = self.find('.title').text(),
          value = input.val(),
          source = self.find('img').attr('src'),
          id = input.attr('id'),
          listItem = $('<li/>', {'value': value, 'id': id }),
          imageElement = $('<img/>', {'src': source, 'title': title});

      $('div.corresponding2').append(listItem);
      listItem.append(imageElement);
  });
}

I'm having a problem figuring out how to turn all of the repetitive code into something much more manageable. But since each option set has its own each loop, the $(this) variable (and all corresponding variables) are specific to the loop that is run on the ('.options') element.

If I do one each loop and use a counter, like this:

$('wrapper').each(function(i){ // ... });

I still run into the problem of needing to re-declare all my new variables specific to that optionSet's turn in the loop.

Can someone help me figure out how I can condense this so that I'm not constantly repeating the same code every time I add a new option set to the function?

share|improve this question

3 Answers 3

Create a function with the common code:

function redundantSee() {

  var optionSet1 = $('.wrapper:eq(0)'),
      optionSet2 = $('.wrapper:eq(1)');


    function addImage(correspond, item) {
      var self = $(this),
          input = self.find('input'),
          title = self.find('.title').text(),
          value = input.val(),
          source = self.find('img').attr('src'),
          id = input.attr('id'),
          listItem = $('<li/>', {'value': value, 'id': id }),
          imageElement = $('<img/>', {'src': source, 'title': title});

      $(correspond).append(item);
      item.append(imageElement);
    }

    optionSet1.find('.options').each(function() {addImage('div.corresponding1', nameListItem);})
    optionSet1.find('.options').each(function() {addImage('div.corresponding2', listItem);})
}

Here's a further reduced version that is also a little more efficient:

function redundantSee() {
    function processSet(set, correspond, item) {
        set.find('.options').each(function() {addImage(correspond, item);})
    }   

    function addImage(correspond, item) {
      var self = $(this),
          input = self.find('input'),
          title = self.find('.title').text(),
          value = input.val(),
          source = self.find('img').attr('src'),
          id = input.attr('id'),
          listItem = $('<li/>', {'value': value, 'id': id }),
          imageElement = $('<img/>', {'src': source, 'title': title});

      $(correspond).append(item);
      item.append(imageElement);
    }

    var wrappers = $('.wrapper');
    processSet(wrappers.eq(0), 'div.corresponding1', nameListItem);
    processSet(wrappers.eq(1), 'div.corresponding2', listItem);
}
share|improve this answer
    
These are two completely new ways for me to try out.... so definitely helpful. I always want to abstract the anonymous callback function of the each loop into its own function... wouldn't I be able to do that here by substituting .each(function(){addImage(correspond,item);} with .each(addImage(correspond,item);} ? –  laserface Nov 7 '12 at 5:05
    
@user1798630 - no. .each() takes a function reference and that function is called by .each() with two predetermined arguments. Your version executes addImage() immediately and passes the return result from that (which is nothing) to .each() as the function reference. It will not work. You have to pass a function reference - you can't call it right away and you can't determine what the arguments are. –  jfriend00 Nov 7 '12 at 5:13
    
I see... that makes sense now. That actually clears up some other issues that I've had with callbacks too. I appreciate the help! –  laserface Nov 7 '12 at 5:18
    
Because extending jQuery was suggested on my initial post, and it looked like the simplest option, I chose that. It required only one function definition with a few parameters. I appreciate you looking at my code and offering solutions though... it helps me think about it in a different way. –  laserface Nov 8 '12 at 4:28

Pretty much explained in the comments

//extract reusable code
function operateOptions(el, correspondsTo) {


    var self = $(el),
        input = self.find('input'),
        id = input.attr('id'),
        value = input.val(),
        title = self.find('.title').text(),
        source = self.find('img').attr('src'),
        listItem = $('<li/>', {
            'value': value,
            'id': id
        }),
        imageElement = $('<img/>', {
            'src': source,
            'title': title
        });

    //chain methods
    listItem.append(imageElement).appendTo(correspondsTo);
}

//create function that accepts multiple options
function redundantSee(list) {

    //loop through each option
    $.each(list, function (optionSet, correspondsTo) {

        //operate on each option in the optionSet
        $(optionSet).find('.options').each(function (i, el) {

            //send over the option element and it's corresponding selector
            operateOptions(el, correspondsTo);
        });
    });
}

redundantSee({'.wrapper:eq(0)': 'div.corresponding1', '.wrapper:eq(1)': 'div.corresponding2'});​

Here's a pretty-printed, cleaned version:

function operateOptions(el, correspondsTo) {
    var self = $(el),
        input = self.find('input'),
        id = input.attr('id'),
        value = input.val(),
        title = self.find('.title').text(),
        source = self.find('img').attr('src'),
        listItem = $('<li/>', {
            'value': value,
            'id': id
        }),
        imageElement = $('<img/>', {
            'src': source,
            'title': title
        });
    listItem.append(imageElement).appendTo(correspondsTo);
}

function redundantSee(list) {
    $.each(list, function (optionSet, correspondsTo) {
        $(optionSet).find('.options').each(function (i, el) {
            operateOptions(el, correspondsTo);
        });
    });
}
redundantSee({'.wrapper:eq(0)': 'div.corresponding1', '.wrapper:eq(1)': 'div.corresponding2'});​
share|improve this answer
    
This is interesting. I'm gonna try this out cause it is such a different way of thinking than I am used to. So $.each's first parameter 'list' is operated on by the callback for each element. It looks like what I understand $.map would do.. but I have not used $.map yet, so not sure of the differences. Anyway, very cool and I'm gonna give this a whirl. So many different ways to go on this one! –  laserface Nov 7 '12 at 5:13

It doesn't make much sense to put <li> elements inside a <div>. By the HTML5 specification, <li> elements belong inside <ol>, <ul>, or <menu> only. If you don't want it to look like a bulleted list, use CSS to suppress the list styling. But don't use a <div> to contain a list — it violates the spec and is semantically inferior is well.

share|improve this answer
    
Thanks for correcting me on the HTML5 spec. However, this is a JavaScript / jQuery question. Most likely I was inserting HTML into an already prepared DOM that was generated with someone else's PHP code, and it probably wasn't using HTML5. Either way, thanks for keeping me up to spec. –  laserface Apr 2 at 13:25

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.