Simple image rotation script

Question

I am writing a simple image rotation script. Users press either the prev or next button to view the previous or next images. It works well, but the code looks over the top. Is there a way I can reduce the amount of code used?

index.php

<div id="container">
<img src="pic1" id="pic1" class="top-pic" alt="" />
<img src="pic2" id="pic2" alt="" />
<img src="pic3" id="pic3" alt="" />
<img src="pic4" id="pic4" alt="" />
</div>

<input type="button" name="prev" id="prev" value="prev" />
<input type="button" name="next" id="next" value="next" />

jQuery

$('#next').click(function(){
    if($('#pic1').hasClass('top-pic')){
        $('#pic1').removeClass('top-pic');
        $('#pic2').addClass('top-pic');
    }else if($('#pic2').hasClass('top-pic')){
        $('#pic2').removeClass('top-pic');
        $('#pic3').addClass('top-pic');
    }else if($('#pic3').hasClass('top-pic')){
        $('#pic3').removeClass('top-pic');
        $('#pic4').addClass('top-pic');
    }else if($('#pic4').hasClass('top-pic')){
        $('#pic4').removeClass('top-pic');
        $('#pic1').addClass('top-pic');
    }

});

$('#prev').click(function(){
    if($('#pic1').hasClass('top-pic')){
        $('#pic1').removeClass('top-pic');
        $('#pic4').addClass('top-pic');
    }else if($('#pic4').hasClass('top-pic')){
        $('#pic4').removeClass('top-pic');
        $('#pic3').addClass('top-pic');
    }else if($('#pic3').hasClass('top-pic')){
        $('#pic3').removeClass('top-pic');
        $('#pic2').addClass('top-pic');
    }else if($('#pic2').hasClass('top-pic')){
        $('#pic2').removeClass('top-pic');
        $('#pic1').addClass('top-pic');
    }

});

Answer 1

You can use jQuery's .next() and .prev()

function nextPic() {
  var current = $("#container .top-pic");
  var next = current.next("img");
  if( next.length === 0 ) {
    next = $("#container img:first");
  }
  current.removeClass("top-pic");
  next.addClass("top-pic");
}

function prevPic() {
  var current = $("#container .top-pic");
  var prev = current.prev("img");
  if( prev.length === 0 ) {
    prev = $("#container img:last");
  }
  current.removeClass("top-pic");
  prev.addClass("top-pic");
}

$('#next').click(nextPic);
$('#prev').click(prevPic);

Or, avoiding the duplication:

function switchTopPic(direction) {
  var map = { prev: 'last', next: 'first' },
      current = $("#container .top-pic"),
      target = current[direction]("img");
  if( target.length === 0 ) {
    target = $("#container img")[map[direction]]();
  }
  current.removeClass("top-pic");
  target.addClass("top-pic");
}

$('#next').click(function () { switchTopPic("next") });
$('#prev').click(function () { switchTopPic("prev") });

Or given the markup in the question, the last little bit could just be:

$('#next, #prev').click(function () { switchTopPic(this.id) });

Either way, it should handle an arbitrary number of images (including none at all), and it doesn't require a top-pic to be set ahead of time. All state information is kept in the markup, and does not require the image IDs to be sequential (they don't even need an ID at all).

Edit: Here's a jsfiddle

Answer 2

You need to abstract your code from the design, at the moment, adding another picture is going to yield !!!fun!!! (if I'm allowed to use my dwarven vocabulary). At the moment adding another picture would require that you copy and paste code and extend that if further. This is obviously not desirable.

By keeping track of what picture is currently highlighted with a simple integer (for saving the index) you should be able to reduce all this. Warning, this is untested:

var idx = 1;

$('#next').click(function(){
    nextPic(1); // Hightlight the next
});

$('#prev').click(function(){
    nextPic(-1); // Highlight the previous
});

function nextPic(var step) {
    var nextIdx = idx + step;
    if($('#pic' + nextIdx).length === 0) {
        // Check if the element does not exist.
        if(nextIdx <= 0) {
            // If we are below or at zero, we need to wrap to the top.
            // So we count all imgs of the container.
            nextIdx = $('#container img').length;
        } else {
            // wrap around to the first.
            nextIdx = 1;
        }
    }

    $('#pic' + idx).removeClass('top-pic');
    $('#pic' + nextIdx).addClass('top-pic');

    idx = nextIdx;
}

This way you don't even need to change the code if you add more pictures, and it should even work if you add or remove pictures on the fly (given you don't remove the currently selected one).

Answer 3

It's rather contrived, but this is what I've come up with:

$('#next, #prev').click(function(){
    $imageCount = $('#container img').length;
    $current = $('#container .top-pic');
    $current.removeClass('top-pic');
    $currentIdNumber = parseInt($current.attr('id').substr(3));
    $newId = '#pic' + (this.id == 'next' ? $currentIdNumber % $imageCount + 1 : ($currentIdNumber + $imageCount - 2) % $imageCount + 1);
    $('#container ' + $newId).addClass('top-pic');
});

jsfiddle example

I could make it shorter still, but that won't improve the readability of the already difficult to read suggestion. ;-)

Answer 4

In two chains:

$('#next').click(function () {
    $('#container .top-pic').removeClass().next()
        .add($('#container img:first')).last().addClass('top-pic');
});
$('#prev').click(function () {
    $('#container .top-pic').removeClass().prev()
        .add($('#container img:last')).eq(0).addClass('top-pic');
});

Please let me know if this code requires additional explanation.

Answer 5

I came up with this solution, it avoids duplication and it should be doing the least amount of dom queries and if checks. I think it is important to not only stay dry when you have similar logic like previous vs next but also with selectors that jquery will use to get element references.

var first = $('#container img').first(),
    last = $('#container img').last(),
    topPicClass = '.top-pic';


function createOnClick(navFn, endNavEl) {
    return function() {
        var cur = $(topPicClass),
            next = cur[navFn]();

            if(next.length === 0) {
                next = endNavEl;
            }

            cur.removeClass(topPicClass);
            next.addClass(topPicClass);
        }
}

$('#next').click(createOnClick('next', first));
$('#prev').click(createOnClick('prev', last));