Random String Chooser

I was tasked with creating the class RandomStringChooser that prints the strings in the passed array in a random order, based on the for loop in the tester. If the number of iterations in the for loop is larger than the size of the array passed, then “NONE” should be returned. Look at the tester in the program for an example of how this is run.

I’m looking for feedback on code efficiency, neatness, and other nitpicks that can improve the look of my code. I’d very much like to kick bad habits to the curb.

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

// Declare the RandomStringChooser class

public class RandomStringChooser {

    /** Declare any fields (instance variables) **/
    private ArrayList<String> wordArray;
    private Random r;

    /** Declare any constructors */
    public RandomStringChooser(String[] wordArray) {
        this.wordArray = convert(wordArray);
        this.r = new Random();
    }

    /**Convert array to arraylist**/
    private ArrayList<String> convert(String[] array) {
        ArrayList<String> arraylist = new ArrayList<String>();
        for(String string : array) {
            arraylist.add(string);
        }
        return arraylist;
    }

    /** Write the getNext method */
    public String getNext() {
        if(this.wordArray.size() != 0) {
            int index = r.nextInt(this.wordArray.size());
            String word = this.wordArray.get(index);
            if(word != null) {
                this.wordArray.remove(index);
                return word;
            }
            return "NONE";
        }
        return "NONE";
    }

    /** This is a main method for testing the class */
    public static void main(String[] args) {
        System.out.println("It should print the words in the array in a random order and then NONE twice");
        String[] wordArray = {"wheels", "on", "the", "bus"};
        RandomStringChooser sChooser = new RandomStringChooser(wordArray);
        //This loop will output the words in `wordArray` in random order, then "NONE" twice
        for (int k = 0; k < 6; k++) {
            System.out.println(sChooser.getNext() + " ");
        }
        //This loop will output the words in `wordArray` in random order, then stop
        for (int k = 0; k < 4; k++) {
            System.out.println(sChooser.getNext() + " ")
        }

     } // end of main

} // end of class

Interface

When I see a method named get…(), I expect it to behave like a getter method: it should retrieve data, with no side-effects. Your getNext(), though, has the side-effect of consuming one random element from the list. Therefore, next() would be a more appropriate name than getNext().

Once you rename it, you’ll see that your RandomStringChooser should probably just implement the Iterator<String> interface. Java programmers instantly recognize an Iterator and know how one should behave.

Then, you’ll see that the Iterator interface says that next() should throw NoSuchElementException when the elements have been exhausted — and that is what you should do too. In fact, returning “NONE” as a special string to indicate a missing value is a bad practice.

Furthermore, if one of the input strings is in fact null, it shouldn’t be the job of your RandomStringChooser to translate it to “NONE”.

Consider changing the signature of the constructor to public RandomStringChooser(String... words). The code will continue to work the same way, but you would also have the option of invoking it as new RandomStringChooser("wheels", "on", "the", "bus"), with multiple string arguments.

Efficiency

Whenever you extract a random element from the list, you call this.wordArray.remove(index). Removing an element causes all of the subsequent elements to be copied over to fill the gap. Therefore, if you randomly go through an n-element list, you will perform O(n²) copy operations, which is quite poor for efficiency.

A better idea would be to implement a Fisher-Yates shuffle, which just swaps one pair of elements per output string.

Miscellaneous

wordArray is poorly named: it’s actually a java.util.List, not an array.

Your signposting comments are annoying noise to anyone who knows Java.

Suggested implementation

import java.util.Arrays;
import java.util.Iterator;
import java.util.NoSuchElementException;
import java.util.Random;

public class RandomStringChooser implements Iterator<String> {

    private Random r;
    private String[] words;
    private int n;

    public RandomStringChooser(String... words) {
        this.r = new Random();
        this.words = Arrays.copyOf(words, words.length);
        this.reset();
    }

    /**
     * Makes all words eligible to be chosen again, even if they have been
     * previously returned by <code>next()</code>.
     */
    public void reset() {
        this.n = this.words.length;
    }

    public boolean hasNext() {
        return this.n > 0;
    }

    public String next() {
        if (!this.hasNext()) {
            throw new NoSuchElementException();
        }

        // Fisher-Yates shuffle
        int index = r.nextInt(this.n--);
        String word = this.words[index];
        this.words[index] = this.words[this.n];
        this.words[this.n] = word;
        return word;
    }

    /** This is a main method for testing the class */
    public static void main(String[] args) {
        System.out.println("It should print the words in the array in a random order and then NONE twice");
        RandomStringChooser sChooser = new RandomStringChooser(
            "wheels", "on", "the", "bus"
        );
        //This loop will output the words in `wordArray` in random order, then "NONE" twice
        for (int k = 0; k < 6; k++) {
            System.out.println(sChooser.hasNext() ? sChooser.next() : "NONE");
        }
        //This loop will output the words in `wordArray` in random order, then stop
        sChooser.reset();
        for (int k = 0; k < 4; k++) {
            System.out.println(sChooser.hasNext() ? sChooser.next() : "NONE");
        }
     }

}