Jquery sortable style function

Question

This is a basic sorting function written in jQuery that moves items in the DOM around to create empty spaces for a droppable(). Since there is so much going on in a droppable over event, i'd love some feedback on how this could be done better.

This is all in a function called on the over event of the droppable()

I have an array of empty spots

    var emptyHolders = [];

    $('.pipeline-holder').each(function(){
        if($(this).hasClass('holder-empty')){
            var eid = $(this).attr('id');
            emptyHolders.push(eid.substring(14));
        }
    });

cid is the droppable element that you're over top of, so I find the next open slot using

   for (var i = 0; i < emptyHolders.length; i++) {
        var currentEmpty = emptyHolders[i];
        if (currentEmpty > cid) {
            nextEmpty = currentEmpty;
            i = emptyHolders.length;
        } else {
            prevEmpty = parseInt(currentEmpty);
        }
    }

If there is a an empty slot further down the list, I use a for loop to loop through moving the items around the DOM to make the space needed to drop the element.

 if (nextEmpty != null) {
        var moveMe = nextEmpty -1;

        for (var i = moveMe; i >= cid; i--) {

            var nextcount = i + 1;
            var me = $('#pipeline-rank-' + i);       
            var next = $('#pipeline-rank-' + i).parents('.pipeline-rank-row').next().find('.pipeline-holder');
            var pid = $('#pipeline-rank-' + i).find('.content-wrapper').attr('id');

            next.append($('#pipeline-rank-' + i).find('.content-wrapper'));
            next.removeClass('holder-empty');
            next.siblings('.remember-my-position-hover').html(pid);
        }
         $('#pipeline-rank-' + cid).addClass('holder-empty');
    } 

If there is not one further down the list, I do the same thing in reverse to check if there is a spot above the droppable to push items up into.

It's usable here: http://jsfiddle.net/mstefanko/LEjf8/9/

Any thoughts are extremely appreciated!

Answer 1

Here you have some general thoughts:

1. First of all, cache jQuery elements (var $this = $(this); instead of multiple executions of $(this)). http://jsfiddle.net/tomalec/pXPdS/1/

2. You can also chain jQuery methods, it simplifies codes, and helps with above.

3. To reduce amount of code you can use toggleClass instead of addClass removeClass.http://jsfiddle.net/tomalec/pXPdS/2/

4. Instead of selecting $('#rankDrag') in start callback you can use reference given in arguments ui.helper http://jsfiddle.net/tomalec/pXPdS/3/

5. To get id of empty/not empty holder ids, in my opinion, .map is more readable than .each. Moreover, you can move that function outside rearrange to avoid re-declaring it every time.

   function getTruncatedId(index, element){
       return element.id.substring(14);
   }
   
   function rearrange(dropid) {
       //..
       emptyHolders = holders.filter('.holder-empty').map(getTruncatedId);
       isRanked = holders.filter(':not(.holder-empty)').map(getTruncatedId);
       //..
   }
   

   http://jsfiddle.net/tomalec/pXPdS/4/

6. Pass dropped element to rearrange, so you will not have to traverse through DOM again:

   rearrange( $(this).attr('id') );//dropId = $(this).attr('id');
   //..
   isEmpty = $('#'+dropId).hasClass('holder-empty');
   

   http://jsfiddle.net/tomalec/pXPdS/5/