1
\$\begingroup\$

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();
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ More of a general comment about logging, but it's not a good idea to keep logs buffered and only print them out periodically. If something deserves to be logged, then log it right away. If your program crashes with messages in your StringBuilder buffer, you won't see them. \$\endgroup\$ – avojak Sep 27 '16 at 14:01
  • \$\begingroup\$ What if I write remained StringBuilder contents on Application class onCreate. Because if any crash happens, Android System will always call this method. Tell me any other case where this approach can fail. \$\endgroup\$ – Harish Gyanani Sep 28 '16 at 5:33
1
\$\begingroup\$

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...

\$\endgroup\$
  • \$\begingroup\$ static boolean isFirstTime=true; if ( isFirstTime && MyApplication.sExternalStorageDirectory == null ) { isFirstTime=false; new NullPointerException("sExternalStorageDirectory is null") .printStackTrace(); return; } above code of block can be optimized by static boolean variable. \$\endgroup\$ – Harish Gyanani Sep 28 '16 at 5:44
  • \$\begingroup\$ what if my app crashes will i get all remaining logs by making static BufferedWriter objects. will they flush out all data before force app close by system while crashing? \$\endgroup\$ – Harish Gyanani Sep 28 '16 at 5:47
  • \$\begingroup\$ 1. Why I am using my method for log? Ans- because I want to disable/comment it in release build. that's why i am not using android.util.Log class directly. 2. ErrorType using IntDef so enum not required. \$\endgroup\$ – Harish Gyanani Sep 28 '16 at 6:15

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.