Shuffling a list of track indices

This question has a follow-up: Follow up to Shuffling a list of track indices

I have a bunch of paths in an ArrayList and when a user has activated the shuffle button, I want to have a random order to play all of them. This class should help me get every index once. The methods getNext() and getPrevious() should help me to get the last / next song when the user clicks on either the next or previous button.

public class ShuffleGenerator {

    private ArrayList indexList;
    private int currentIndex;

    public ShuffleGenerator(){
        indexList = new ArrayList();
        currentIndex = 0;
    }

    public void initalize( int seed, int size ){

        if( !indexList.isEmpty() ){
            indexList.clear();
        }

        if( size > 0 ){

            //fill it with indices
            ArrayList indices = new ArrayList();
            for( int i = 0; i  0 );

        }

    }
    /**
     * Get the next index.
     * @return -1 if failed otherweise the index.
     */
    public int getNext(){

        //if currentIndex is invalid
        if( currentIndex+1 >= indexList.size() || indexList.isEmpty() ){
            return -1;
        }

        currentIndex++;
        return indexList.get(currentIndex);
    }

    /**
     * Get the previous index.
     * @return -1 if failed otherwise the index.
     */
    public int getPrevious(){

        //if currentIndex is invalid
        if( currentIndex-1 < 0 || indexList.isEmpty() ){
            return -1;
        }

        //decrement the current index so we can get the previous one
        currentIndex--;

        return indexList.get(currentIndex);
    }

}

I tested it twice and it seems like I get every index from 0 to size-1 in a random order. But is it really giving me every index? I might have made a mistake and just not noticed yet. And is there anything to improve?

I would remove the:

if( size > 0 )

and replace the

do {...} while

with a

while {...}

to simplify the structure of the code.

You can also redesign your algorithm to simplify it further. You can use an Array instead of ArrayList and use the following shuffle algorithm which avoids the use of a supplementary array. Here is the complete code:

import java.util.Random;

public class ShuffleGenerator {
    private int indices[];
    private int nextIndex;

    ShuffleGenerator(int size) {
        indices = new int[size];
        nextIndex = 0;
        for( int i = 0; i =i into position i
            int j = i + rnd.nextInt(indices.length-i);
            // swap i and j
            int c = indices[i];
            indices[i] = indices[j];
            indices[j] = c;
        }
    }

    /**
     * Get the next index.
     * @return -1 if at the end of the sequence.
     */
    public int getNext(){
        if( nextIndex >= indices.length){
            return -1;
        }
        return indices[nextIndex++];
    }

    /**
     * Get the previous index.
     * @return -1 if at the start of the sequence.
     */
    public int getPrevious(){
        if( nextIndex <= 0){
            return -1;
        }
        return indices[--nextIndex];
    }
}

BTW I think it makes more sense to pass a Random object instead of a seed. How do you choose the seed to pass to the function?

added thanks to @Pimgd I noticed that there was a problem in your code: the first call to getNext() should return the first element of the sequence not the second...

Actually the interface getNext,getPrevious is not very good in my opinion (image how you would use it). I think it would be best to have getCurrent, next, prev where next and prev return void. Or you could simply define length and at methods and remove the cursor from the class.

StackExchange Code Review Q#59294, answer score: 7