3
\$\begingroup\$

Only current and next element (cNum) is to be shown; in case it is the last element in the structure - the first element should be shown.

var childNum = $('#h').roundabout("getChildInFocus");
var cNum = (Number(childNum)) + 2;
if (cNum == 5) cNum = 1;

$("#h li").hide();
$("#h li:nth-child(" + cNum + ")").show();

if (cNum == 4) {
    $("#h li:nth-child(1)").show();
} else {
    $("#h li:nth-child(" + (Number(cNum) + 1).toString() + ")").show();
}
\$\endgroup\$
  • 1
    \$\begingroup\$ What is cNum used for? It's really confusing. \$\endgroup\$ – Larry Battle Sep 10 '12 at 19:27
  • \$\begingroup\$ Welcome to Code Review! With a jsFiddle or live html page, you would get more answers. :) \$\endgroup\$ – Quentin Pradet Sep 21 '12 at 14:09
3
\$\begingroup\$

Here are a few tips.

1)

childNum is not needed since it's only used once.

Old Code:

var childNum = $('#h').roundabout("getChildInFocus");
var cNum = (Number(childNum)) + 2;

New Code:

var cNum = $('#h').roundabout("getChildInFocus") + 2;

2)

In general, adding a object to a string results in a string. And adding a object to a number results in a number. Use parentheses to make the conversion clear.

Old Code:

$("#h li:nth-child(" + (Number(cNum) + 1).toString() + ")").show();

New Code:

$("#h li:nth-child(" + (1 + cNum) + ")").show(); 

3)

Cache commonly referenced elements. Old Code:

$("#h li:nth-child(" + cNum + ")").show();

New Code:

var $lis = $("#h li");
$lis.find(":nth-child(" + cNum + ")").show();

4)

Combine operations on selectors. Old Code:

$("#h li").hide();
$("#h li:nth-child(" + cNum + ")").show();

New Code:

$("#h li").hide().find(":nth-child(" + cNum + ")").show();

5)

Resetting the value of cNum to get rid of the else condition gives the following.

Old Code:

if (cNum == 4) {
    $("#h li:nth-child(1)").show();
} else {
    $("#h li:nth-child(" + (1+cNum) + ")").show();
}

New Code 1:

if (cNum == 4) {
    cNum = 0;
} 
$("#h li:nth-child(" + (1+cNum) + ")").show();

New Code 2:

if (cNum == 5) {
    cNum = 1;
} 
$("#h li:nth-child(" + cNum + ")").show();  

As you can see New Code 2 is the same from the new code in tip 4. So you can delete this redundant code.

Final Code:

var cNum = 2 + $('#h').roundabout("getChildInFocus");
if (cNum == 5){
    cNum = 1;
}
$("#h li").hide().find(":nth-child(" + cNum + ")").show();
\$\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.