3
\$\begingroup\$

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!

\$\endgroup\$
3
+100
\$\begingroup\$

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/

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.