0
\$\begingroup\$

I got a random image rotator working by using the following script (demo). But I was told that it is a bad practice to extend Array.prototype. And it does. It conflicts with the chained select box unless I modify the selected box script. So I guess it's better to not use Array.prototype.shuffle ,isn't it?

Here's The script:

                Array.prototype.shuffle = function() {
                var s = [];
                while (this.length) s.push(this.splice(Math.random() * this.length, 1));
                while (s.length) this.push(s.pop());
                return this;
            }

            var picData = [
                ['img1','url_1'],
                ['img2','url_2'],
                ['img3','url_3'],

            picO = new Array();
            randIndex = new Array(); 
            for(i=0; i < picData.length; i++){
                picO[i] = new Image();
                picO[i].src = picData[i][0];
                picO[i].alt = picData[i][1];
                randIndex.push(i);
            }
            randIndex.shuffle();
            window.onload=function(){
                var mainImgs = document.getElementById('carouselh').getElementsByTagName('img');


                for(i=0; i < mainImgs.length; i++){
                    mainImgs[i].src = picO[randIndex[i]].src;
                    mainImgs[i].parentNode.href = picData[randIndex[i]][1];
                    mainImgs[i].alt = picData[randIndex[i]][1];
                }

            }

I'm not sure where to start. I'm reading this article, should I do something like

    Array.shuffle = function() {for(var 
a=[];this.length;)a.push(array.splice(Math.random()*array.length,1));
for(;a.length;)this.push(a.pop());return this};

Would anyone please point me in some directions to rewrite that part?

\$\endgroup\$
  • 1
    \$\begingroup\$ Don't forget to declare your variables, i is global. Also literal syntax is always preferred, so [] instead of new Array(). \$\endgroup\$ – elclanrs Feb 22 '14 at 23:35
3
\$\begingroup\$

My advice is to use a known shuffle algorithm for this.

This stackoverflow answer describes the Fisher-Yates shuffle and provides a function for you to use.

https://stackoverflow.com/a/2450976/423413

Bottom line use that function and replace your Array.prototype.shuffle function and call it something like shuffleArray

var shuffleArray = function(arr) {
    ...
}

Then replace the line that says randIndex.shuffle() with a call to the new function.

shuffleArray(randIndex);
\$\endgroup\$
2
\$\begingroup\$

From a once over:

  • As elclanrs mentions, randIndex = []; is preferred over randIndex = new Array();
  • Also as elclanrs mentions, you have global variables: i, picO, and randIndex.
  • If you like to randIndex.shuffle();, then you could add the shuffle to randIndex straight. This is only a good idea if you will only ever shuffle randIndex which seems to be the case. So then you can do this:

    randIndex.shuffle = function shuffle(array) {
      var currentIndex = this.length,
          temporaryValue,
          randomIndex;
    
      // While there remain elements to shuffle...
      while (currentIndex) {
        // Pick a remaining element...
        randomIndex = Math.floor(Math.random() * currentIndex);
        currentIndex--;
    
        // And swap it with the current element.
        temporaryValue = this[currentIndex];
        this[currentIndex] = this[randomIndex];
        this[randomIndex] = temporaryValue;
      }
    }
    
  • This: picO[i] = new Image(); does not seem to make sense, since you are not using the generated image anywhere. You might as well use picO[i] = {};
  • It does not make sense to randomize an array with indexes, you might as well randomize picO and not use randIndex at all
  • In fact, it does not make sense to have an array of arrays which you then convert to an array of objects with the same data points, why not have the array of objects from the start ?
  • mainImgs[i].alt = picData[randIndex[i]][1]; should at least be

    `mainImgs[i].alt = picO[randIndex[i]].alt;`
    
  • All in all, your code could be written as:

    var picData = [
      {src: 'img1', url: 'url_1'},
      {src: 'img2', url: 'url_2'},
      {src: 'img3', url: 'url_3'},
    ];
    //Perfect shuffling routine, I must say yours is shorter and might suffice
    picData.shuffle  = function shuffle() {
      var currentIndex = this.length,
          temporaryValue,
          randomIndex;
      // While there remain elements to shuffle...
      while (currentIndex) {
        // Pick a remaining element...
        randomIndex = Math.floor(Math.random() * currentIndex);
        currentIndex--;
        // And swap it with the current element.
        temporaryValue = this[currentIndex];
        this[currentIndex] = this[randomIndex];
        this[randomIndex] = temporaryValue;
        }
    };
    
    picData.shuffle();
    
    window.onload=function(){
      var mainImgs = document.getElementById('carouselh').getElementsByTagName('img');
    
      for(var i=0; i < mainImgs.length; i++){
        mainImgs[i].src             = picData[i].src;
        mainImgs[i].parentNode.href = picData[i].url;
        mainImgs[i].alt             = picData[i].url;
      }
    };
    

The only problem I still see is stuffin window.onload instead of using an EventListener, I will leave that up to you.

\$\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.