snippetjavaModerate
Read from a file and pick a random line
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
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:
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:
This class only does one thing. You could read the file somewhere else and create a
This implementation also improves the weakness of your
Or you could even go one step further and make the class work with anything, not only strings:
Bugs
If you have 10 lines, the call
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 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.