Get random item from array, within a for loop, then remove from array

Question

I am writing a script that onload will add a class to a random 4 of 12 DIV's and then remove the ID of the DIV from the array.

I have an array setup with all 12 DIV ID's in it.

Sometimes when I reload the page, 4 DIV'S have that class and other times only 3 DIV's have that class.

Kinda stuck on why this is happening. I commented out the remove from array code to see if that was the issue, but still same problem.

Here is my code:

//Randomize Which Shoes Are Positive And Negative
function randomizeShoes(){
    allGroundShoes = new Array('ground_black_1','ground_black_2','ground_brown_1','ground_brown_2','ground_clown_1','ground_clown_2','ground_disco_1','ground_disco_2','ground_moccasins_1','ground_moccasins_2','ground_red_1','ground_red_2');
    for(var i=0; i < 4; i++){
        randomAllGroundShoes = allGroundShoes[Math.floor(Math.random() * allGroundShoes.length)];
        $('#'+randomAllGroundShoes+'').addClass('shoeNegitive');
        //randomShoeID = allGroundShoes.indexOf('randomAllGroundShoes');
        //if(randomShoeID != -1){ allGroundShoes.splice(randomShoeID, 1); }
    }
}

Answer 1

When you remove the found element, you are passing in a string literal instead of the variable name:

allGroundShoes.indexOf('randomAllGroundShoes');

Since there is no element 'randomAllGroundShoes', the element would never be found, and no elements would ever be removed from the array.

It should be:

allGroundShoes.indexOf(randomAllGroundShoes);

But, you are doing the same thing more than once. You don't need to check allGroundShoes.indexOf() at all. You could just store the random number in a variable and reference it again. But, even that is more than you need. Just call splice() to get your value:

randomAllGroundShoes = allGroundShoes.splice(
    Math.floor(Math.random() * allGroundShoes.length), 1)[0];
$('#'+randomAllGroundShoes).addClass('shoeNegitive');

This way, you retrieve your value and remove it from the array in one step - no additional lookup required.

jsfiddle.net/kRNTg

Answer 2

Your remove code is the problem, I believe. Try changing the last two commented out lines to be:

randomShoeID = allGroundShoes.indexOf(randomAllGroundShoes);
if(randomShoeID != -1){ allGroundShoes = allGroundShoes.splice(randomShoeID, 1); }

Answer 3

Possibly, that same number might be getting generated twice.

In that case you have to have a checking mechanism if that number is already used or not.

What you can do is generate an array of 4 unique random number and iterate through that to set the classes.

var unqiue_arr = []
while(arr.length < 8){
  var randomnumber=Math.floor(Math.random()*12)
  var found=false;
  for(var i=0;i<3;i++){
    if(unqiue_arr[i]==randomnumber){found=true;break}
  }
  if(!found)unqiue_arr[unqiue_arr.length]=randomnumber;
}

Answer 4

Your code with the commented out lines has the problem that when the items are not removed, they might get selected twice or more by the random selection.

Your commented out lines had the same problem. You did search for allGroundShoes.indexOf('randomAllGroundShoes'); which (being a string instead of the variable) was never contained in the array.

Use this code instead:

function randomizeShoes(){
    allGroundShoes = ['ground_black_1','ground_black_2','ground_brown_1','ground_brown_2','ground_clown_1','ground_clown_2','ground_disco_1','ground_disco_2','ground_moccasins_1','ground_moccasins_2','ground_red_1','ground_red_2'];
    for (var i=0; i<4; i++){
        var randomShoeID = Math.floor(Math.random() * allGroundShoes.length);
        var randomAllGroundShoes = allGroundShoes[randomShoeID];
        $('#'+randomAllGroundShoes+'').addClass('shoeNegitive');
        allGroundShoes.splice(randomShoeID, 1);
    }
}

There is no need for searching the index of an item which you already know, and there is no need for checking against -1 as you know the item is contained in the array from which you just pulled it out.