Read from a file and pick a random line

Question

I've been learning Java lately and had some great feedback from the previous threads (hangman and guess a number games). I was challenged by a user to try to get this code done, so I'm posting it here for further review. Basically my weakeast (among many) points are exception handling and proper documentation. What do you think about it? How would you improve it?

Code can be found also in GitHub

package app;

import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.util.List;

public class RandomWord {
private String fileName = "src/app/wordlist.txt";
List<String> lines = null;

public static void main(String[] args) {
    RandomWord randomWord = new RandomWord();
    randomWord.init();
}

private void init() {
    try {
        lines = Files.readAllLines(Paths.get(fileName),
                StandardCharsets.UTF_8);
    } catch (IOException e) {
        System.out.println("File can't be opened.");
    }

    int randomWordIndex = getRandomNumber(1, lines.size());
    System.out.println(lines.get(randomWordIndex));
}

/**
 * Returns a random number in the range of the specified min and max
 * parameters.
 * 
 * @param min
 *            the minimum value to return
 * @param max
 *            the maximum value to return
 * @return a random number between the specified range
 */
private int getRandomNumber(int min, int max) {
    return min + (int) (Math.random() * ((max - min) + 1));
}
}

Answer 1

Hardcoding

The path to the file with words is hardcoded. That's not so good. It would be better to make it a command line argument.

If you must hardcode something, make it a constant, for example:

private static final String PATH = "src/app/wordlist.txt";

fileName was not a good name for a path anyway. A "file name" usually refers to the last path segment without parent directories.

Single responsibility principle

The class violates the single responsibility principle: it picks a random word, and it reads a file. I would do something like this instead:

class RandomStringPicker {
    private final List<String> list;

    public RandomStringPicker(List<String> list) {
        this.list = list;
    }

    public String pick() {
        return list.get(pickRandomNumber(list.size()));
    }

    private int pickRandomNumber(int max) {
        return (int) (Math.random() * max);
    }
}

This class only does one thing. You could read the file somewhere else and create a RandomStringPicker class from the lines you read. But now you can reuse this class for other purposes too, for example in unit tests.

This implementation also improves the weakness of your init method. If you forget to call init, your program will compile, but not work. In the example above there is no such uncertainty: if the class is not constructed with a list, it will not compile.

Or you could even go one step further and make the class work with anything, not only strings:

class RandomPicker<T> {
    private final List<T> list;

    public RandomPicker(List<T> list) {
        this.list = list;
    }

    public T pick() {
        return list.get(pickRandomNumber(list.size()));
    }

    private int pickRandomNumber(int max) {
        return (int) (Math.random() * max);
    }
}

Bugs

If you have 10 lines, the call getRandomNumber(1, lines.size()) will return a random number between 1 and 10, inclusive, which has two problems:

• The first line (index = 0) will never be picked
• If 10 is picked, that will be beyond the end of your list and throw an exception

Related to this issue is that you don't really need a minimum parameter for your random number picker. So that's an unnecessary detail in your implementation, you could use something more simple like the example above.

Printing, validating and logging

It's not good practice to print things on the console. If you want to verify your code works, write unit tests. If you want to log messages during runtime, use a logger.

Nitpicks

• The code is not well indented
• Unnecessary brackets in (max - min) + 1. This is the same: max - min + 1

Answer 2

Exception Handling
You wrap the readAllLines with a try catch block, but if an error occurs, you simply write a line to the console, and continue... in the very next line you call line.size(), which will result in a NullPointerException...
Morale of the story is - if you don't have anything smart to do in your catch block - it is better to let the exception bubble up to where it can be handled, or otherwise stop the application completely, notifying the user what exactly was the problems (that's what exceptions are for...)

Documentation
The only documentation you have is for the getRandomNumber(int min, int max) method, and it is full of reiterations on how this methods returns a random number between a min and a max number.
This kind of documentation does not help anyone - the signature of the method is self-explanatory. Documentation has a nasty habit of rotting - since you tend not to maintain it as you maintain the code, and it becomes inaccurate and ultimately misleading.

Documentation would be needed when the method's signature does not convey what the method is doing (like in init), but I would sooner change the name (and the content) of the method before writing documentation, as suggested in the other answer here, since I believe that comments are a code-smell, and that self-explanatory code is an indicator for better design.

Answer 3

It's largely fine by me. :)

Just one glaring point: your implementation would not return the first line, as getRandomNumber() will not return 0.

Answer 4

I'll focus on the choice of algorithm. Choose janos' answer for style review, I'd say.

Files can be arbitrarily large. Larger than what can actually fit in your VM's memory. Your algorithm, while simple, does not deal well with a source of numbers of which the size is not known at the start.

So lets devise a better algorithm that can deal with an arbitrarily large number of inputs, of which we do not know the size beforehand.

To abstract this we will act as if our function gets as input an Iterator<T> (it could be one that reads from a file, or simply iterates a list, or reads primes from a webservice, ...)

First off, we'll need to handle the case that the Iterator is empty. Your current solution simply throws an IndexOutOfBoundsException, but we can do better, and we'll throw a custom exception.

The idea of the algorithm is that we have a current choice, and for each new input we may take it as our current choice instead. Of course, the odds that we should change our current choice should decrease proportionally to the number of inputs we've seen.

We'll need to keep track of a few things :

• the item we would have picked if we'd have seen all inputs : currentPick
• the number of inputs we've seen so far : totalInputs

When we read the first number it's the only one we can pick so far, so currentPick becomes that number and we increase totalInputs.

Then for every next input, we determine the chance that that number becomes our currentPick, and that chance is 1/totalInputs. So we ask our random source to get the next int in [0, totalInputs[ and if it turns out to be 0, we change our currentPick to the latest item.

so something like this :

public static <T> T pickItem(Iterator<T> inputs) {
    if (!inputs.hasNext()) {
        throw new EmptyInputException();
    }

    T currentPick = inputs.next();
    long totalInputs = 1;

    Random random = new Random();

    while (inputs.hasNext()) {
        T next = inputs.next();
        totalInputs++;
        if (shouldPick(totalInputs, random)) {
            currentPick = next;
        }
    }
    return currentPick;
}

private static boolean shouldPick(long totalInputs, Random random) {
    return nextLong(random, totalInputs) == 0;
}

private static long nextLong(Random random, long bound) {
    return (long) (random.nextDouble() * bound);
}