current community

your communities

more stack exchange communities

Stack Exchange
tour help
careers 2.0
Take the 2-minute tour ×
Stack Overflow is a question and answer site for professional and enthusiast programmers. It's 100% free, no registration required.
up vote 0 down vote favorite

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); }
    }
}
share|improve this question
    
Math.floor(Math.random() * allGroundShoes.length) needs to be Math.floor(Math.random() * allGroundShoes.length - 1) because arrays start at 0. The length will return 12 but allGroundShoes[12] will not exist, as the last item would be allGroundShoes[11] since it starts at allGroundShoes[0]. –  MrXenotype Nov 21 '12 at 0:06
    
@MrXenotype It seems like that would be the case, but Math.random() returns a floating point number from 0.0 to less than 1 (i.e. 0 inclusive to 1 exclusive). See the MDN page for details. –  GregL Nov 21 '12 at 0:11
add comment

4 Answers

up vote 2 down vote accepted

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

share|improve this answer
    
Just what I said :-) Yet I'd recommend to use a variable for the index instead of this lengthy one-liner (why use randomAllGroundShoes at all, we could move the random selection into the jQuery selection) ... –  Bergi Nov 21 '12 at 0:39
    
@gilly3 Thanks a lot, PERFECT and explained.... thanks again! –  Papa Wheelz Nov 21 '12 at 0:56
add comment
up vote 0 down vote

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); }
share|improve this answer
1  
After this operation, allGroundShoes will contain 1 element - the element that was just found. –  gilly3 Nov 21 '12 at 0:39
    
@gilly3 Well noted, sir. I had forgotten splice returns the removed array element(s). –  GregL Nov 21 '12 at 2:38
add comment
up vote 0 down vote

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;
}
share|improve this answer
1  
You would think that, hey? But Math.random() never gives a result of 1.0. See this MDN link for details. –  GregL Nov 21 '12 at 0:08
    
@GregL +1 actually yes, in that case is should be the second reason., editing my answer –  Atif Mohammed Ameenuddin Nov 21 '12 at 0:10
    
He is already removing the element from the array (or at least, he's trying to). That is enough to prevent duplicates - as long as it works. This nested loop approach is quite inefficient. –  gilly3 Nov 21 '12 at 0:42
add comment
up vote 0 down vote

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.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.