Building an application activity

I'm beginning the long process of refractoring an Android app written entirely in SpaghettiJava™. I've been programming for a relatively short period of time (two years), and have tried refactoring the code base multiple times, but always get hung up on the wrong things and end up making very slow progress.

I've just finished refactoring the launch activity and would like some feedback. Am I going in the right direction, have I successfully made the class clearer, with better structure and coding practice? What follows is a before and after.

Before

```
public class EntryActivity extends Activity{

private String TAG = "debugEA";

public static final String TM_DIR
= Environment.getExternalStorageDirectory() + File.separator + "TM";

@Override
protected void onCreate(Bundle savedInstanceState) {
Log.debugOut(TAG,"MA.onCreate()");
super.onCreate(savedInstanceState);

Thread.setDefaultUncaughtExceptionHandler(new UncaughtExceptionHandler());

/*
* Create all folders needed for app
*/
File testfordir = new File(TM_DIR);
if (testfordir.exists() == false) {
testfordir.mkdir();
}
testfordir = new File(TM_DIR + File.separator + "kmls");
if (testfordir.exists() == false) {
testfordir.mkdir();
}
testfordir = new File(TM_DIR + File.separator + "kmls" + File.separator + "Save");
if (testfordir.exists() == false) {
testfordir.mkdir();
}
testfordir = new File(TM_DIR + File.separator + "FR");
if (testfordir.exists() == false) {
testfordir.mkdir();
}
testfordir = new File(TM_DIR + File.separator + "ConfigFiles");
if (testfordir.exists() == false) {
testfordir.mkdir();
}
testfordir = new File(TM_DIR + File.separator + "LogHistory");
if (testfordir.exists() == false) {
testfordir.mkdir();
}
testfordir = new File(TM_DIR + File.separator + "CrashReports");
if (testfordir.exists() == false) {
testfordir.mkdir();
}
testfordir = new File(TM_DIR + File.separator + "Zi

You have done some interesting refactoring, but, as you expected, you have not gone far enough. Whenever you have repeating code, you should be doing more functional extraction. Consider this code of yours:

/**
 * Create any missing external application directories.
 * */
private static void makeTmDirs() {
    File testfordir = new File(TM_DIR);
    if (!testfordir.exists()) testfordir.mkdir();

    testfordir = new File(KMLS_DIR);
    if (!testfordir.exists()) testfordir.mkdir();

    testfordir = new File(FR_DIR);
    if (!testfordir.exists()) testfordir.mkdir();

    testfordir = new File(CONFIG_DIR);
    if (!testfordir.exists()) testfordir.mkdir();

    testfordir = new File(LOG_DIR);
    if (!testfordir.exists()) testfordir.mkdir();

    testfordir = new File(CRASH_DIR);
    if (!testfordir.exists()) testfordir.mkdir();

    testfordir = new File(ZIP_DIR);
    if (!testfordir.exists()) testfordir.mkdir();
}

How abut this version:

/**
 * Create any missing external application directories.
 * */
private static void makeTmDirs() {
    String[] folders = {TM_DIR, KMLS_DIR, FR_DIR, CONFIG_DIR, LOG_DIR, CRASH_DIR, ZIP_DIR};
    for (String folder : folders) {
        File dir = new File(folder);
        if (!dir.exists()) {
           dir.mkdir();
        }
    }
}

Note that the code above shows the simplification, but, it should also check the return-value of the mkdir() call.

Your createMissingConfigFiles call is similar:

/**
 * Creates any missing configuration files
 * from the corresponding resource in the raw folder.
 * */
private void createMissingConfigFiles() {
    File testForFile = new File(CONFIG_DIR + File.separator + "logNameValueKey.txt");
    if (!testForFile.exists()) {
        try {
            createFile(testForFile, R.raw.lognamevaluekey);
        } catch (IOException ioe) {
            //TODO: Decide what to do here.
        }
    }

    testForFile = new File(CONFIG_DIR + File.separator + "speciesNameValueKey.txt");
    if (!testForFile.exists()) {
        try {
            createFile(testForFile, R.raw.speciesnamevaluekey);
        } catch (IOException ioe) {
            //TODO: Decide what to do here.
        }
    }

    testForFile = new File(CONFIG_DIR + File.separator + "log_name_codes.txt");
    if (!testForFile.exists()) {
        try {
            createFile(testForFile, R.raw.lognamecodes);
        } catch (IOException ioe) {
            //TODO: Decide what to do here.
        }
    }
}

This would be solved with a method:

private void createMissingConfig(String fileName, String resourceName) {
    File testForFile = new File(new File(CONFIG_DIR), fileName);
    if (!testForFile.exists()) {
        try {
            createFile(testForFile, resourceName);
        } catch (IOException ioe) {
            //TODO: Decide what to do here.
        }
    }

}

Your outer method now becomes:

/**
 * Creates any missing configuration files
 * from the corresponding resource in the raw folder.
 * */
private void createMissingConfigFiles() {
    createMissingConfig("logNameValueKey.txt", R.raw.lognamevaluekey);
    createMissingConfig("speciesNameValueKey.txt", R.raw.speciesnamevaluekey);
    createMissingConfig("log_name_codes.txt", R.raw.lognamecodes);

}

This same pattern can be carried through in almost all your methods....

StackExchange Code Review Q#86531, answer score: 7