Manual list shuffling in Java produces same result each time

Question

I am trying to create a method for shuffling a list manually. The list is shuffled, but the same result is produced every time. Code example below:

package ch11;
import java.util.ArrayList;
import java.util.Arrays;
public class Chapter_11_E07_ShuffleArrayList {

    public static void main(String[] args) {

        Integer[] array = {1, 2, 3, 4, 5, 6, 7, 8};

        ArrayList<Integer> intList = new ArrayList<>(Arrays.asList(array));

        System.out.println("Before shuffle: ");
        for (Integer x: intList)
            System.out.print(x + " ");

        shuffle(intList);

        System.out.println("\nAfter shuffle: ");
        for (Integer x: intList)
            System.out.print(x + " ");

    }

    public static void shuffle(ArrayList<Integer> intList) {
        // Simple solution
        // java.util.Collections.shuffle(intList);

        // Manual shuffle
        for (Integer x: intList) {

            int newIndex = (int) Math.random() * intList.size();

            Integer temp = intList.get(x);
            intList.set(x, intList.get(newIndex));
            intList.set(newIndex, temp);    
        }   
    }

}

It seems to work to some extent, but is Math.random * intList.size() producing the same random index every time? Inputs are highly appreciated!

Maybe instead of Math.random, try the Random class, it has better results: https://stackoverflow.com/questions/17445813/random-seed-math-random-in-java — Tobias, May 10 '16 at 09:18
You got already an answer here below. Still I have a question: What does your line `Integer temp = intList.get(x);´ mean? It's not an index. If you want the temporary index, you may need indexOf(x) — Martin, May 10 '16 at 09:24
The shuffling algorithm itself looks flawed, see [here](https://en.wikipedia.org/wiki/Fisher%E2%80%93Yates_shuffle). You should pick `nextIndex` between `x` and `intList.size()` and not `0` and `intList.size()`. — biziclop, May 10 '16 at 09:30
@Martin: Maybe I have misunderstood how for each loops work, since I have always used traditional for loops for these types of tasks, but I'm trying to incorporate foreach. If the x variable is not an index, why is it accepted as an index parameter for intList.set(int index, Integer element) ? — Esben86, May 10 '16 at 09:33
Im surprised about that too. Maybe because the value of the Integers are small enough to be in the bounds. All in all you want the Index of x, as described above, then x of course, and then the value of the Integer, where you put your x ( get(newIndex) ). Then you can change their places. — Martin, May 10 '16 at 09:37
@Martin: Your suspicions are correct. Tried to use some higher values in the array, and I got "out of bounds" exception every time. When using intList.indexOf(x) as an index paramater, it works every time. Thanks alot for your input! — Esben86, May 10 '16 at 09:41
@Esben86 I would avoid using the foreach loop in this case altogether. Just use `for (int idx = 0; idx < intList.size(); idx++) { ... }`, it's much more readable that way. — biziclop, May 10 '16 at 09:42
@biziclop: I agree, since im most familiar with the standard for loop. Anyway, i read alot of places that foreach should be preferred over standard for loops, wherever they can be used. These statements are often backed up by that foreach improves readabilty and slight preformance improvement in some cases. Still very new to anything related to programming, and I'm easily influenced by everyone that seems to know what they are talking about. — Esben86, May 10 '16 at 09:47
@Esben86 I think the "where they can be used" clause is important. In most cases you don't care about the index of an element, you just want the elements. That's what foreach is good at. If you need to deal with the element index though, use the traditional `for`. — biziclop, May 10 '16 at 10:01

Sergey Kalinichenko · Accepted Answer · 2016-05-10T09:19:49.183

12

This is because

int newIndex = (int) Math.random() * intList.size();

is not parenthesized properly. It should be

int newIndex = (int)(Math.random() * intList.size());

To avoid simple errors like this, make new Random object, and call nextInt(intList.size()).

edited May 10 '16 at 09:19

answered May 10 '16 at 09:17

Sergey Kalinichenko

714,442
84
1,110
1,523

Using `Random` has the added advantage of producing repeatable results (if you provide it with a seed), making debugging your code a lot easier. – biziclop May 10 '16 at 09:21

score 1 · Answer 2 · answered May 10 '16 at 09:49

1

To show what I've meant in the comments above, here the code:

for (Integer x: intList) {
    int newIndex = (int) (Math.random() * intList.size());
    int oldIndex = intList.indexOf(x);
    Integer temp = intList.get(newIndex);
    intList.set(newIndex, x);
    intList.set(oldIndex, temp);    
}

answered May 10 '16 at 09:49

Martin

239
1
15

Thanks again! I understood what you meant in the comment, and wrote it slightly different than you, but it works fine. – Esben86 May 10 '16 at 09:51
@Martin This only works for lists with unique elements. – biziclop May 10 '16 at 10:03
@biziclop It shuffles the way he wanted. When there are for some reason non-unique elements in the list, then it still exchanges the positions of the items in a random way. Of course the number of non-unique items stays the same. If you have 3 time a 45 in the list, you will still have 3 x 45 after shuffling. – Martin May 10 '16 at 10:11
To prove I made a test-run now with non-unique items: Before shuffle: 1 2 4 4 4 6 7 8 After shuffle: 2 6 4 7 4 1 8 4 – Martin May 10 '16 at 10:27
And isn't it odd that two of the three `4`s is still in its place? When I say it doesn't work, what I mean is that the shuffle will be predictable. As I said, this shuffle algorithm is slightly flawed anyway, but this makes it even more flawed. – biziclop May 10 '16 at 10:29
That must not mean, that they are the "same" 4's as before. In fact the probability, that 2 4's are on their "old" places is very high in this case. But I wont do the math now ;) – Martin May 10 '16 at 10:34

Manual list shuffling in Java produces same result each time

2 Answers2