Follow up to Shuffling a list of track indices

Question

This is the improved code to my last question:

package Utils;

import java.util.ArrayList;
import java.util.Random;

/**
 * A ShuffleGenerator will create a private list of indices and shuffle them.
 * You can access the shuffled indices with the methods getPrevious() and getNext().
 * The method initialize(long, int) can be called as often as you want but should be called at least once
 * after creating an instance of the ShuffleGenerator class. 
 * <p>
 * Example for a ShuffleGenerator situation :
 * <pre>
 * <pre>
 * {@code
 * //Your list
 * ArrayList<Whatever> myList = new ArrayList<Whatever>();
 * //... fill list with items
 * 
 * ShuffleGenerator generator = new ShuffleGenerator();
 * generator.initialize( 546884668, myList.size() );
 * 
 *  //Holds now a random index from myList
 *  int shuffledIndex = generator.getNext();
 * }
 * <pre>
 * @author Davlog
 * @see ShuffleGenerator#ShuffleGenerator()
 * @see ShuffleGenerator#getPrevious()
 * @see ShuffleGenerator#getNext()
 * @see ShuffleGenerator#initialize(long, int)
 * @see ShuffleGenerator#setTrack(int)
 */
public class ShuffleGenerator {

    private ArrayList<Integer> indexList;
    private int currentListIndex;

    /**
     * Class constructor.
     */
    public ShuffleGenerator(){
        indexList = new ArrayList<Integer>();
        currentListIndex = 0;
    }

    /**
     * This will create a shuffled list with the indices from 0 to size-1.
     * When calling again, it will re-create a new shuffle list as long as the seed is different.
     * @param seed for shuffling. Should be always different for re-initialization. 
     * @param size the amount of indices for the shuffled list.
     */
    public void initialize( long seed, int size ){

        indexList.clear();
        currentListIndex = 0;

        if( size > 0 ){

            //fill it with indices
            ArrayList<Integer> indices = new ArrayList<Integer>();
            for( int i = 0; i < size; i++ ){
                indices.add(i);
            }

            Random rnd = new Random(seed);

            do{

                int index = rnd.nextInt(indices.size());
                indexList.add( indices.remove(index) );

            }while( !indices.isEmpty() );           
        }       
    }

    public int getCurrent(){
        //if currentIndex is out of bounds, return -1 
        if( currentListIndex >= indexList.size() ){
            return -1;
        }

        return indexList.get(currentListIndex);
    }

    /**
     * Get the next index of the shuffled list.
     * @return -1 if failed, otherwise the index.
     */
    public int getNext(){
        //if currentIndex is out of bounds, return -1 
        if( currentListIndex+1 >= indexList.size() ){
            return -1;
        }

        return indexList.get(++currentListIndex);
    }

    /**
     * Get the previous index of the shuffled list.
     * @return -1 if failed, otherwise the index.
     */
    public int getPrevious(){
        //if currentIndex is out of bounds, return -1 
        if( currentListIndex-1 < 0 || indexList.isEmpty() ){
            return -1;
        }

        return indexList.get(--currentListIndex);
    }

    /**
     * This method will try to find the index you passed in the shuffled list
     * and set the current index of it to the position of your index. 
     * The methods getPrevious() and getNext() will then give you the index before
     * or after that index.
     * @param index The index from your list.
     * @return True if succeeded, otherwise false.
     */
    public boolean setTrack( int index ){
        /*
         * Loop through the whole list and stop when it has been found
         * Found will then be returned.
         */
        boolean found = false;
        for( int listIndex : indexList ){
            if( listIndex == index ){
                currentListIndex = listIndex;
                break;
            }
        }
        return found;
    }
}

Also added a few methods to it suchs as setTrack(int). Is it better or did I forget something? I did not want to use an int[] array because I want to be able to resize the list by just calling the initialize(long,int) method. However, if you say now that it'd better to create a new instance of a ShuffleGenerator than re-initialize the list, I'll change it.

Answer 1

Reinventing the wheel

For the record, you can use Collections.shuffle to shuffle collections. I suppose you're doing this for the sake of the exercise.

Ergonomy

The implementation is not ergonomic. It's not easy to use. I would expect from a generator to return something that's iterable, so that I can loop over its elements, and stop anytime I want, leaving the rest of the unexplored elements uncalculated. With generators you cannot walk backwards, so the getPrevious method wouldn't really make sense. Or perhaps generator is just not a good term for your purpose.

Excessive comments

There are too many pointless comments.

/**
 * Class constructor.
 */
public ShuffleGenerator(){

// ...

//if currentIndex is out of bounds, return -1 
if( currentListIndex >= indexList.size() ){

Duh?

Good practices

Use the interface type in variable and method declaration, unless you have a specific need of the implementation. So instead of:

private ArrayList<Integer> indexList;

Use:

private List<Integer> indexList;

Also, any member variable that can be final, make it final:

private final List<Integer> indexList;

Unit testing

Some unit tests would be nice, in case some of us might want to refactor your main logic and verify it's still correct. For example:

@Test
public void testGen_2_5() {
    ShuffleGenerator gen = new ShuffleGenerator();
    gen.initialize(2, 5);
    assertEquals(3, gen.getCurrent());
    assertEquals(1, gen.getNext());
    assertEquals(4, gen.getNext());
    assertEquals(0, gen.getNext());
    assertEquals(2, gen.getNext());
}

Suggested implementation

If you don't mind getting rid of getCurrent and getPrevious, then this would be a pure generator implementation that's simpler and more ergonomic:

class ShuffleGenerator {

    private final Random random;
    private final int size;

    ShuffleGenerator(int size, long seed) {
        this.random = new Random(seed);
        this.size = size;
    }

    Iterable<Integer> generate() {
        List<Integer> remaining = new ArrayList<>(size);
        for (int i = 0; i < size; ++i) {
            remaining.add(i);
        }

        return new Iterable<Integer>() {
            @Override
            public Iterator<Integer> iterator() {
                return new Iterator<Integer>() {
                    @Override
                    public boolean hasNext() {
                        return !remaining.isEmpty();
                    }

                    @Override
                    public Integer next() {
                        return remaining.remove(random.nextInt(remaining.size()));
                    }
                };
            }
        };
    }
}

Unit tests:

public class ShuffleGeneratorTest {
    @Test
    public void testGen5() {
        ShuffleGenerator gen = new ShuffleGenerator(5, 0);
        Iterator<Integer> iter = gen.generate().iterator();
        assertEquals(0, (int) iter.next());
        assertEquals(4, (int) iter.next());
        assertEquals(2, (int) iter.next());
        assertEquals(3, (int) iter.next());
        assertEquals(1, (int) iter.next());
    }

    @Test
    public void testCannotIterateBeyond() {
        int size = 5;
        ShuffleGenerator gen = new ShuffleGenerator(size, 0);
        int i = 0;
        for (Integer item : gen.generate()) {
            assertNotNull(item);
            ++i;
        }
        assertEquals(size, i);
    }

    @Test(expected = UnsupportedOperationException.class)
    public void testRemoveIter() {
        ShuffleGenerator gen = new ShuffleGenerator(5, 0);
        Iterator<Integer> iter = gen.generate().iterator();
        iter.remove();
    }

    @Test
    public void testReshuffle() {
        int size = 5;
        ShuffleGenerator gen = new ShuffleGenerator(size, 0);
        int i = 0;
        for (Integer item : gen.generate()) {
            assertNotNull(item);
            ++i;
        }
        assertEquals(size, i);
        i = 0;
        for (Integer item : gen.generate()) {
            assertNotNull(item);
            ++i;
        }
        assertEquals(size, i);
    }
}

Answer 2

Readability

currentListIndex-1 < 0

I'd make that

currentListIndex <= 0

That way you simplify the statement.

Shuffling

As pointed out in a previous answer on your first question, you can make use of Collections.shuffle (example source if you're interested to see how it works). You can even keep using your Random object.

It would look kinda like this:

public void initialize( long seed, int size ){

    indexList.clear();
    currentListIndex = 0;

    if( size > 0 ){

        //fill it with indices
        for( int i = 0; i < size; i++ ){
            indexList.add(i);
        }

        Random rnd = new Random(seed);
        Collections.shuffle(indexList, rnd);         
    }       
}

A further optimization would be to use ArrayList.ensureCapacity. This ensures that there will only be 1 resizing of the array, and could potentially speed up performance quite a bit if someone decided to, say... shuffle their entire song library.

Comments

public int getCurrent()

is missing javadoc. I wouldn't document getters normally, but yours has special behavior if the index is out of bounds. That's why I would add javadoc so people know they might have to deal with -1.

Further polishing

Label the arguments for your methods as final. You don't do anything with them, and with final you ensure this.

Additionally, the class name is a bit off. ShuffleGenerator sounds like it generates... shuffles... which is not really what it does. Maybe it's better off as a ShuffledList (but then you might have both the shuffled and the unshuffled indices)... or a TrackShuffler ... Ah! It's a ShuffledPlaylist!

A ShuffledPlaylist would be different from a normal Playlist in that it is, well, shuffled. And a normal Playlist, well, that's just a list of tracks with a counter indicating the current index. That's my reasoning for naming it a ShuffledPlaylist.

Answer 3

I'd suggest sticking to Resource Acquisition is Initialization (RAII). To that end I would rename the function

public void initialize( long seed, int size )

to

public void reshuffle( long seed, int size )

and make the constructor call reshuffle.

That way the user can't possibly create an object that is in an uninitialized state.