current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 0 down vote favorite

I was looking into removing multiple inputs and selects at once using jQuery, but I couldn't find a solution, so I ended up with the following code:

$(function() {

  $('a.add').click(function(evt) {
    //clone lists
    //is there any way to simplify this part --begin--
    $('#s0 option').clone().appendTo('#s'+obj);
    $('#i0 option').clone().appendTo('#i'+obj);
    $('#r0 option').clone().appendTo('#r'+obj);
    $('#ieu0 option').clone().appendTo('#ieu'+obj);
    $('#ied0 option').clone().appendTo('#ied'+obj);
    //--end--
    evt.preventDefault(); //prevents scrolling
  });

  //remove inputs/selects
  $('a.remove').click(function(evt) {
    //animation is not necessary. How can I simplify this code? --begin--
    $('#comp'+x).animate({opacity:"hide"}, "slow").remove();
    $('#ppt'+x).animate({opacity:"hide"}, "slow").remove();
    $('#ct'+x).animate({opacity:"hide"}, "slow").remove();
    $('#p'+x).animate({opacity:"hide"}, "slow").remove();
    $('#ipvs'+x).animate({opacity:"hide"}, "slow").remove();
    $('#s'+x).remove();
    $('#i'+x).remove();
    $('#r'+x).remove();
    $('#ipvs'+x).remove();
    $('#other'+x).remove();
    //--end--
  } 
  evt.preventDefault();
  });
});

How can the sections marked between --begin-- and --end-- be simplified/improved? Thanks !

share|improve this question

migrated from stackoverflow.com Aug 22 '12 at 13:39

This question came from our site for professional and enthusiast programmers.
    
.each() – zzzzBov Aug 21 '12 at 18:10
1  
It would be hard to know how to best do this without seeing the HTML structure. – Mike Brant Aug 21 '12 at 18:12
2  
Just a hint, you can select elements using , to separate them. – Ricardo Lohmann Aug 21 '12 at 18:12
    
Check this fiddle, it might help you. – guy mograbi Aug 21 '12 at 19:50

2 Answers 2

up vote 2 down vote accepted

I would go with ChrisThompson's method of adding classes and referring to them. But to answer your question straight and simple

the first block can look like 

$.each( ['s','i','r','ieu','ied'], function(index,item){
        $('#' + item + '0 option').clone().appendTo('#' + item + obj );
    });

However, you did not specify what obj is

The second block can use the same technic to be simplified.

The entire code would look something like this

$(function() {
  $('a.add').click(function(evt) {
    //clone lists
    //is there any way to simplify this part --begin--
    $.each( ['s','i','r','ieu','ied'], function(index,item){
        $('#' + item + '0 option').clons().appendTo('#' + item + obj );
    });
    //--end--
    evt.preventDefault(); //prevents scrolling
  });

  //remove inputs/selects
  $('a.remove').click(function(evt) {
    //animation is not necessary. How can I simplify this code? --begin--
    $.each( ['comp','ppt','ct','p','ipvs'], function(index,item){ $('#' + item + x).animate({opacity:"hide"} ,"slow").remove();});
    $.each(['s','i','r','other'], function(index,item){ $('#'+item+x).remove()});
    //--end--
    evt.preventDefault();
  } 
  });
});

However, it seems to me you are using too complex HTML structure. you need to

  1. Stop using IDs, and start using classes for elements that have things in common For example :

  2. Instead of cloning each item, you can wrap all of the in a DIV and simple use html() to clone the entire HTML. When you inject the HTML to some other div, give the new wrapper div an ID.

For example 

$(".my-form").append($("<div/>").addClass(".object-item").attr("id","newObject" + $(".my-form .object-item").length ).html( $(".items-to-clone-wrapper").html() );

You then have a unique identifier for every element on the page. for example $("#newObject1 .s") instead of $("#s1").

Here is a fiddle I made on the assumptions I made about your code. I hope it will be of use to you. please let me know if there's something I can do.

share|improve this answer
    
one though - I wouldn't necessarily use jQuery's each() function, as it tends to be slower (sometimes much slower) than JavaScript's native for statement – Zathrus Writer Aug 22 '12 at 14:02
    
ok. then simply replace the each with a native for loop. - the answer still stands, it will still make the code simpler. – guy mograbi Aug 22 '12 at 15:08
    
yes, I agree... I've also removed my answer, seeing you already have it in yours now :) – Zathrus Writer Aug 22 '12 at 20:44
    
Exactly what I was looking for, thank you so much to everyone! I did put the cloned items inside a div tag as suggested, and to remove the whole thing I just used: <code> $('a.remove').click(function(evt) { if(i > 0) { $('#clonedItems'+i).remove(); i-- ; } evt.preventDefault(); }); </code> Not sure how to use the code block... I'm a noob here. Anyways, thank you all! – R.D. Aug 28 '12 at 16:49
up vote 0 down vote

for the top part you might be able to add some logic to your html. it looks when something is clicked you move the options to another element. if you just specified in the HTML the source and target elements through a class name like: <select class="source" target="targetSelector"... and <select class="target"...

you could use a script like 

$('a.add').click(function(){
    $('.source').each(function(){
        var source = $(this);
        var target = $(source.attr('target'));
        target.append(source.find('option'))
    })
})

Not really more simplified but a way to abstract your logic a bit.

share|improve this answer

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.