Planetary Nomenclature

Question

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 + " " + greek + "-" + number + "" + suffix;

}

As you can tell, I have 2 text files. One for actual star names, and another for (what I think is) the Greek alphabet. It randomly picks a line to read up to and uses that name. What could be a faster, more efficient way of doing this?

The name structure would essentially be something like:

 Name: Gemini ALPHA-229c

I have every planet generated added to an ArrayList one by one after all planets fields are created.

Any suggestions?

Answer 1

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:

1. it creates a random number generator
2. it generates a random number
3. it opens and reads data from a file and then closes it.
4. it generates another random number
5. it opens and reads data from another file, and then closes it.
6. it generates yet another random number
7. then another
8. it builds a string value from a number of components.
9. 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<String> 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();

Answer 2

Opening and closing both files for every name is very inefficient and is almost certainly the cause of the slow performance. A better bet would be to open both files once, outside the loop, and pass them into the function where the seek can be done.

Alternatively, open both files once, read each of them line by line into an ArrayList, pass those lists to the function, and randomly index into them.

Answer 3

Expanding on the concept of passing in an array (as others have suggested) is that it decouples the method from the storage mechanism. In doing so your code becomes more testable, reusable and extensible, e.g. you can get your data from multiple sources without changing your code. Taking this approach further for more complex collections or code, you'd benefit from abstracting further by passing in an interface bound collection or provider.

To avoid going down the rabbit hole in a single post, here's some further reading to get you started:

Interfaces and Loose Coupling

Dependency Injection

and this list of design patterns