Android log in file for testing purpose

Question

I have created a method which shows log in logcat and also writes into the file. Every time this method is called it appends String in file. There are so many read/write operations on disk/non-volatile storage/SD card happening. I want to batch these operations so that minimum SD card read/write happens and i do not miss any log info.

I have a idea to store logs in static StringBuilder variable and check if its length>200000 then write to file otherwise append it in variable. Will that work perfectly, how other developer handle this task?

public static void log(String tag, String s, @ErrorType int error) {
    Log.d(tag, s);
    if (MyApplication.sExternalStorageDirectory == null) {
        new NullPointerException("sExternalStorageDirectory is null")
                .printStackTrace();
        return;
    }
    String fileName = "logs";
    String fileExtension = ".txt";
    if (error == ErrorType.WEBSERVICE_RESPONSE_ERROR) {
        fileName = "web_api_error_log";
    } else if (error == ErrorType.CLIENT_SIDE_ERROR) {
        fileName = "client_log";
    }
    File f = new File(MyApplication.sExternalStorageDirectory, fileName + fileExtension);
    try {
        if (!f.exists()) {
            f.createNewFile();
        }
        BufferedWriter bfw = new BufferedWriter(new FileWriter(f, true));
        bfw.append(tag + "->" + s);
        bfw.newLine();
        bfw.close();
    } catch (IOException e) {
        e.printStackTrace();
    }
}

Answer 1

This review will consist of two parts: One part that addresses the code you present and one part that will make the whole code completely superfluous...

The second part is easy:

Use a logging framework

Use the Android standard Log class. Everything else is just a configuration file. Other than that you won't need any configuration... This whole method becomes completely useless as soon as you do that..

---

Now for the first part:

if (MyApplication.sExternalStorageDirectory == null) {
    new NullPointerException("sExternalStorageDirectory is null")
            .printStackTrace();
    return;
}

This part of the code is basically useless (at least in this place). You should ensure on startup (or to be more correct in onCreate) that this "ExternalStorageDirectory" exists and is not null. Then you should obtain a lock on it and only give that away for onDestroy. If you can guarantee that this exists during the lifetime of your app, you don't need that check.

if (error == ErrorType.WEBSERVICE_RESPONSE_ERROR) {
    fileName = "web_api_error_log";
} else if (error == ErrorType.CLIENT_SIDE_ERROR) {
    fileName = "client_log";
}

This code implies that ErrorType is an enum. If it isn't, it should be. int-flags are a thing of the last millennium.

File f = new File(MyApplication.sExternalStorageDirectory, fileName + fileExtension);
try {
    if (!f.exists()) {
        f.createNewFile();
    }
    BufferedWriter bfw = new BufferedWriter(new FileWriter(f, true));
    bfw.append(tag + "->" + s);
    bfw.newLine();
    bfw.close();
} catch (IOException e) {
    e.printStackTrace();
}

This is where your performance problem is. What you actually want to do is cache the BufferedWriter instances in the static context to avoid the significant I/O overhead you incur when you create them.

You're looking for a "static initializer block":

class YourClass {
    private static final BufferedWriter webserviceLog;
    private static final BufferedWriter clientLog;
    private static final BufferedWriter defaultLog;
    static {
        // ensure external storage directory exists
        // wrap this in a try-catch block!
        webserviceLog = new BufferedWriter(new FileWriter(new File(MyApplication.sExternalStorageDirectory, "web_api_error_log.txt", true)));
        clientLog = new BufferedWriter(new FileWriter(new File(MyApplication.sExternalStorageDirectory, "client_log.txt", true)));
        defaultLog = new BufferedWriter(new FileWriter(new File(MyApplication.sExternalStorageDirectory, "logs.txt", true)));
    }

It should be rather obvious how you have to adapt the code of the method you showed to work with these instances instead of building filenames...