jQuery code is too verbose, would like pointers on how to make it shorter [closed]

Question

I have some jQuery code that I would like reviews and pointers on on how to bring its line count down and shorten.

$('#p1').click(function() {
    $('#list').fadeOut(450);
    $('#q1').delay(600).fadeIn(450)
});
$('#p2').click(function() {
    $('#list').fadeOut(450);
    $('#q2').delay(600).fadeIn(450)
});
$('#p3').click(function() {
    $('#list').fadeOut(450);
    $('#q3').delay(600).fadeIn(450)
});
$('#p4').click(function() {
    $('#list').fadeOut(450);
    $('#q4').delay(600).fadeIn(450)
});

...

$('#p12').click(function() {
    $('#list').fadeOut(450);
    $('#q12').delay(600).fadeIn(450)
});
$('#p13').click(function() {
    $('#list').fadeOut(450);
    $('#q13').delay(600).fadeIn(450)
});

Can this code be better optimised? Or at least made less verbose?

Answer 1

You can use a for loop, but you should make sure the loop counter's value gets into a correct scope for click event handler:

var clickHandler = function(k) {
    return function() {
        $('#list').fadeOut(450);
        $('#q' + k).delay(600).fadeIn(450);
    };
};
for (var i = 1; i < 14; ++i) {
    $('#p' + i).click(clickHandler(i));
}

Otherwise the delay and fadeIn would get applied to #q13 element exclusively, since actual counter (with its final value of 13) would get into closure.

---

EDIT: Since quite a lot of answers got it wrong here, I'll attempt to explain more precisely what's going on in this code, as it seems to be pretty confusing.

The "natural" solution with injecting the click handler directly into loop would be the following:

for(var i = 1; i < 14; i++) {
    $('#p'+i).click(function() {
        $('#list').fadeOut(450);
        $('#q'+i).delay(600).fadeIn(450)
    });
}

But this is not at all equivalent to the extended form, which lists all the 13 variants one after another. The problem is that while there are indeed 13 functions created here, they are all closed over the same variable i, whose value changes. It finally arrives at the value of 13 and the loop ends.

Some time later the functions attached to #p1...#p13 elements are called (when one of those elements are clicked) and they use that final value of i. This results in only #q13 being animated.

What needs to be done here is to do something called lambda lifting and eliminate the free variable i, whose value gets inadvertly changed. A common technique for that is to provide a "factory function" which accepts value for our variable and outputs an actual function which we'll use as event handler:

var clickHandler = function(k) {
    return function() {
        $('#list').fadeOut(450);
        $('#q' + k).delay(600).fadeIn(450);
    };
};

Since the scope of k parameter is local to clickHandler, every call to clickHandler gets different k variable. The function returned from clickHandler is therefore closed over different variables, which can in turn have different values. This is exactly what we need. We can then call clickHandler from our loop, passing it the counter's value:

for (var i = 1; i < 14; ++i) {
    $('#p' + i).click(clickHandler(i));
}

I hope this makes the difference somewhat clearer.

---

EDIT: As Esailija pointed out in the comments, it is also possible to use jQuery.each to achieve similar effect:

$.each(new Array(13), function(idx) {
    $('#p' + (idx + 1)).click(function() {
        $('#list').fadeOut(450);
        $('#q' + idx).delay(600).fadeIn(450);
    });
});

This is probably the solution of choice if you're already aware of the closure/scoping issue I've tried to outline above.

Answer 2

The ideal solution, IMO, contrary to the accepted answer, is not to base your code around relationships between id values, but to base it around relationships in the DOM. jQuery provides you with an index method, that allows you to see where your element is in relation to its siblings. You can then use this value to select the appropriate other element to show.

This does, of course, require you to structure your HTML in a semantic way. For instance:

<div id="list">
    <a href="#">Link 1</a>
    <a href="#">Link 2</a>
    <a href="#">Link 3</a>
    <a href="#">Link 4</a>
    <a href="#">Link 5</a>
    <a href="#">Link 6</a>
    <a href="#">Link 7</a>
    <a href="#">Link 8</a>
</div>
<div id="questions"> <!-- perhaps this is what q stands for... -->
    <div>1 ...</div>
    <div>2 ...</div>
    <div>3 ...</div>
    <div>4 ...</div>
    <div>5 ...</div>
    <div>6 ...</div>
    <div>7 ...</div>
    <div>8 ...</div>
</div>

The first link applies to the first nested div, the second to the second, etc. The markup is simple, easy to read and clearly semantic.

Your code can then be very simple, with no need to worry about factories. Indeed, the nicest way to do it is with event bubbling and delegation, handled admirably by jQuery's on method (in 1.7; before this, use delegate).

$('#list').on('click', 'a', function() {
    $('#list').fadeOut(450);
    $('#questions div').eq($(this).index()).delay(600).fadeIn(450);
});

Working jsFiddle (I know the animation looks a little clunky, sorry.)

$(this).index() finds the relation of the current element in relation to its siblings. .eq() filters the selection (of #questions div elements) to find the element in the corresponding position.

The code may not be quite as fast (if you use id values, it will almost certainly be mildly faster), but your markup is simpler and therefore more robust.

Answer 3

If you must insist on not using event delegation and classes then:

$.each( new Array(13), function(index){
    $('#p'+(index+1) ).click(function() {
        $('#list').fadeOut(450);
        $('#q'+(index+1)).delay(600).fadeIn(450)
    });
});

Using classes and delegation:

$( document ).delegate( ".my-class", "click", function(){
var idIndex = this.id.match( /\d+$/, )[0];

        $('#list').fadeOut(450);
            $('#q'+idIndex).delay(600).fadeIn(450)

});

Answer 4

Like others, you can use a for-loop to do this, but you are actually bounding yourself to 14 ps and whenever you add a new p you need to increase your for-loop test condition.

This is what I would do:

$("[id^=p]").bind("click", function() {
    $('#list').fadeOut(450);
    $('#q' + $(this).attr("id").match(/^#p(\d+)$/i)[1]).delay(600).fadeIn(450)
})