Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This is my first post of hopefully many here on code review. I am writing a function for a rotating navigation with js and jquery. I am using a plugin called jquery transit for animating rotation. The code works, but it is very long for such a simple function, is there any way this can be improved. I am self taught and always looking for ways to improve. Thanks in advance.

var rotation = 0;
$('nav ul li a').bind('click', function() {
'use strict';
var item = $(this).parent();
var itemI = item.index();
var navAll = $(this).parents('ul');

var condition = rotation + '_' + itemI;

switch(condition)
{
   case '0_0': 
        //do nothing
        break;
    case '0_1':
        navAll.transition({rotate: '-=90deg'});
        $('nav ul li a').transition({rotate:'90deg'});
        rotation = 270;
        break;
    case '0_2':
        navAll.transition({rotate: '-=180deg'});
        $('nav ul li a').transition({rotate:'180deg'});
        rotation = 180;
        break;
    case '0_3':
        navAll.transition({rotate: '+=90deg'});
        $('nav ul li a').transition({rotate:'-90deg'});
        rotation = 90;
        break;


    case '90_0':
        navAll.transition({rotate: '0deg'});
        $('nav ul li a').transition({rotate:'0deg'});
        rotation = 0;
        break;
    case '90_1':
        navAll.transition({rotate: '-90deg'});
        $('nav ul li a').transition({rotate:'90deg'});
        rotation = 270;
        break;
    case '90_2':
        navAll.transition({rotate: '+=90deg'});
        $('nav ul li a').transition({rotate:'-180deg'});
        rotation = 180;
        break;
    case '90_3':
        break;

    case '180_0':
        navAll.transition({rotate: '0deg'});
        $('nav ul li a').transition({rotate:'0deg'});
        rotation = 0;
        break;
    case '180_1':
        navAll.transition({rotate: '-90deg'});
        $('nav ul li a').transition({rotate:'90deg'});
        rotation = 270;
        break;
    case '180_2':
        //do nothing
        break;
    case '180_3':
        navAll.transition({rotate: '-=90deg'});
        $('nav ul li a').transition({rotate:'-90deg'});
        rotation = 90;
        break;


    case '270_0':
        navAll.transition({rotate: '0deg'});
        $('nav ul li a').transition({rotate:'0deg'});
        rotation = 0;
        break;
    case '270_1':
        //do nothing
        break;
    case '270_2':
        navAll.transition({rotate: '-=90deg'});
        $('nav ul li a').transition({rotate:'180deg'});
        rotation = 180;
        break;
    case '270_3':
        navAll.transition({rotate: '-=180deg'});
        $('nav ul li a').transition({rotate:'-90deg'});
        rotation = 90;
        break;


}

});
share|improve this question
    
Just realized that the $('nav ul li a') should be put in a variable. Any other suggestions? –  Devender Dec 29 '12 at 20:39

3 Answers 3

Instead of that complex switch statement, I'd just use an option object:

var allLinks = $('nav ul li a');
var rotation = 0;

$('nav').on('click', 'ul li a', function() {
    'use strict';
    var $this = $(this);
    var item = $this.parent();
    var navAll = $this.parents('ul');

    var option = {
        '0_1': {
            rotateAll: '-=90deg',
            rotateThis: '90deg',
            rotation: 270
        },
        '0_2': {
            rotateAll: '-=180deg',
            rotateThis: '180deg',
            rotation: 180
        }
        // Add them all here
    }[ rotation + '_' + item.index() ];

    if ( option ) {
        navAll.transition({rotate: option.rotateAll});
        allLinks.transition({rotate: option.rotateThis});
        rotation = option.rotation;
    }
}

There are 2 more things to consider:

  1. Are you sure you really need that nav ul li a selector? Wouldn't nav a do the job just the same? While we're at it, why are you listening to the click on the a elements, and then find the li via .parent()? Can't you just listen to the click event on the lis themselves?
  2. Is there any method to that configuration? I wasn't able to deduce any algorithm from what you've provided, but I'm sure there is. You'd be better off calculating it on the fly, if possible.
share|improve this answer
    
one methodical aspect I can see: rotation = (3 - item.index()) * 90 –  Stuart Dec 31 '12 at 2:10

I tried to deduce what it is that you're doing, and what I came up with is that you have four items arranged like a cross and you rotate it when one is clicked so that item is on top. You're also rotating each item, individually, so they continue to be upright during the rotation.

If that's what you have, I think this code should do the same thing. It's set to work with a variable number of items, so, in theory, you should be able to add to or remove from the set and the rotations will adjust.

Here is a demo.

The items in the <ul> are arranged in a circle via CSS.

<ul>
    <li id="a">0</li>
    <li id="b">1</li>
    <li id="c">2</li>
    <li id="d">3</li>
    <li id="e">4</li>
    <li id="f">5</li>
    <li id="g">6</li>
    <li id="h">7</li>
</ul>

The JavaScript:

var idx = 0,
    $lis = $('li'),
    len = $lis.length,
    center = (len - 1) / 2,
    halfLen = len / 2,
    degChg = 360 / len,
    $ul = $('ul');

$ul.on('click', 'li', function() {

    var $this = $(this),
        newIdx = $this.index(),
        chg = getPositionChg(idx, newIdx) * degChg;

    $ul.transition({rotate: '-=' + chg + 'deg'});
    // couldnt get this to work without using
    // the .each() method
    $lis.each(function() {
        $(this).transition({rotate: '+=' + chg + 'deg'});
    });
    idx = newIdx;
});

function getPositionChg(curIdx, nextIdx) {
    if (curIdx < center) {
        // if curIdx is left of center and change does not
        // cross the 0 index (is clockwise)
        if (nextIdx > curIdx + halfLen) {
            return nextIdx - len - curIdx;
        } else {
            return nextIdx - curIdx;
        }       
    } else {
        // if curIdx is right of center and change does
        // cross the 0 index
        if (nextIdx <= curIdx - halfLen) {
            return len - curIdx + nextIdx;
        } else {
            return nextIdx - curIdx;
        }
    }
}

​
share|improve this answer
    
I'd recommend extracting the position of the list elements as well so that is dynamically generated with the script. –  Shmiddty Jan 4 '13 at 19:15

You could do something like this, although there is a slight risk of making the code more cryptic than it is already.

var rotation = 0;
var r1Options = [[undefined, 0, 0, 0],
                 [-90, -90, -90, undefined],
                 [-180, 90, undefined, -180],
                 [90, undefined, -90, -180]],
    r2Options = [[undefined, 0, 0, 0],
                 [90, 90, 90, undefined],
                 [180, -180, undefined, 180],
                 [-90, undefined, -90, -90]];

function rotateString(degs) {
    var sign = '';
    if (degs < 0) sign = '-';
    else if (degs > 0) sign = '+';
    return sign + '=' + Math.abs(degs) + 'deg';
}
$('nav ul li a').bind('click', function() {
    'use strict';
    var item = $(this).parent();
    var itemI = item.index();
    var navAll = $(this).parents('ul');
    var r1 = r1Options[itemI][rotation / 90],
        r2 = r2Options[itemI][rotation / 90];
    if (r1 != undefined && r2 != undefined) {
        navAll.transition({rotate: rotateString(r1)});
        $('nav ul li a').transition({rotate: rotateString(r2)});
        rotation = (3 - itemI) * 90;
    }
}
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.