5
\$\begingroup\$

What I'm trying to achieve here is sort queue of .item-dragable between different lists. Users can click on right and left arrows and items move in queue respectively, where the previous or next item gets placed in place of item that was moved. There is also a placeholder element that should not be counted as a valid sortable, so there are checks to ignore it. If an item is in the last list and is moved to the right, it moves to the first list. If it is in the first list and gets moved to the left it moves to last list (last item in last list, first item in first list).

The first thing that came to mind was switch statements, but I can't figure out a way to implement them in this case, as each check is unique. Could you please provide another alternative or let me know if there is any other better way than using so many if / else statements in this case?

There are more bits to this code that find correct adjacent lists, re initiate some stuff etc., but adding these in would make question very lengthy.

if ( (item.is(':last-child') || item.next().hasClass('item-dragable-placeholder') ) && direction == "right") {
        var itemListAdjTmp = itemListAdj.find('.item-dragable').first(),
            itemPartner;
        if(itemList.find('.item-dragable').length <= 1) {
            itemPartner = item.next('li');
            item.insertBefore(itemListAdjTmp);
            itemListAdjTmp.insertBefore(itemPartner);

        } else {
            itemPartner = item.prev('.item-dragable');
            item.insertBefore(itemListAdjTmp);
            itemListAdjTmp.insertAfter(itemPartner);
        }
    } 
    else if (!item.is(':last-child') && !item.next().hasClass('item-dragable-placeholder') && direction == "right") {
        item.insertAfter(item.next('.item-dragable'));
    }
    else if (item.is(':first-child') && direction == "left") {
        var itemListAdjTmp = itemListAdj.find('.item-dragable').last(),
            itemPartner;
        if(itemList.find('.item-dragable').length <= 1) {
            itemPartner = item.next('li');
            item.insertAfter(itemListAdjTmp);
            itemListAdjTmp.insertBefore(itemPartner);

        } else {
            itemPartner = item.next('.item-dragable');
            item.insertAfter(itemListAdjTmp);
            itemListAdjTmp.insertBefore(itemPartner);
        }
    }
    else if (!item.is('first-child') && direction == "left") {
        item.insertBefore(item.prev('.item-dragable'));
    }

Example markup:

<li class="queue-month">
                <h3>November</h3>
                <ol class="clearfix sortable-queue-month">
                    <li class="item-dragable" data-position="4">
                        <a href="#" class="remove" title="Remove item">Remove item</a>
                        <img src="..." alt="Socks outline">
                        <span class="drag-indicator">Drag</span>
                        <div class="sort-indicator-mobile">
                            <a href="#" class="sort-left">Sort Left</a>
                            <a href="#" class="sort-right">Sort Right</a>
                        </div>
                    </li>
                    <li class="item-dragable" data-position="5">
                        <a href="#" class="remove" title="Remove item">Remove item</a>
                        <img src="..." alt="Socks outline">
                        <span class="drag-indicator">Drag</span>
                        <div class="sort-indicator-mobile">
                            <a href="#" class="sort-left">Sort Left</a>
                            <a href="#" class="sort-right">Sort Right</a>
                        </div>
                    </li>
                    <li class="item-dragable" data-position="5">
                        <a href="#" class="remove" title="Remove item">Remove item</a>
                        <img src="<?= CDN ?>img/png/sock-placeholder.png" alt="Socks outline">
                        <span class="drag-indicator">Drag</span>
                        <div class="sort-indicator-mobile">
                            <a href="#" class="sort-left">Sort Left</a>
                            <a href="#" class="sort-right">Sort Right</a>
                        </div>
                    </li>
                </ol>
            </li>

            <li class="queue-month">
                <h3>November</h3>
                <ol class="clearfix">
                    <li class="item-dragable-placeholder">
                        <a href="#" class="add-surprise-item" data-image="<?= CDN ?>img/png/sock-placeholder.png">Surprise me</a>
                    </li>
                </ol>
            </li>
\$\endgroup\$
0

3 Answers 3

Reset to default
2
\$\begingroup\$

You can split this up into "left" and "right"

Kind of hard to explain, but I will show you with the code I have rewritten

if ( direction == "right" ) {
    if (item.is(':last-child') || item.next().hasClass('item-dragable-placeholder')) {
        var itemListAdjTmp = itemListAdj.find('.item-dragable').first(),
            itemPartner;
        if(itemList.find('.item-dragable').length <= 1) {
            itemPartner = item.next('li');
            item.insertBefore(itemListAdjTmp);
            itemListAdjTmp.insertBefore(itemPartner);
        } else {
            itemPartner = item.prev('.item-dragable');
            item.insertBefore(itemListAdjTmp);
            itemListAdjTmp.insertAfter(itemPartner);
        }
    } else {
        item.insertAfter(item.next('.item-dragable'));
    }
} else {
    if (item.is(':first-child')) {
        var itemListAdjTmp = itemListAdj.find('.item-dragable').last(),
            itemPartner;
        if(itemList.find('.item-dragable').length <= 1) {
            itemPartner = item.next('li');
            item.insertAfter(itemListAdjTmp);
            itemListAdjTmp.insertBefore(itemPartner);
        } else {
            itemPartner = item.next('.item-dragable');
            item.insertAfter(itemListAdjTmp);
            itemListAdjTmp.insertBefore(itemPartner);
        }
    } else {
        item.insertBefore(item.prev('.item-dragable'));
    }
}

to Dry it out you really do want to split the code into right and left

This solution actually adds another if statement, but also removes duplicate conditions throughout the code. This solution also stops else if statements from being run if say direction == "left" it skips all the "right" stuff.

To Further DRY your code and keep you from repeating yourself too much, it was pointed out to me that

item.insertAfter(itemListAdjTmp);
itemListAdjTmp.insertBefore(itemPartner);

is being repeated on the left side. I looked at the if/else structure to make sure that it wouldn't change the logic if I took them out, and you are performing one or the other no matter what so I could take this bit of code out of the if/else block and then you don't have to repeat it.

So now it looks like this for the else statement

} else {
    if (item.is(':first-child')) {
        var itemListAdjTmp = itemListAdj.find('.item-dragable').last(),
            itemPartner;
        if(itemList.find('.item-dragable').length <= 1) {
            itemPartner = item.next('li');
        } else {
            itemPartner = item.next('.item-dragable');
        }
        item.insertAfter(itemListAdjTmp);
        itemListAdjTmp.insertBefore(itemPartner);
    } else {
        item.insertBefore(item.prev('.item-dragable'));
    }
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ I'm glad to help. \$\endgroup\$
    – deathismyfriend
    Jul 8, 2015 at 15:12
  • 1
    \$\begingroup\$ @Malachi Awesome :) just applied these left / right separations to other code, works good, thank you \$\endgroup\$
    – Ilja
    Jul 8, 2015 at 15:19
3
\$\begingroup\$

Here's some rewriting to remove the verbose nested if/else statements and some of the duplicate code.

First thing is to declare the variables on the first two lines outside of the if/else statements, since they don't change (and you'd just be repeating yourself).

The first one, itemPartner essentially uses an if/else in it's declaration.

Roughly translated:

[is this condition met] ? [yes] : [no]

It's just a shorter way of handling that if/else bit to declare that variable.

The second is the group of elements you pull the first and last out of. Since the group doesn't change, I figured we could declare it outside of the if/else blocks, and get the first and last within the blocks.

var itemPartner = (itemList.find('.item-dragable').length <= 1) ? item.next('li') : item.prev('.item-dragable'),
    itemListAdjTmp = itemListAdj.find('.item-dragable');

if(direction == "right"){
    if (item.is(':last-child') || item.next().hasClass('item-dragable-placeholder')) {
        item.insertBefore(itemListAdjTmp.first());
        itemListAdjTmp.first().insertAfter(itemPartner);
    }
    else if (!item.is(':last-child') && !item.next().hasClass('item-dragable-placeholder')) {
        item.insertAfter(item.next('.item-dragable'));
    }
}else if (direction == "left") {
    if (item.is(':first-child')) {
        item.insertAfter(itemListAdjTmp.last());
        itemListAdjTmp.last().insertBefore(itemPartner);
    }
    else{
        item.insertBefore(item.prev('.item-dragable'));
    }
}

I could take it a step further, and write it as I tend to prefer, with most variables declared up front. To me, this is easier to read and make sense of, since the variable names in the if/else statements clearly indicate what they refer to in only one word (instead of a line of code), but that may just be a personal preference.

To demonstrate:

var itemPartner = (itemList.find('.item-dragable').length <= 1) ? item.next('li') : item.prev('.item-dragable'),
    itemListAdjTmp = itemListAdj.find('.item-dragable'),
    isFirstChild = item.is(':first-child'),
    isLastChild = item.is(':last-child'),
    hasDragableClass = item.next().hasClass('item-dragable-placeholder');

if (direction == "right") {
    if (isLastChild || hasDragableClass) {
        item.insertBefore(itemListAdjTmp.first());
        itemListAdjTmp.first().insertAfter(itemPartner);
    }
    else if (!isLastChild && !hasDragableClass) {
        item.insertAfter(item.next('.item-dragable'));
    }
} else if (direction == "left") {
    if (isFirstChild) {
        item.insertAfter(itemListAdjTmp.last());
        itemListAdjTmp.last().insertBefore(itemPartner);
    }
    else {
        item.insertBefore(item.prev('.item-dragable'));
    }
}
\$\endgroup\$
0
1
\$\begingroup\$

You're not going to be able to use a switch to deal with the multiple levels of nested if statements. Also switch statements are not really conducive to multi-condition if statements.

It's really hard to tell what is going on from this code but could you split it up into two functions, one for "right" and one for "left"?

My knowledge of javascript is not the best but given the complexity of what appears to be done I would recommend that if this code works well now and does what it is supposed to, I would consider commenting it so that when you or someone else comes back to look at it again in the future it will be easier to understand what is going on.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ @IlyaKnaup It's really hard to tell what is going on from this code but could you split it up into two functions, one for "right" and one for "left"? \$\endgroup\$
    – paullb
    Jul 8, 2015 at 9:14
  • \$\begingroup\$ @paullb basically it sorts list elements in different ordered lists, if item is moved right change it with right list element, or if its last move it to next ordered list, if that ordered list is last move it to first, this is vice versa if element is moved left. And there is a placeholder there that should not be considered as a valid element, so there are checks to account for it. The only way I can think of to move it into separate functions is still using if else statements and only bit I clean up is that && direction ones :/ \$\endgroup\$
    – Ilja
    Jul 8, 2015 at 9:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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