I need help to enhance the performance of java source code. The code take txt file that has name, amount and date in form (name) (amount) (date yyyy-mm-dd) separated by spaces. After reading the file the data will be (name) (amount) (date dd/MM/yyyy).
public static void main(String[] args) throws IOException {
//we have an lang.ArrayIndexOutOfBoundsException because there is no index 0 in args
long start = System.currentTimeMillis();
File inputFile;
int read;
char[] charBuffer = new char[1024];
String token = "";
int tokenIndex = 0;
String name = null;
int year = -1;
int month = -1;
int day = -1;
String amount = null;
try {
inputFile = new File(args[0]);
FileReader reader = new FileReader(inputFile);
while((read = reader.read(charBuffer)) != -1) {
for (int i = 0; i < read; ++i) {
char c = charBuffer[i];
if (c == '\r' || c == '\n') {
if (token.length() != 0) {
// read date
String date = token;
year = Integer.parseInt(date.substring(0, 4));
month = Integer.parseInt(date.substring(5, 7));
day = Integer.parseInt(date.substring(8, 10));
// flush line
System.out.print(name.toUpperCase());
System.out.print(" ");
System.out.print(amount.indexOf('.') == -1 ? true : false);
System.out.print(" ");
System.out.print(day < 10 ? "0" + day : day);
System.out.print("/");
System.out.print(month < 10 ? "0" + month : month);
System.out.print("/");
System.out.print(year);
System.out.println();
name = null;
year = -1;
month = -1;
day = -1;
amount = null;
token = "";
tokenIndex = 0;
}
}
else if (c == ' ') {
// flush token
if (tokenIndex == 0) {
// read name
name = token;
token = "";
tokenIndex++;
}
else if (tokenIndex == 1) {
// read amount
amount = token;
token = "";
tokenIndex++;
}
}
else token = token + c;
}
}
} catch (ArrayIndexOutOfBoundsException e) {System.out.println (e);}
System.out.println ("Time taken for String in if/else if :"+ (System.currentTimeMillis() - start));
}
I was thinking to separate the nested if-statements because I think its a bad OO code. Any ideas to enhance the code?
-
\$\begingroup\$ I haven't looked at the code yet, but why the double downvote? \$\endgroup\$user1149– user11492016年04月03日 15:24:25 +00:00Commented Apr 3, 2016 at 15:24
-
1\$\begingroup\$ @BarryCarter I didn't downvote, but this post looks problematic so far: the title talks about performance, but the text suggests OP was not able to execute the program. On Code Review we're expecting fully working code. If you cannot execute it, that implies untested, and most likely broken code, which is off-topic. I recommend the poster to test the program well, and adjust the title and text accordingly, to make this point obvious. \$\endgroup\$janos– janos2016年04月03日 16:13:01 +00:00Commented Apr 3, 2016 at 16:13
1 Answer 1
Exceptions used for control flow
inputFile = new File(args[0]);
Using exceptions for the control flow in your program is harmful for the speed of executing. When making a exception, much of the time is spent on constructing the stack trace.
if (args.length == 0) {
System.out.println("Usage: java -jar program.jar <filename>");
return;
}
Using unbuffered IO
FileReader reader = new FileReader(inputFile); while((read = reader.read(charBuffer)) != -1) {
You are using unbuffered IO, this means for every call to the read()
method, a call will be dispatched to the underlying file system to read 1 byte. Wrapping the FileReader
in a BufferedReader
increases the performance, and gives you access to prebuild methods such as nextLine
BufferedReader reader = new BufferedReader(new FileReader(inputFile));
You are trying to reimplement the wheel with your loop
while((read = reader.read(charBuffer)) != -1) { for (int i = 0; i < read; ++i) { char c = charBuffer[i]; if (c == '\r' || c == '\n') { if (token.length() != 0) { // read date ... } } else if (c == ' ') { // flush token if (tokenIndex == 0) { // read name name = token; token = ""; tokenIndex++; } else if (tokenIndex == 1) { // read amount amount = token; token = ""; tokenIndex++; } } else token = token + c; }
The above code is pretty inefficient duo to the large amount of stringbuilder and string object created, a better way is reading a whole line using BufferedReader.readLine()
and splitting that on spaces:
String line;
while((line = reader.readLine()) != null) {
int firstSpace = line.indexOf(' ');
int secondSpace = line.indexOf(' ', firstSpace + 1);
String name = line.subString(0, firstSpace);
String amount = line.subString(firstSpace + 1, secondSpace);
String date = line.subString(secondSpace + 1, line.length());
year = Integer.parseInt(date.substring(0, 4));
...
}
This also decreases the size of the actual code
Don't declare variables until you actually need them
File inputFile;
...
int read;
String token = "";
...
String name = null;
int year = -1;
int month = -1;
int day = -1;
String amount = null;
While the performance impact of this isn't that high, it makes your code look better when seeing the variables declared on the point where it is used.
Not closing your IO streams
CLosing your IO streams is important to prevent the java program from getting errors for too many open files, closing can be done by calling reader.close()
, or by using a try-with-resources block.
Fixed code
After applying all the steps above, you should get a code similar to the following:
public static void main(String[] args) throws IOException { if (args.length == 0) { System.out.println("Usage: java -jar program.jar <filename>"); return; } File inputFile = new File(args[0]); long start = System.currentTimeMillis(); try (BufferedReader reader = new BufferedReader(new FileReader(inputFile))) { String line; while((line = reader.readLine()) != null) { int firstSpace = line.indexOf(' '); int secondSpace = line.indexOf(' ', firstSpace + 1); String name = line.subString(0, firstSpace); String amount = line.subString(firstSpace + 1, secondSpace); String date = line.subString(secondSpace + 1, line.length()); int year = Integer.parseInt(date.substring(0, 4)); int month = Integer.parseInt(date.substring(5, 7)); int day = Integer.parseInt(date.substring(8, 10)); // flush line System.out.print(name.toUpperCase()); System.out.print(" "); System.out.print(amount.indexOf('.') == -1 ? true : false); System.out.print(" "); System.out.print(day < 10 ? "0" + day : day); System.out.print("/"); System.out.print(month < 10 ? "0" + month : month); System.out.print("/"); System.out.print(year); System.out.println(); } } System.out.println ("Time taken for String in if/else if :"+ (System.currentTimeMillis() - start)); }
-
\$\begingroup\$ Thank you so much, I learned too much from your review :) \$\endgroup\$user3006595– user30065952016年04月04日 11:21:50 +00:00Commented Apr 4, 2016 at 11:21