3
\$\begingroup\$

I wrote this code using jQuery, but I think it can be improved. Can you provide some feedback?

    var $i = 1,
    totalImg = $('.home-featured-bg .absolute div').length,
    $theWidth = $(window).width(),
    $theHeight = $(window).height();
    $('.home-featured-bg .absolute').css('width',$theWidth * totalImg+'px');
    $('.home-featured-bg .absolute .slide,.home-featured-bg .absolute .slide img').css({'width':$theWidth+'px','height' : $theHeight+'px'});

window.setInterval(function(){
    $('.events-links a').hide();
    if($i == 1)
    {

        var $el = $theWidth;
    }else{
        var $el = $theWidth * $i;
    }
    if($i < totalImg)
    {
        $('.home-featured-bg .absolute').animate({'margin-left':'-'+$el+'px'},2000);
        $i++;
        $('.events-links a').eq($i - 1).css('display','block');
    }else{
        $('.home-featured-bg .absolute').animate({'margin-left':'0px'},2000);
        $i = 1;
        $('.events-links a').eq($i - 1).css('display','block');
    }


},3000);

$(window).resize(function(){
    totalImg = $('.home-featured-bg .absolute div').length,
    $theWidth = $(window).width(),
    $theHeight = $(window).height();
    $('.home-featured-bg .absolute').css('width',$theWidth * totalImg+'px');
    $('.home-featured-bg .absolute .slide,.home-featured-bg .absolute .slide img').css({'width':$theWidth+'px','height' : $theHeight});
});
\$\endgroup\$
3
\$\begingroup\$

Here few suggestion:

in the function you have :

if($i == 1)
{

    var $el = $theWidth;
}else{
    var $el = $theWidth * $i;
}

could be replaced by a single line:

var $el = $theWidth * $i;

and you could modify the following if :

if($i < totalImg)
{
    $('.home-featured-bg .absolute').animate({'margin-left':'-'+$el+'px'},2000);
    $i++;
    $('.events-links a').eq($i - 1).css('display','block');
}else{
    $('.home-featured-bg .absolute').animate({'margin-left':'0px'},2000);
    $i = 1;
    $('.events-links a').eq($i - 1).css('display','block');
}

by extract the $('.events-links a').eq($i - 1).css('display','block'); like this:

if($i < totalImg)
{
    $('.home-featured-bg .absolute').animate({'margin-left':'-'+$el+'px'},2000);
    $i++;
}else{
    $('.home-featured-bg .absolute').animate({'margin-left':'0px'},2000);
    $i = 1;
}
$('.events-links a').eq($i - 1).css('display','block');
\$\endgroup\$
0
\$\begingroup\$

I agree with Nettogrof on the following:

var $el = $theWidth * $i;

But you can shorten your code even more by doing the following (*not tested):

var posNeg = '-';
if($i < totalImg) {$i = 0; posNeg='';}
$('.home-featured-bg .absolute').animate({marginLeft:posNeg +$el+'px'},2000);
$('.events-links a').eq($i).show();
$i++;

More things you can do to improve code:

Set variables for selector objects, makes changes easier, and easier to manage etc., e.g:

Instead of :

$('.myClass').fadeIn();
$('.myClass').css('color','#ff0000');

Use :

var mySel = $('.myClass');
mySel.fadeIn().css('color','#ff0000');
\$\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.