Skip to main content
Code Review

Return to Answer

deleted 53 characters in body
Source Link
janos
janos
  • 111.7k
  • 15
  • 152
  • 391

The path to the file with words is a hardcoded. That's not so good. It would be better to usemake it a command line argument and the String[] args parameter of the main method.

The path to the file with words is a hardcoded. That's not so good. It would be better to use a command line argument and the String[] args parameter of the main method.

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.

added 648 characters in body
Source Link
janos
janos
  • 111.7k
  • 15
  • 152
  • 391

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

Nitpicks

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

Printing, validating and logging

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

Nitpicks

  • The code is not well indented
  • Unnecessary brackets in (max - min) + 1. This is the same: max - min + 1
Source Link
janos
janos
  • 111.7k
  • 15
  • 152
  • 391

Hardcoding

The path to the file with words is a hardcoded. That's not so good. It would be better to use a command line argument and the String[] args parameter of the main method.

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.

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.