patternjavaMinor
Sorting file lines in natural order
Viewed 0 times
sortingfileordernaturallines
Problem
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
*
*/
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 ");
System.exit(-1);
}
//read in file into collection
try {
BufferedReader in = new BufferedReader(new FileReader(inputFilename));
Vector fileContents = new Vector();
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");
}
}
}Solution
Welcome to Code Review.
Before reviewing your code, it needs to be fixed:
First, make it work...
-
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"
To fix this, you need to check the length of
-
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"
This is because you inserted a
For now, just remove the cast
-
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.
-
You are appending to the end of the output file
The signature of the
So your
... then, clean it up.
General issues
-
You have several unused imports. Remove them.
-
-
Your error messages are unhelpful.
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
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
Required imports (JRE7):
The error messages will now be a bit more helpful:
or
Before reviewing your code, it needs to be fixed:
First, make it work...
-
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].-
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:-
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.
-
You are appending to the end of the output file
The signature of the
FileWriter constructor is:public FileWriter(String fileName, boolean append) throws IOExceptionSo 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 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 ");
}
}
private static List readLinesFrom(String path) throws IOException {
return Files.readAllLines(getPath(path), StandardCharsets.UTF_8);
}
private static void sortAndAddTimestampTo(List lines) {
Collections.sort(lines);
lines.add("File sorted on: ");
lines.add(new Date().toString());
}
private static void writeLinesTo(String path, List 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
Code Snippets
java.lang.ArrayIndexOutOfBoundsException: 0
at codereview.TestClass.main(TestClass.java:30)java.lang.ClassCastException: java.util.Date cannot be cast to java.lang.String
at codereview.TestClass.main(TestClass.java:59)public FileWriter(String fileName, boolean append) throws IOExceptionpublic 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);
}
}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);
}Context
StackExchange Code Review Q#17781, answer score: 7
Revisions (0)
No revisions yet.