Sorting file lines in natural order

Question

I'm new to Java and have been through some tutorials and I'm in the next step of checking how I'm doing now. Is there a better way of doing this?

import java.io.BufferedReader;
import java.io.BufferedWriter;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.io.FileWriter;
import java.io.IOException;
import java.util.Collections;
import java.util.Date;
import java.util.Vector;

import org.apache.log4j.helpers.FileWatchdog;


/**
* 
* Simple class that reads in a file - sorts its lines in natural order,  
* writes it back out to the given filename and adds a timestamp of when it was sorted
* 
* Program will be called with:
* 
* java TestClass <input_filename> <output_filename>
*
*/

public class TestClass {


public static void main(String [] args) throws RuntimeException{
    boolean OVERWRITE_FILE = true;
    //get filename from args
    String inputFilename = args[0];
    String outputFilename = args[1];

    //check usage
    if(args.length != 2){
        System.out.println("Incorrect number of args");
        System.out.println("Correct usage is:");
        System.out.println(" java TestClass <input_filename> <output_filename> ");
        System.exit(-1);
    }

    //read in file into collection
    try {
        BufferedReader in = new BufferedReader(new FileReader(inputFilename));
        Vector fileContents = new Vector<Object>();
        while(in.ready()){
            fileContents.add(in.readLine());
        }

        //sort the collection in natural order
        Collections.sort(fileContents);

        //add timestamp
        fileContents.add("File Sorted on: ");
        fileContents.add(new Date());

        //write outputFile
        BufferedWriter out = new BufferedWriter(new FileWriter(outputFilename, OVERWRITE_FILE));
        for(int i = 0; i < fileContents.size(); i++){
            out.write((String)fileContents.get(i) + "\n");
        }
        //close output file
        out.close();

    } catch (IOException e) {
        System.out.println("Error while performing IO");
    }
}
}

Answer 1

Welcome to Code Review.

Before reviewing your code, it needs to be fixed:

First, make it work...

1. Usage is never printed

   Your program is supposed to print the usage instructions if the wrong number of parameters ist supplied. Check what happens if you run it without any parameters:

   Exception in thread "main"

   java.lang.ArrayIndexOutOfBoundsException: 0
   at codereview.TestClass.main(TestClass.java:30)
   

   To fix this, you need to check the length of args before accessing args[0] and args[1].

2. Failure because of unnecessary cast
   If the correct number of arguments is supplied, your program will fail with the following output:

   Exception in thread "main"

   java.lang.ClassCastException: java.util.Date cannot be cast to java.lang.String
   at codereview.TestClass.main(TestClass.java:59)
   

   This is because you inserted a Date object into your fileContents Vector. Please thoroughly study generics as a better understanding of this important topic would have prevented the error.

   For now, just remove the cast (String)fileContents.get(i) and test your code. A complete solution is given later in this answer. No, really run your code now. You'll see that there is another problem:

3. Sorting is not in the natural order

   You are sorting a list of Strings. This means that the sort order will always be alphabetical (1,3,10,12 would be sorted as 1,10,12,3) even though your documentation comments state that the lines are sorted in their natural order. You can't promise to do that without knowing what kind of input they contain. As it currently stands, you should just change your comments.

4. You are appending to the end of the output file
   The signature of the FileWriter constructor is:

   public FileWriter(String fileName, boolean append) throws IOException
   

   So your OVERWRITE_FILE is a lie. Rename it to APPEND_FILE and set it to false (a cleaner, completely different solution will be given below).

---

... then, clean it up.

General issues

• You have several unused imports. Remove them.

• RuntimeException is unchecked, so you should remove throws RuntimeException from main.

• Your error messages are unhelpful. Error performing IO does not tell the user what went wrong.

Code structure

Readability, maintainability, reusability: you should strive to achieve these three goals in your code (apart from correctness, of course, which goes above all else). One problem with your code is that everything happens directly inside main. You should delegate the bulk of your work into helper methods:

public static void main(String[] args) {
    try
    {
        validateNumberOfArguments(args.length);
        String inputFilename = args[0];
        String outputFilename = args[1];
        List<String> lines = readLinesFrom(inputFilename);
        sortAndAddTimestampTo(lines);
        writeLinesTo(outputFilename, lines);
    }
    catch (IOException e){
        exitBecause("Error accessing file", e);
    }
}

This gives a nice overview of what your code does - similar to article headings in a newspaper or chapters in a book.

Here's the implementation. Your main has been split up into concise methods. Each of them does one thing, so they are easy to understand.

private static void validateNumberOfArguments(int length) {
    if (length != 2) {
        exitBecause("Incorrect number of args", "Correct usage is:", 
        "java TestClass <input_filename> <output_filename>");
    }
}

private static List<String> readLinesFrom(String path) throws IOException {
    return Files.readAllLines(getPath(path), StandardCharsets.UTF_8);
}

private static void sortAndAddTimestampTo(List<String> lines) {
    Collections.sort(lines);
    lines.add("File sorted on: ");
    lines.add(new Date().toString());
}

private static void writeLinesTo(String path, List<String> lines) throws IOException {
    Files.write(getPath(path), lines, StandardCharsets.UTF_8);
}

private static Path getPath(String fileName) {
    return new File(fileName).toPath();
}

private static void exitBecause(Object... reasons) {
    for (Object reason : reasons) {
        System.out.println(reason);
    }
    System.exit(-1);
}

Required imports (JRE7):

import java.io.File;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Collections;
import java.util.Date;
import java.util.List;

The error messages will now be a bit more helpful:

Error accessing file
java.nio.file.NoSuchFileException: input.txt

or

Error accessing file
java.io.IOException: The process cannot access the file because it is being used by 
another process