Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

share|improve this question

3 Answers 3

up vote 15 down vote accepted

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();
share|improve this answer
2  
I accepted this as the answer, not for the code, but for the explanation of method responsibilities that my Java professor has failed to mention. (I mention my professor, but I'm already done with his class. This isn't an assignment) Both answers were great though, as the first one from Roger got me thinking about work arounds. and This answer gave me crucial insight into code devlopement. Thank you guys! –  user3517084 Apr 9 at 22:47
    
I am curious, does this actually speed things up for you? If so, by how much? –  user1477388 Apr 10 at 17:45
2  
@user1477388 - each file should be opened once only. The stars and greeks instances should be kept 'permanently'. The Random number generator is also permanent. Getting a random number is fast compared to creating the Random instance. Ys, if you do it with only 1-ever stars/greeks/random then it will be faster. –  rolfl Apr 10 at 17:47
    
I haven't implemented this code, and probably wont, because it helped me to realize some of things I have to learn and relearn. Once I feel confident enough to write better methods and have a more coherent structure, I'll try my hand at making this Planet Generator from scratch, again. –  user3517084 Apr 11 at 19:45

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.

share|improve this answer
    
I think I understand, though I'm not sure. Right now as I have it, it's adding every name the loop iterates over to the field until it reaches randGreek or randName, and that's what's making it slow? So are you saying if I just have it iterate over all the lines in the file it needs to, WITHOUT defining a field, it would perform faster? –  user3517084 Apr 9 at 21:41
    
@user3517084 - How large is the file (KB/MB/) ? –  rolfl Apr 9 at 21:44
    
The biggest one, StarNames, is 2.9KB –  user3517084 Apr 9 at 21:45
3  
@user3517084 That's part of it -- repeatedly looping through the file. But just the overhead of opening it and closing it is also a major slowdown. You could try reading both files into an ArrayList at the start, pass those to the function, and then just randomly index into the list. –  Roger Apr 9 at 21:48

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

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.