Follow up to Shuffling a list of track indices

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.
*
* Example for a ShuffleGenerator situation :
*
*
* {@code
* //Your list
* ArrayList myList = new ArrayList();
* //... fill list with items
*
* ShuffleGenerator generator = new ShuffleGenerator();
* generator.initialize( 546884668, myList.size() );
*
* //Holds now a random index from myList
* int shuffledIndex = generator.getNext();
* }
*
* @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 indexList;
private int currentListIndex;

/**
* Class constructor.
*/
public ShuffleGenerator(){
indexList = new ArrayList();
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 indices = new ArrayList();
for( int i = 0; i = indexList.size() ){
return -1;
}

return indexList.get(currentListIndex);
}

/

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 indexList;

Use:

private List indexList;

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

private final List 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 generate() {
        List remaining = new ArrayList<>(size);
        for (int i = 0; i () {
            @Override
            public Iterator iterator() {
                return new Iterator() {
                    @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 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 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);
    }
}

StackExchange Code Review Q#59401, answer score: 9