My card Shuffler not working can any one tell what is the ambugity in that Shuffler

Question

Try to shuffel 52 card of one deck with the random no.but this is returning same list as given. cardInDeck = 52;

public ArrayList<Card> cardShuffler() {
    int newI;
    Card temp,temp2,temp3;
    Random randIndex = new Random();

    for (int i = 0; i < cardsInDeck; i++) {
        newI = randIndex.nextInt(cardsInDeck);
        Log.i("Nulll", String.valueOf(newI));
        temp = cards.get(i);
        temp2= cards.get(newI);

        //temp3 = temp;
        //temp = temp2;
        //temp2 = temp3;
        cards.set(i, temp2);
        cards.set(newI, temp);

    }
    return cards;
}

This have very low probability,, I have tried many time and result are same. — Awais Ayub, Aug 24 '14 at 08:42
I've tried your code and after defining `cards` and `cardsInDeck` inside that method it works. So I think that the problem is outside of this code. Have you tried to view what's in the `cards` before returning it? — Michał Schielmann, Aug 24 '14 at 08:42
One possibility (I'm guessing without the code calling this method) - You state that the method is returning the same list as given - are you sure you are assigning the return value of this method to something? — jdphenix, Aug 24 '14 at 08:48

score 3 · Accepted Answer · answered Aug 24 '14 at 09:07

Consider using Collections.shuffle().

You can use it like this:

// assume `cards` is a `List`, 
Collections.shuffle(cards); 
// cards is now shuffled

For the curious, look over http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/java/util/Collections.java on line 514.

Michał Schielmann · Answer 2 · 2014-08-24T09:10:05.063

I think that the problem is not in the code you've provided. Try this (added logging and cards initialization) and it works:

private void tryToSuffle()
{
    int newI;
    Integer temp,temp2,temp3;
    Random randIndex = new Random();


    //initializing data.
    Integer cardsInDeck = 52;
    ArrayList<Integer> cards = new ArrayList<Integer>();
    for (int i =0; i < cardsInDeck; i++) {
        cards.add(i);
    }
    //....

    //no change in the algorythm itself
    for (int i = 0; i < cardsInDeck; i++) {
        newI = randIndex.nextInt(cardsInDeck);

        temp = cards.get(i);
        temp2= cards.get(newI);

        cards.set(i, temp2);
        cards.set(newI, temp);

    }

    //printing the result.
    for (int i =0; i < cardsInDeck; i++) {
        System.out.println(cards.get(i));
    }
}

So without changing the main algorythm it works as planned. So probably what your not doing is assigning the returned value to the proper variable. Why inside this method your using some instance field cards? Couldn't you pass them as argument?

EDIT: Just to support what I've writen above I made this test, and it works (without changes in the shuffling algorythm):

public class ShufflerTest {

    private final Integer cardsInDeck = 52;
    public static void main(String args[])
    {
        new ShufflerTest().run();
    }

    private void run() {
        ArrayList<Integer> cards = new ArrayList<Integer>();
        for (int i =0; i < cardsInDeck; i++) {
            cards.add(i);
        }

        cards = tryToShuffle(cards);

        //printing the result.
        System.out.println(cards.toString());
    }

    private ArrayList<Integer> tryToShuffle(final ArrayList<Integer> cards)
    {
        int newI;
        Integer temp,temp2;
        Random randIndex = new Random();

        ArrayList<Integer> shuffledCards = new ArrayList<Integer>();
        shuffledCards.addAll(cards);

        for (int i = 0; i < cardsInDeck; i++) {
            newI = randIndex.nextInt(cardsInDeck);

            temp = shuffledCards.get(i);
            temp2= shuffledCards.get(newI);

            shuffledCards.set(i, temp2);
            shuffledCards.set(newI, temp);

        }

        return shuffledCards;
    }
}

In the example I've added the shuffledCards array not to change the original cards array. This is good practice not to mess with the input param collection as you don't know if some other class uses it also. But if you decide not too use this additional shuffledCards variable it would work also.

My example output is (i've changed it to cards.toString() replacing the loop): [46, 16, 23, 21, 28, 8, 37, 4, 47, 17, 9, 41, 51, 30, 20, 26, 10, 3, 2, 14, 29, 40, 25, 33, 34, 42, 15, 27, 32, 43, 39, 6, 22, 45, 31, 35, 48, 13, 5, 1, 12, 19, 49, 50, 44, 11, 7, 0, 18, 24, 38, 36]

Yes these are shuffeld with same algo but return same card. – Awais Ayub Aug 24 '14 at 09:05 — Awais Ayub, Aug 24 '14 at 09:05
What do you mean `return same card`? My output is shuffled. – Michał Schielmann Aug 24 '14 at 09:06 — Michał Schielmann, Aug 24 '14 at 09:06
Does the code above work as expected? – Michał Schielmann Aug 24 '14 at 09:10 — Michał Schielmann, Aug 24 '14 at 09:10

score 0 · Answer 3 · answered Aug 24 '14 at 08:37

0

You need this fix:

public ArrayList<Card> cardShuffler() {
    int newI;
    Card temp,temp2,temp3;
    Random randIndex = new Random();

    for (int i = 0; i < cardsInDeck; i++) {
        newI = randIndex.nextInt(cardsInDeck - i) + i; // fixed here
        Log.i("Nulll", String.valueOf(newI));
        temp = cards.get(i);
        temp2= cards.get(newI);

        //temp3 = temp;
        //temp = temp2;
        //temp2 = temp3;
        cards.set(i, temp2);
        cards.set(newI, temp);

    }
    return cards;
}

answered Aug 24 '14 at 08:37

shlomi33

1,458
8
9

i have tried before and this one is also produce 0 to 51. no need of it no. are generating well. I'hv check many time. – Awais Ayub Aug 24 '14 at 08:40
What does it change, how do you know it will fix things? For me it works great without your fix. – Michał Schielmann Aug 24 '14 at 08:44
and if i do this will give Null Pointer Exception. if random no genrat 50 and 'i = 50' that will become 100 and this not a index. – Awais Ayub Aug 24 '14 at 08:44
no it can not cause if i = 50 then randIndex.nextInt(52 -50) will return 0 or 1 – shlomi33 Aug 24 '14 at 08:49
BTW, my correction is right in the mathematical aspect. You might still have issues with the deck of cards representation that makes your algo fail. To be convinced please read this p.281 http://www.mktechnicalclasses.com/Notes/Cracking%20the%20Coding%20Interview,%204%20Edition%20-%20150%20Programming%20Interview%20Questions%20and%20Solutions.pdf – shlomi33 Aug 24 '14 at 08:55
But it doesn't solve OP problem. And you didn't explain why is that change necessary. – Michał Schielmann Aug 24 '14 at 09:12

My card Shuffler not working can any one tell what is the ambugity in that Shuffler

3 Answers3