4
\$\begingroup\$

I am using the following code to copy the contents of two files into one. Earlier I was not closing the Buff and as eclipse gave warnings I did close it. I want to know if this code is susceptible to failure.

public void mergeFiles() throws IOException{      
        ArrayList<String> list = new ArrayList<String>();
        BufferedReader br = null;
        BufferedReader r = null;
        try{
            br = new BufferedReader(new FileReader(getApplicationContext().getFilesDir()+"/BirthdayReminders/fileone.txt"));
            r = new BufferedReader(new FileReader(getApplicationContext().getFilesDir()+"/BirthdayReminders/filetwo.txt"));
            String s1 =null;
            String s2 = null;

            while ((s1 = br.readLine()) != null){                         
                list.add(s1);        
            }
            while((s2 = r.readLine()) != null){    
                list.add(s2);    
            } 
        }
        catch (IOException e){
            e.printStackTrace();
            FLAG =1; 
        }finally{
            br.close();
            r.close();
        }


        BufferedWriter writer=null;
        writer = new BufferedWriter(new FileWriter(getApplicationContext().getFilesDir()+"/BirthdayReminders/output.txt"));
        String listWord;              
        for (int i = 0; i< list.size(); i++){
            listWord = list.get(i);
            writer.write(listWord);
            writer.write("\n");
        }

        writer.close();    
    }

Should I check if br is null in the finally block - my intuition tells me no, because finally will be only executed when try is. I have something over Here which is contradictory to my belief in declaring the variables before try block. What is the best way to do this?

\$\endgroup\$
5
\$\begingroup\$

There are a few issues here with your code. The primary issue is that I believe the whole thing can be accomplished a lot better if you use the library functions currently available in Java. You have already had try-with-resources suggested to you as well.

End-of-line

One quick note, though: your does character decoding of the files you read, and replaces all newline sequences with \n. This may well be the desired behaviour, but you have not specified that as part of your text. It is possible, even likely, that this behaviour will create bugs. For example, if the input code is a UNIX shell script, and you are running this on a windows machine, the copy will break the line-endings, and the scripts will no longer work. On the other hand, it's possible that, when appending the files, that the first file has no end-of-line on the last line, and the new file will be appended at line-end.

These sorts of problems are real, and I have had to handle them before. There is no perfect solution other than to offer different 'compatibility' modes or functions.

Bytes vs. chars.

Your code decodes the input files to chars, and then writes them out as chars again. If your files are known to be text files, and the default encoding for the system is OK, then this should work. You may end up with some incompatibilities in the output though since you do not specify the output charset. It's possible that the input files have characters you may not be able to output. When possible and appropriate you should use UTF-8 or similar as the output encoding. That way you can write out all unicode characters.

Your code assumes char-based inputs, that's fine, but be aware that having a byte-based system would probably be useful as well, for binary data.

DRY

Your code repeats a few blocks. This code repetition would be solved by having the common code extracted to a function.

Exceptions

Your exception handling is very poor. If the first input files have problems reading the data, you report/print an exception, then write out an empty file, and return. If there's a problem with the actual write side, though, you don't print the exception, you just throw it.

Your handling for exceptions should be consistent, and symmetrical. What you do for reads should also be done for writes.

FLAG

What is that? You have an indicator on the class to determine whether there was an error condition? This is not re-entrant, and can lead to problems. The entire method should, in fact, be a static method, and not have any outside effects.

Alternative

Putting all of this together, I would suggest:

/* Transfer chars from source to destination in efficient chunks */
private static final void transfer(final Reader source, final Writer destination) throws IOException {
    char[] buffer = new char[1024 * 16];
    int len = 0;
    while ((len = source.read(buffer)) >= 0) {
        destination.write(buffer, 0, len);
    }
}

public void mergeFiles(final File output, final File inputfile1, final File inputfile2) 
             throws IOException{

    try (
        Reader sourceA = Files.newBufferedReader(inputfile1.toPath());
        Reader sourceB = Files.newBufferedReader(inputfile2.toPath());
        Writer destination = Files.newBufferedWriter(output.toPath(), StandardCharsets.UTF_8); ) {

        transfer(sourceA, destination);
        transfer(sourceB, destination);

    }
}

The above system will write out the data in the same format as the input. Using a buffered approach, rather than a line-by-line approach, will be more efficient. It also preserves the end-of-line system used in the source files, rather than forcing a new, and perhaps broken system on to it.

The output will be UTF-8 because I force it, but you should consider making that a parameter as input to the merge.

\$\endgroup\$
  • \$\begingroup\$ This is insightful, however the knowledge you have provided is way above what I have known and I guess I will have to do a bit of reading on reads and writes again. I am implementing all of your suggestions. \$\endgroup\$ – User3 Jan 16 '15 at 4:15
  • \$\begingroup\$ However I cannot use a try with resources as I am running on android below 19API. \$\endgroup\$ – User3 Jan 16 '15 at 4:19
  • \$\begingroup\$ Nice explanation! Example works as expected! \$\endgroup\$ – aholbreich Mar 30 '16 at 12:55
5
\$\begingroup\$
  1. A little bit about code style: it is conventional to surround binary operators with whitespaces(for instance, getApplicationContext().getFilesDir()+"/BirthdayReminders/fileone.txt"
    should be
    getApplicationContext().getFilesDir() + "/BirthdayReminders/fileone.txt") and there should be a whitespace after a closing bracket and before the opening one(that is,
    } finally { is better than }finally{ and so on).

  2. Yes, you should check that br and r are not null before calling the close method on them. The FileReader constructor can throw an exception. In this case they will be null. However, you can use try-with-resources(if you have Java 7 or later) to make your code more simple and concise:

    try (
        BufferedReader reader1 = new BufferedReader(new FileReader(...));
        BufferedReader reader2 = new BufferedReader(new FileReader(...))
    ) {
        // do something
    } catch (IOException e) {
        // do something else
    }
    
  3. Variable names: r and br are not really descriptive(however, their scope is very narrow so it is not a very big deal in this case). The same holds true for other variables: I would use something like lines instead of list and so on.

  4. This kind of initialization is pointless in my opinion:

    BufferedWriter writer = null;
    writer = new BufferedWriter(new FileWriter(getApplicationContext().getFilesDir() + "/BirthdayReminders/output.txt"));
    

    This one looks better:

    BufferedWriter writer = new BufferedWriter(new FileWriter(getApplicationContext().getFilesDir() + "/BirthdayReminders/output.txt"));
    
  5. The listWord variable and an index-based loop is redundant:

    for (String listWord: list) {
        // do something
    }
    

    For-each loop is shorter and simpler.

\$\endgroup\$
2
\$\begingroup\$

Yes you should check if br is null because the initialization of br can fail if the file is for example not found.

You should also wrap writing to the output in a try finally. Closing the output is more important than closing the input as closing will also flush any buffered output that may not have been written yet.

Hardcoding the file paths is a bad idea and kills any chance at reusing the method without needing copy past, instead just pass the files in as parameters.

As an optimization there is no need to put the strings into a list just to write them out again. Instead write out as you read:

public void mergeFiles(File output, File inputfile1, File inputfile2) throws IOException{
    BufferedWriter writer=null;
    BufferedReader br = null;
    BufferedReader r = null;
    try{
        writer = new BufferedWriter(new FileWriter(output);
        br = new BufferedReader(new FileReader(inputfile1);
        r = new BufferedReader(new FileReader(inputfile2);
        String s1 =null;

        while ((s1 = br.readLine()) != null){                         
            writer.write(s1);
            writer.write("\n");
        }
        while((s1 = r.readLine()) != null){    
            writer.write(s1);
            writer.write("\n"); 
        } 
    }
    catch (IOException e){
        e.printStackTrace();
        FLAG =1; 
    }finally{
        if(br != null)br.close();
        if(r != null)r.close();
        if(writer != null)writer.close();
    }
}
\$\endgroup\$

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.