1
\$\begingroup\$

I have these two carousels that randomly pick up pictures. [fiddle] Each of them picks up pictures and links from their own different inner array. It works fine as it is, but I was just wondering if there's any part that I can encapsulate/shorten. The only thing I can come up with that doesn't return any error is them sharing the same $(document).ready(function(){ [fiddle].

$('#carousel1').jsCarousel({ onthumbnailclick: function(src) { load(src); }, autoscroll: true, circular: true, masked: false, itemstodisplay: 3, orientation: 'h' });

$('#carouselu').jsCarousel({ onthumbnailclick: function(src) { load(src); }, autoscroll: true, circular: true, masked: false, itemstodisplay: 3, orientation: 'h' });


var theImages = [
                ['img1','url_1'],
                ['img2','url_2'],
                ['img3','url_3'],
                ['img4','url_4'],
                ['img5','url_5'],   ];

var shuffled = [];

while (theImages.length) {
    shuffled.push(theImages.splice(Math.random() * theImages.length, 1)[0]);
}



$(document).ready(function(){

var imgPath = "/pic/"; 
var urlPath = "/url/";

    $("#carousel1 .1").each(function(index){    
        $(this).prepend('<a href="'+imgPath+shuffled[index][1]+'"><img src="'+urlPath+shuffled[index][0]+'"></a>');

    });

});


/* Second rotator */

var theImagesabc = [
                ['imga','url_a'],
                ['imgb','url_b'],
                ['imgc','url_c'],
                ['imgd','url_d'],
                ['imge','url_f']    ];

var shuffledabc = [];

while (theImagesabc.length) {
    shuffledabc.push(theImagesabc.splice(Math.random() * theImagesabc.length, 1)[0]);
}


$(document).ready(function(){

var imgPath = "/pic/"; 
var urlPath = "/url/";

    $("#carouselu .u").each(function(index){    
        $(this).prepend('<a href="'+imgPath+shuffledabc[index][1]+'"><img src="'+urlPath+shuffledabc[index][0]+'"></a>');

    });

});
\$\endgroup\$
3
\$\begingroup\$

Beyond the repetition in the JS, you've got some repetition across the HTML and JS; I'd avoid having both the array and the empty divs. If you have more divs than you have images in the array, weird stuff starts happening. There's no reason to manually have to manage both of those "lists" when you can just add divs as necessary.

Here's how I might write it, to include that while avoiding the repetition

function randomizeCarousel(selector, images) {
  var i, l, carousel = $(selector);

  function shuffle(images) {
    var i, shuffled = [];
    while(images.length) {
      i = Math.floor(Math.random() * images.length);
      shuffled = shuffled.concat(images.splice(i, 1));
    }
    return shuffled;
  }

  function addImage(src, href) {
    var container = $("<div>"),
        link      = $("<a></a>").attr("href", href).appendTo(container),
        img       = $("<img/>").attr("src", src).appendTo(link);
    carousel.append(container);
  }

  images = shuffle(images.slice(0));
  for(i = 0, l = images.length; i < l; i++) {
    addImage("/pic/" + images[i][0], "/url/" + images[i][1]);
  }

  carousel.jsCarousel({
    onthumbnailclick: function(src) { load(src); },
    autoscroll:       true,
    circular:         true,
    masked:           false,
    itemstodisplay:   3,
    orientation: 'h'
  });
}

$(function () {
  randomizeCarousel("#carousel1", [...]); // add your 1st array here
  randomizeCarousel("#carousel2", [...]); // add your 2nd array here
  // ... etc.
});
\$\endgroup\$
  • \$\begingroup\$ if the carousels have their own image paths and links to different subdomains, instead of using actual paths "/pic/" and "/url/" in addImage("/pic/" + images[i][0], "/url/" + images[i][1]); , is it possible to use a domain as a variable and then change its value for each carousel's ID? \$\endgroup\$ – RedGiant Feb 26 '14 at 12:05
  • \$\begingroup\$ @user35295 Then it'd be easier to simply have the full URLs in the array to begin with. Or you can have a function alter the arrays, before you pass them on to the carousel-builder-function. \$\endgroup\$ – Flambino Feb 26 '14 at 21:04
2
\$\begingroup\$

From a once over:

  • You can use $('#carousel1, #carousel2').jsCarousel(...); or even better

    $('.carouselSelector').jsCarousel(...);
    
  • The name theImagesabc is terrible, just terrible
  • Shuffling each array in it's own for loop is not DRY.
  • Prepending each carousel is not DRY either
  • You could rewrite (document).ready():

    $(document).ready(function(){
    
      var imgPath = "/pic/"; 
      var urlPath = "/url/";
    
      setUpCarousel( '#carousel1 .1', imgPath , urlPath , theImages  );
      setUpCarousel( '#carouselu .u', imgPath , urlPath , theImagesabc  );
    
      function setUpCarousel( selectorString, imgPath , urlPath , images )
      {
        var shuffledImages= [];
        //Shuffle images into shuffledImages
        while (images.length) {
          shuffledImages.push(images.splice(Math.random() * images.length, 1)[0]);
        }
        //Build the carousel
        $( selectorString ).each(function(index){    
          $(this).prepend('<a href="'+urlPath+shuffledImages[index][1]+'"><img src="'+imgPath+shuffledImages[index][0]+'"></a>');
        }); 
      }  
    
    }
    
\$\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.