0

I need a shuffle method to shuffle elements of an array which holds objects from another class. At the moment I wrote this code to test with integers first, but it seems to not working perfectly. Most of the elements are being duplicated. Can someone please spot the mistake? And also come up with a more efficient method for this. I am not sure if I can use collections.shuffle because I have further use of my shuffled array later.

  public static void shuffle()
{
  int[] a = new int[52];
  int count = 0;
  int random = 0;
  while (count!=51){
    random = (int)(Math.random() * 52);
    for (int i=0; i <=count; i++){
      if (a[count] != b[random])
        result = true;
    }
    if (result){
      a[count] = b[random];
      count++; 
    }
    b = Arrays.copyOf(a, a.length);
  }
}
Paul
  • 1
  • 2
  • *I am not sure if I can use collections.shuffle because I have further use of my shuffled array later.*. Could you please elaborate? Why couldn't you use your array later if you have called Collections.shuffle() before? – JB Nizet Feb 28 '15 at 07:20
  • umm well, isnt that method for arrayList? – Paul Feb 28 '15 at 07:25
  • No. It's for `List`s. And you can create a `List` view on an array using `Arrays.asList()`. – JB Nizet Feb 28 '15 at 07:27
  • I used the following: Collections.shuffle(Arrays.asList(b)); But it is printing the same exact array in order. – Paul Feb 28 '15 at 07:28
  • Arrays.asList(b) returns a list containing a single element, the array of ints b. Use an array of `Integer`: you can't create a `List`. – JB Nizet Feb 28 '15 at 07:32
  • Yeah. It worked. Thanks. Will it also work with objects as elements of array? – Paul Feb 28 '15 at 07:35
  • Yes. shuffles changes the positions of elements in a list randomly, whatever the type of the elements is. Read the javadoc. – JB Nizet Feb 28 '15 at 07:40
  • Thankyou. If possible, could you spot error in my code? – Paul Feb 28 '15 at 07:57
  • possible duplicate of [Random shuffling of an array](http://stackoverflow.com/questions/1519736/random-shuffling-of-an-array) – DNA Feb 28 '15 at 08:00

3 Answers3

1

First you should not define shuffle() in this way. I would treat b as a parameter and pass it into shuffle() instead of a static field (as your shuffle() is declared as static, your b is also static right? It looks strange to share b between all instances), and result is declared as a local variable.

This part

for (int i=0; i <=count; i++){
    if (a[count] != b[random])
        result = true;
    }

checks whether any one of a[0], a[1] until a[count] is not equal to b[random]. If yes, then assign b[random] to a[count] and increase count by 1. As a[] is not initialized, it is only an array of 0. (a[count] != b[random]) appears to be always true and hence result is true.

Then, for this part,

if (result){
    a[count] = b[random];
      count++; 
    }

say for example random=5, then at the first round of the while loop a[0]=b[5], count=1 (due to count++), and b becomes an array of b[5] and a series of 0. (Due to

b = Arrays.copyOf(a, a.length);

all other elements are replaced by 0.)

Edit: Here I provide a simple method, not thoroughly tested, but should work:

public static int[] shuffle(int[] array) {
    int[] a = new int[array.length];
    //convert int[] to ArrayList<Integer>
    ArrayList<Integer> list = new ArrayList<>();
    for (int i: array)
        list.add(i);
    //now shuffle:
    for (int i=0; i<array.length; i++) {
        int rand = (int)(Math.random()*list.size());
        a[i] = list.remove(rand);
    }
    return a;
}

The array returned is shuffled. Actually I can't say the method "shuffles" the array. It simply creates an empty array, and repeatedly selects an element randomly and put it at the front.

Edit2: This really "shuffles", and this is another approach: does not return a new array. It shuffles the array, 100 times.

public static void shuffle(int[] array) {
    for (int i=0; i<100; i++) {
        int r1 = (int)(Math.random()*array.length);
        int r2 = (int)(Math.random()*array.length);
        int tmp = array[r1];
        array[r1] = array[r2];
        array[r2] = tmp;
    }
}
tonychow0929
  • 458
  • 4
  • 10
0
import java.util.Random;


public class Shuffle {

    public static void main(String[] args) {
        Integer[] a = new Integer[52];
        for (Integer i=0;i<a.length;i++) a[i] = i+1;
        // Let's shuffle
        Random rd = new Random();
        for (Integer i=0;i<a.length;i++){
            Integer changeBy = rd.nextInt(a.length);
            Integer aux=a[i];
            a[i]=a[changeBy];
            a[changeBy] = aux;
        }
        // Now show the shuffled array
        for (Integer i=0;i < a.length; i++) System.out.print(a[i]+",");

    }

}

Hope this small algorithm helps you. As you can see from 2 different runs, it really shuffles your array:

11,1,24,13,28,15,25,48,5,22,12,32,29,42,34,7,33,31,47,18,51,40,8,17,41,20,6,36,21,45,27,52,38,10,30,14,23,19,43,4,50,46,44,3,49,37,35,2,9,26,16,39 3,10,37,26,41,15,28,52,6,24,20,43,33,21,51,32,25,40,50,8,7,5,4,35,13,16,49,17,29,47,12,14,36,39,45,30,2,42,23,38,31,19,27,46,34,11,18,1,22,48,9,44

alphamikevictor
  • 595
  • 7
  • 17
-1

Why no you HashSet for shuffle?

Elements in java.lang.HashSet are shuffling by their hashcode.

public static void shuffle()
{
    int [] b; // you origin array
    Set<Integer> temp = new HashSet<Integer>();
    for (int i : b) {
        temp.add(i);
    }
    Integer [] a = new Integer[b.length];
    a = temp.toArray(a); // new shuffle array

}
lichengwu
  • 4,277
  • 6
  • 29
  • 42
  • You don't really want to create a new `HashSet` everytime you want to shuffle, do you? – Rohit Jain Feb 28 '15 at 07:25
  • We havent studied HashSet yet so I can not use it. – Paul Feb 28 '15 at 07:27
  • Just because a HashSet maintains no order for its elements doesn't mean the order is random. That's a pretty big difference. HashSet doesn't shuffle anything. – JB Nizet Feb 28 '15 at 07:28
  • @RohitJain create new `HashSet` for every shuffle is a bad practice. It wastes memory. – lichengwu Feb 28 '15 at 07:33
  • HashSet uses hashCode. Fine. That's right. But it uses them in a deterministic way. It doesn't shuffle its elements. Execute this: https://ideone.com/4tIZql, which "shuffles" a list of integers 10 times using your technique. Does that look shuffled randomly to you? Compare it to my technique: https://ideone.com/lXTSm4. – JB Nizet Feb 28 '15 at 07:37
  • @JBNizet Integer.hashCode is qual to the primitive `int` value。 – lichengwu Feb 28 '15 at 07:49
  • @lichengwu try it with whatever type you want. It will never be random. Also, a HashSet will remove duplicates. – JB Nizet Feb 28 '15 at 07:55
  • @lichengwu: you get the exact same sequence of strings, from 90 to 10 in order, ten times in a row, and you stil think that's random? I wouldn't open a casino if I were you :) – JB Nizet Feb 28 '15 at 08:27
  • It's random for every given array(not same). :) – lichengwu Feb 28 '15 at 08:31
  • OK. So your definition of "shuffling" is that, if I give you a deck of cards and ask you to shuffle it 10 times, you'll always give me the cards in the same order, because it's always the same deck of cards. Sorry, but that's not just what shuffling means. You're the only one on earth to have this definition of shuffling. – JB Nizet Feb 28 '15 at 08:39