0
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ I haven't looked at the code yet, but why the double downvote? \$\endgroup\$ – Barry Carter Apr 3 '16 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 Apr 3 '16 at 16:13
1
\$\begingroup\$

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));
}
\$\endgroup\$
  • \$\begingroup\$ Thank you so much, I learned too much from your review :) \$\endgroup\$ – user3006595 Apr 4 '16 at 11:21

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.