Planetary Nomenclature

I'm new when it comes to Java and programming in general, but I feel I have a decent grasp on a lot of the fundamentals.

I've created this "random planet generator" that is virtually useless aside from minimal entertainment, but I've created it to help solidify what I've been learning. I'm not asking for code, but for a direction with as much info as possible.

My random planet generator lists:

• name:

• temp:

• size (circumference):

• Density:

• Length of Day:

• Number of Moons:

• Gravity:

• If planet has rings:

My problem is, that for the name, it is horribly inefficient. Before I added the name field, I could generate millions of random planets in seconds. Now, I only set it to 50,000 planets and it still takes a minute or two to generate them all.

Here's the method I use to name the planets:

```
public void setName() throws FileNotFoundException {
String prefix = "";
String greek = "";
int number;
char suffix;
Random rand = new Random();

// randomly pull from list of star names
int randName = rand.nextInt(300);
File prefixFile = new File("StarNames.txt");
Scanner inputFilePrefix = new Scanner(prefixFile);
for (int i = 0; i <= randName; i++) {
prefix = inputFilePrefix.nextLine();
}
inputFilePrefix.close();

// randomly pull of list of Greek names
int randGreek = rand.nextInt(11);
File greekFile = new File("Greek.txt");
Scanner inputFileGreek = new Scanner(greekFile);
for (int i = 0; i <= randGreek; i++) {
greek = inputFileGreek.nextLine();
}
inputFileGreek.close();

// random number
int randNum = rand.nextInt(1000);
number = randNum;

// random letter
int randLetter = rand.nextInt(4);
if (randLetter == 0) {
suffix = 'a';
} else if (randLetter == 1) {
suffix = 'b';
} else if (randLetter == 2) {
suffix = 'c';
} else {
suffix = 'd';
}

// append to class name
this.name = prefix + " "

Review

You have posted one method for review, but that method has multiple responsibilities.

Good code follows the "Single Responsibility Principle". Your method does the following things:

• it creates a random number generator

• it generates a random number

• it opens and reads data from a file and then closes it.

• it generates another random number

• it opens and reads data from another file, and then closes it.

• it generates yet another random number

• then another

• it builds a string value from a number of components.

• sets the value of the name field to this string

That is a lot of things happening in one method.

Additionally, your code has a number of magic numbers: 300 star names, 11 Greek letters, 1000 random values, and 4 letters. Each of these values are coded as magic numbers.

The File names are 'magic' too.

In that list, I can see the following bad practices:

-
two repeated items - selecting a random string from a file

-
a set method (setName) that does not take a value to set (setters should take a value, not generate a value)

-
You create a new Random instance each time

-
you repeatedly open/read/close multiple files.

-
you throw an IOException on a method which does not have a particularly IO-sounding name.

The above things should all be resolved.....
Alternative

Create a class that generates the names for you. Consider this class:

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

public class PlanetaryGenerator {
    
    private static final class RandomLine {
        private final List lines;
        private final Random random = new Random();

        public RandomLine(String filename) throws IOException {
            this.lines = Files.readAllLines(Paths.get(filename), StandardCharsets.UTF_8);
        }
        
        public String getRandom() {
            return lines.get(random.nextInt(lines.size()));
        }
        
    }

    private static final String[] SUFFIXES = {"a", "b", "c", "d"};

    private static final int MAXNUMBER = 1000;
    
    private final RandomLine stars;
    private final RandomLine greekletters;
    private final Random random;
    
    public PlanetaryGenerator(String starsfile, String greekfile) throws IOException {
        this.stars = new RandomLine(starsfile);
        this.greekletters = new RandomLine(greekfile);
        this.random = new Random();
    }

    public String generateName() {
        String prefix = stars.getRandom();
        String greek = greekletters.getRandom();
        int number = random.nextInt(MAXNUMBER);
        String suffix = SUFFIXES[random.nextInt(SUFFIXES.length)];

        // append to class name
        return String.format("%s %s-$d%s", prefix, greek, number, suffix);

    }
}

Then, in your existing code you can create a single instance of this, and just call it when you need a new name:

static PlanetaryGenerator ....

// get a new name:
String name = generator.generateName();

StackExchange Code Review Q#46754, answer score: 15