Populating two lists, each from their respective with user input field

Question

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?

Answer 1

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);
}

Answer 2

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'});​

Answer 3

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.