10
\$\begingroup\$

So I was doing something which creates an airplane with an actual airline code, number, airplane type and so on. The airline code and airplane type are stored in a txt file so I can add more later to my leisure... but the code I came up with to get a random line is extremely messy. Is there a better way? I searched all over stackexchange but it all seemed quite complex for little files which will have only (if ever) a couple dozen lines of content.

public String generate() {

    int totalLines = 0;
    File file = new File("icaocodes.txt");

    BufferedReader br = null;

    try {
        br = new BufferedReader(new FileReader(file));

        while ((br.readLine()) != null) {
            totalLines++;
        }
        br.close();

        br = new BufferedReader(new FileReader(file));

        Random random = new Random();
        int randomInt = random.nextInt(totalLines);
        int count=0;
        String icaocode;
        while ( (icaocode = br.readLine()) != null) {               
            if (count == randomInt) {
                br.close();
                return icaocode;
            }
            count++;
        }
         br.close();


    } catch (FileNotFoundException e) {
        System.out.println("File not found: " + file.toString());
    } catch (IOException e) {
        System.out.println("Unable to read file: " + file.toString());
    } 

    return "Puppies";

}
\$\endgroup\$
6
\$\begingroup\$

Naive Solution

(I say naive because of the assumption that the file you are working with is small. From a general design standpoint, it is usually best to prepare for the worst case and assume you may have to read a super long file at some point in time.)

Read the file line by line into a List or a Map, then fetch a random entry from here. (Judging by the brief details of what you're working on, a Map of different objects might be a good choice if you'd like to parse a file into different objects for flights, etc.)

Probably Better Solution

There's a good discussion over here about Reservoir Sampling.

Side notes

Because you posted in Code Review...

  • Split the reading from the BufferedReaders into separate try-catch blocks. This is just good practice, as in some cases your program may be able to continue executing even if an exception is thrown.
  • Close your readers in a finally. Check out this post on closing readers
  • Make file final.
  • I'm assuming you just slapped the "Puppies" in the return statement, but if you post more complete code, you can get comments and suggestions on all of your code, not just your approach to finding a random line :)
\$\endgroup\$
  • \$\begingroup\$ Yes the "Puppies" thing was just because, in theory, the code should never run that far... if I saw an airplane called Puppies2382, I'd know something went wrong. The code is called simply by: ICAOCode icaocode = new ICAOCode(); StringBuilder sb = new StringBuilder(); sb.append(icaocode.generate()); \$\endgroup\$ – marianopicco Mar 30 '15 at 22:18
  • 1
    \$\begingroup\$ @Madscud maybe it's a bit beyond you given the beginner tag, but the situation you describe is the perfect place to throw an exception. \$\endgroup\$ – Legato Mar 31 '15 at 0:45
  • 1
    \$\begingroup\$ @Madscud There's plenty of ways to indicate something went wrong while executing (returning a garbage String is one way, albeit not a good one), but the trick is doing so in a useful way that can also give a possible reason to help you debug or to give meaningful feedback to a user. \$\endgroup\$ – avojak Mar 31 '15 at 15:35
5
\$\begingroup\$

Will your input files change in the middle of your application running? If not, a better solution would be to read the file just once, and keep the lines in memory until needed. This will significantly improve your performance.

This can be done really easily... but first, there's a much simpler way to read the lines of data from a file:

List<String> lines = Files.readAllLines(Paths.get("icaocodes.txt"));

Note that Files is available in Java 7 and later but, the readAllLines version I use above is in Java 8 only. You need to use the CharSet-based version of readAllLines if you want it in Java 7.

So, you can read your file easily, make it static:

private static final List<String> readLines(String inputfile) {
    try {
        return Files.readAllLines(Paths.get(inputfile));
    } catch (IOException e) {
        return Collections.singletonList("Puppies");
    }
}

Return a list with the Puppies entry if there is a failure. Log it as well!

private static final List<String> IACODES = readLines("iacodes.txt");
private static final Random random = new Random();

Now, how do you get a random value?

public String generate() {
    return IACODES.get(random.nextInt(IACODES.size()));
}
\$\endgroup\$
  • \$\begingroup\$ @Madscud In case it's not clear from this answer (or you didn't read the docs), the Java 8 version of File.readAllLines simply defaults to UTF-8. So make sure that's right, otherwise use the overloaded version that takes a character set. \$\endgroup\$ – Duncan Jones Mar 31 '15 at 7:53
1
\$\begingroup\$

I advise you to divide your code into functions:

public static String[] readAllFileIntoArray(String filename) {
    return FileUtils.readLines(new File(filename), "utf-8");
}

public static String randomChoice(String[] array) {
    java.util.Random random = new java.util.Random();
    int index = random.nextInt(array.length);
    return array[index];
}

public static String fileRandomChoice(String filename) {
    String[] file_lines = readAllFileIntoArray(filename);
    return randomChoice(file_lines);
}
\$\endgroup\$
  • \$\begingroup\$ Java uses String not string (see your return types). Also, in your first method, you should just have return FileUtils.readLines(new File(filename), "utf-8");. There is an extra closing brace there too. Your second method is broken also, it prints the result rather than returning it. The parameter is invalid Java too. Sorry, way too many mistakes, so a down-vote from me. \$\endgroup\$ – Duncan Jones Mar 31 '15 at 7:54
  • \$\begingroup\$ @Duncan Corrected :) Thanks for helping me with the sintax. \$\endgroup\$ – Caridorc Mar 31 '15 at 12:04
  • \$\begingroup\$ That still won't compile. Maybe you should test it in an IDE. \$\endgroup\$ – Duncan Jones Mar 31 '15 at 13:02
  • \$\begingroup\$ @Duncan I am sorry Java is driving me mad, I will leave it broken because I don't understand how to install FileUtils in order to test it. \$\endgroup\$ – Caridorc Mar 31 '15 at 18:58
  • \$\begingroup\$ You could replace your FileUtils code with Java NIO code, which has been available since Java 7. At a quick glance, you mostly just need to correct the fact that file_lines hasn't been declared. So it ought to be String[] file_lines = readAllFileIntoArray(filename); in your last method. \$\endgroup\$ – Duncan Jones Apr 2 '15 at 8:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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