HiveBrain v1.2.0
Get Started
← Back to all entries
snippetjavaModerate

Read from a file and pick a random line

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
randomfilelinereadpickandfrom

Problem

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 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));
}
}

Solution

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

    public RandomStringPicker(List 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 {
    private final List list;

    public RandomPicker(List 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

Code Snippets

private static final String PATH = "src/app/wordlist.txt";
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);
    }
}
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);
    }
}

Context

StackExchange Code Review Q#47848, answer score: 10

Revisions (0)

No revisions yet.