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 just posted this on the Stack Exchange, but was told I should post it here:

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've simplified this for posting, since there are going to be more than 4 options sets for this function to cycle through.

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 it's 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 redeclare 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
add comment

2 Answers

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
add comment

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
add comment

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.