Shortening some repeated code in a for loop not allowed?

Question

I'm trying to reduce the amount of JS that I have and I'm not sure why I'm not allowed to do it the way I'm trying. It's just this code for some mouseover/mouseout functions on images:

$(document).ready(function() {
    $("#social_btn_1").on('mouseover', function(){
        $(this).attr('src', 'images/social_1_dwn.png');
    })
    $("#social_btn_1").on('mouseout', function(){
        $(this).attr('src', 'images/social_1.png');
    })

    $("#social_btn_2").on('mouseover', function(){
        $(this).attr('src', 'images/social_2_dwn.png');
    })
    $("#social_btn_2").on('mouseout', function(){
        $(this).attr('src', 'images/social_2.png');
    })

    $("#social_btn_3").on('mouseover', function(){
        $(this).attr('src', 'images/social_3_dwn.png');
    })
    $("#social_btn_3").on('mouseout', function(){
        $(this).attr('src', 'images/social_3.png');
    })

    $("#social_btn_4").on('mouseover', function(){
        $(this).attr('src', 'images/social_4_dwn.png');
    })
    $("#social_btn_4").on('mouseout', function(){
        $(this).attr('src', 'images/social_4.png');
    })
});

And I'm trying to shorten by doing this:

  $(document).ready(function() {
        for (var i = 1; i < 5; i++) {
            $("#social_btn_"+ i).on('mouseover', function(){
                $("#social_btn_"+ i).attr('src', 'images/social_'+ i +'_dwn.png');
            })
            $("#social_btn_"+ i).on('mouseout', function(){
                $("#social_btn_"+ i).attr('src', 'images/social_'+ i +'.png');
            })
        }
    });

Can anyone explain why this doesn't work and what is the best way to consolidate the code I have? (I know you can do this stuff with CSS3 by the way, but I need to use JQuery). Thanks.

Answer 1

You're suffering from the ""oh so common"" closure in a loop problem there.

JavaScript variables declared with 'var ' have function scope so i is shared across all elements here. Each of the handlers gets i=5 since it's the same i that is in the scope of .ready

Simplest solution

You can solve this by wrapping the code in a closure - defining a scope:

 $(document).ready(function() {
       for (var i = 1; i < 5; i++) {(function(i){
            $("#social_btn_"+ i).on('mouseover', function(){
                $("#social_btn_"+ i).attr('src', 'images/social_'+ i +'_dwn.png');
            })
            $("#social_btn_"+ i).on('mouseout', function(){
                $("#social_btn_"+ i).attr('src', 'images/social_'+ i +'.png');
            })
        })(i)}
    });

More correct solution

You have "string"+i selectors which is usually a strong indication of code smell most of the time. You're using sequential data and JavaScript sports arrays. Alternatively you can use classes.

You can store a data-hover property on each image pointing to the hover image. Then you can add a social-icon class to the images and do something like:

$(document).ready(function(){
    $(".socialButton").mouseover(function(e){
        $(this).attr("src",$(this).data("hover"));
        $(this).data("old-src",$(this).attr("src"));
    }).mouseout(function(e){
            $(this).attr("src",$(this).data("old-src"));
    });
});

Even more correct solution

Have JavaScript objects representing the state rather than data attributes like in the example above.

Most correct solution

Use CSS, CSS has :hover selector which lets you change CSS properties when it is hovered.

The social buttons become div elements with background-image pointing to the image, you can have a rule like:

#myDiv:hover{
    background-image:url(image/social_1_dwn.png)
}