[TIMOB-23493] android parity: append file

[frankieandone]

JIRA: https://jira.appcelerator.org/browse/TIMOB-23493

[build]

Can one of the admins verify this patch?

[sgtcoolguy]

Looks like your patch for the Ti.UI.Switch default value got mixed up in here...

[sgtcoolguy]

I think we prefer to enumerate the exact imports rather than using wildcard imports (even though it's much wordier).

[sgtcoolguy]

remove extra newline?

[sgtcoolguy]

remove extra newline?

[sgtcoolguy]

I'd break out the various possible data types we handle into separate private methods, personally.

It looks like you're also re-constructing the BufferedWriter again unnecessarily in case of String.

You're not closing the streams/writers properly in case of Exceptions. You call close on each at the bottom, but if an exception is thrown during #write() those will never get called. You need to wrap them in a finally block. Also, you wouldn't need to explicitly close both writers, but only the "outermost" BufferedWriter (which should internally close the FileWriter it's wrapping).

[sgtcoolguy]

Here's an example of how I'd break down the various type handling into separate methods and properly close the readers/writers.

public boolean append(Object data) throws IOException {
    if (data instanceof String) {
        return append((String) data);
    } else if (data instanceof File) {
        return append((File) data);
    } else if (data instanceof TiBlob) {
        return append((TiBlob) data);
    }
    return false;
}

private boolean append(String data) throws IOException {
    BufferedWriter bufferedWriter = null;
    try {
        bufferedWriter = new BufferedWriter(new FileWriter(file, true));
        bufferedWriter.write(data);
        return true;
    } finally {
        if (bufferedWriter != null) {
            try {
                bufferedWriter.close();
            } catch (Exeption e) {
                // ignore
            }
        }
    }
    // if we threw an exception return false
    return false;
}

private boolean append(File data) throws IOException {
    BufferedWriter bufferedWriter = null;
    BufferedReader bufferedReader = null;
    try {
        bufferedReader = new BufferedReader(new InputStreamReader(new FileInputStream(data)));
        bufferedWriter = new BufferedWriter(new FileWriter(file, true));
        String line = null;
        while((line = bufferedReader.readLine()) != null) {
            bufferedWriter.write(line);
            bufferedWriter.newLine();
        }
        return true;
    }
    finally {
        if (bufferedReader != null) {
            try {
                bufferedReader.close();
            } catch (Exception e) {
                // ignore
            }
        }
        if (bufferedWriter != null) {
            try {
                bufferedWriter.close();
            } catch (Exeption e) {
                // ignore
            }
        }
    }
    // if we threw an exception return false
    return false;
}

private boolean append(TiBlob data) throws IOException {
    //base64 is a binary to text schema
    String input = ((TiBlob) data).toBase64();
    return append(input);
}

Keep in mind, I'm just refactoring your code and not looking into whether we should consider performance implications of how this works for appending to a file. It may be that writing line by line when concatenating two files is a slow way to do things - and maybe we should consider using java.nio Channels. It also looks like this solution is very character/string biased, and could conceivably change the newlines used (say if the reader reads a "\r\n" but the writer.newLine() call just writes a "\n"). If the two files are actually containing binary data, I'm not sure concatenating like this would work (we're using Writers/Readers which basically assume text. For binary we should be copying bytes). Also, the TiBlob may contain pure binary data so converting to base64 may be the wrong thing to do here.

You might want to take a look at improving the unit tests we have here to make sure we are handling all these cases: https://github.com/appcelerator/titanium-mobile-mocha-suite/blob/master/Resources/ti.filesystem.file.test.js#L225

That repo contains our unit test suite that will run against this PR. You can override specific files used by the unit test by adding them to your PR under the tests/Resources folder of titanium_mobile. So, clone that repo locally, copy titanium-mobile-mocha-suite/Resources/ti.filesystem.file.test.js into titanium_mobile/tests/Resources/ti.filesystem.file.test.js and then comment out the parts where we skip the append tests on Android. You can also add some txt/binary files into that folder to use as test inputs. Then write some more tests to ensure that:

• we can concatenate two binary files together and the resulting file is what we'd expect.
• we can concatenate two ascii files together, the second one having multiple types of newlines inside it (\r, \r\n, \n), and that those newlines are preserved appropriately to the concatenated file.
• We can concatenate a binary Ti.Blob to a binary File and the resulting File holds the bytes we expect.
• We can concatenate a text-based Ti.Blob (UTF-8) to a test based (UTF-8) File and the result is what we'd expect.

[frankieandone]

Is it necessary to have the first append(Object data) method in your example because I could overload append method but each one takes different parameter types?

[sgtcoolguy]

I believe it still is.