4
\$\begingroup\$

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 + "ZipFiles");
  if (testfordir.exists() == false) {
        testfordir.mkdir();
  }



  File testForFile = new File(Environment.getExternalStorageDirectory()
     +"/TM/ConfigFiles/logNameValueKey.txt");
  if(testForFile.exists() == false){
      Log.debugOut(TAG, "EA.onCreate() logNameValueKey does not exist ");
      createFile("logNameValueKey.txt", R.raw.lognamevaluekey);
  }

  testForFile = new File(Environment.getExternalStorageDirectory() +"/TM/ConfigFiles/speciesNameValueKey.txt");
  if(testForFile.exists() == false){
      Log.debugOut(TAG, "EA.onCreate() speciesNameValueKey does not exist ");
      createFile("speciesNameValueKey.txt", R.raw.speciesnamevaluekey);
  }

  testForFile = new File(Environment.getExternalStorageDirectory() +"/TM/ConfigFiles/log_name_codes.txt");
  if(testForFile.exists() == false){
      Log.debugOut(TAG, "EA.onCreate() log_name_codes does not exist ");
      createFile("log_name_codes.txt", R.raw.lognamecodes);
  }

  SharedPreferences sharedPreferencesGlobal = PreferenceManager.getDefaultSharedPreferences(getBaseContext());
  GlobalStatics.debug = sharedPreferencesGlobal.getBoolean(SetPreferenceActivity.DEBUG_KEY, true);
  Log.debugOut(TAG, "EA.onCreate() GlobalStatics.debug = " + GlobalStatics.debug);

  SharedPreferences sharedPrefThisActivity = getPreferences(Context.MODE_PRIVATE);
  GlobalStatics.setForwarderHarvesterFormat( sharedPrefThisActivity.getInt("harvesterOrforwarderFormat", 0 ));
  if(GlobalStatics.getForwarderHarvesterFormat() == 0){
      Log.debugOut(TAG + "fwh", "MA.onCreate selection = " + 0);

      new AlertDialog.Builder(EntryActivity.this)
      .setTitle("Is this for Forwarder or Harvester?")
      .setItems(new String[]{"Forwarder", "Harvester"}, new DialogInterface.OnClickListener() {
          public void onClick(DialogInterface dialog, int whichButton){
              Log.debugOut("fwh", "fragment selection = " + (whichButton+1));
              selectForwarderOrHarvester( whichButton+1 );
          }
      })
      .setCancelable(false)
      .show();

  }else{
      Log.debugOut(TAG, "MA.onCreate selection = " + GlobalStatics.getForwarderHarvesterFormat());
      Intent mapperIntent = new Intent(this.getBaseContext(), MapperActivity.class);
      startActivity(mapperIntent);
      finish();
  }
}

public void selectForwarderOrHarvester(int selection){
  Log.debugOut(TAG, "selectForwarderOrHarvester selection = " + selection);
  SharedPreferences sharedPref = getPreferences(Context.MODE_PRIVATE);
  SharedPreferences.Editor editor = sharedPref.edit();
  editor.putInt("harvesterOrforwarderFormat", selection);
  editor.commit();
  GlobalStatics.setForwarderHarvesterFormat(selection);
  Intent mapperIntent = new Intent(this.getBaseContext(), MapperActivity.class);
  startActivity(mapperIntent);
  finish();
}
/*
private void createFile(int resID){
   File file = new File("myFile");
      byte[] fileData = new byte[(int) file.length()];
      DataInputStream dis = new DataInputStream(new FileInputStream(file));
      dis.readFully(fileData);
      dis.close();
}
 */
private void createFile(String file, int resID){
  Context context = getBaseContext();
  InputStream inputStream = context.getResources().openRawResource(resID);
  InputStreamReader inputReader = new InputStreamReader(inputStream);
  BufferedReader buffreader = new BufferedReader(inputReader);
  String line;

  try {
      while (( line = buffreader.readLine()) != null) {
          FileUtils.appendLineToFile(line, file);
      }
  } catch (Exception e) {
      return;
  }
}

public void writeFile(String string, String file){
  try{    
      FileWriter out = new FileWriter(Environment.getExternalStorageDirectory() +
              File.separator+"TM" + File.separator + "ConfigFiles" + File.separator
             + file, true);
      Log.debugOut(TAG, "EA.writeFile() write " + string);
      out.write(string + "\n");
      out.close();
  }catch (IOException e) {
      Log.exceptionOut(TAG,"EA.writeFile() Exception");
      //exception handling left as an exercise for the reader 
  }
}
}

After

public class EntryActivity extends Activity {


    /**
     * Root external storage directory for applicaton related files.
     * */
    public static final String TM_DIR = Environment.getExternalStorageDirectory() + File.separator + "TM";

    /**
     * Directory containing kml files.
     * */
    public static final String KMLS_DIR = TM_DIR + File.separator + "kmls";

    /**
     * Directory containing serialized ForwarderRecord objects in xml format.
     * */
    public static final String FR_DIR = TM_DIR + File.separator + "FR";

    /**
     * Directory containing configuration files.
     * */
    public static final String CONFIG_DIR = TM_DIR + File.separator + "ConfigFiles";

    /**
     * Directory containing log files.
     * */
    public static final String LOG_DIR = TM_DIR + File.separator + "LogHistory";

    /**
     * Directory containing crash reports.
     * */
    public static final String CRASH_DIR = TM_DIR + File.separator + "CrashReports";

    /**
     * Directory containing zip files received by IDP.
     * */
    public static final String ZIP_DIR = TM_DIR + File.separator + "ZipFiles";

    /**
     * SharedPreference name used to access saved, user defined, app preferences.
     * */
    public static final String APP_STATE = "APP_STATE";

    /**
     * Key used to retrieve app mode from {@link #APP_STATE} shared preference.
     * */
    public static final String APP_MODE = "APP_MODE";

    /**
     * Title of dialog returned by {@link #getSelectModeDialog()}.
     * */
    public static final String MODE_DIALOG_TITLE = "Is this for Forwarder or Harvester?";

    /**
     * Items of dialog returned by {@link #getSelectModeDialog()}.
     * */
    public static final String[] MODE_DIALOG_ITEMS = {"Forwarder", "Harvester"};

    public enum AppMode implements Serializable {
        UNDEFINED,
        HARVESTER,
        FORWARDER;

        public static AppMode parse(int modeInt) {
            switch (modeInt) {
                case 0:
                    return HARVESTER;
                case 1:
                    return FORWARDER;
                default:
                    return UNDEFINED;
            }
        }
    }

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        makeTmDirs();
        createMissingConfigFiles();

        AppMode appMode = AppMode.parse(
                getSharedPreferences(APP_STATE, MODE_PRIVATE).getInt(APP_MODE, -1));

        if (appMode == AppMode.UNDEFINED) {
            Dialog modeDialog = getSelectModeDialog();
            modeDialog.show();
        } else {
            launchMapperActivity(appMode);
        }
    }

    /**
     * Starts the MapperActivity activity in the given {@code appMode}.
     *
     * @param appMode The mode to launch the app in.
     * */
    private void launchMapperActivity(AppMode appMode) {
        Bundle bundle = new Bundle();
        bundle.putSerializable(APP_MODE, appMode);

        Intent mapperIntent = new Intent(this, MapperActivity.class);
        mapperIntent.putExtras(bundle);
        startActivity(mapperIntent);
        finish();
    }

    /**
     * Returns the dialog used to select the mode to
     * launch the app in. All future launches of the app,
     * will use this mode.
     *
     * @return The {@link android.app.Dialog}.
     * */
    private Dialog getSelectModeDialog() {
        AlertDialog.Builder dialogBuilder;
        (dialogBuilder = new AlertDialog.Builder(EntryActivity.this))
                .setTitle(MODE_DIALOG_TITLE)
                .setItems(MODE_DIALOG_ITEMS, new DialogInterface.OnClickListener() {
                    @Override
                    public void onClick(DialogInterface dialog, int which) {
                        AppMode selectedAppMode = AppMode.parse(which);
                        launchMapperActivity(selectedAppMode);
                    }
                })
                .setCancelable(false);
        return dialogBuilder.create();
    }

    /**
     * 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.
            }
        }
    }

    /**
     * Creates a file from a raw resource.
     *
     * @param file The destination to create the file in.
     * @param resID The raw resource to create the file from.
     * @throws java.io.IOException if an I/O error occurs.
     * */
    private void createFile(File file, int resID) throws IOException {
        InputStream inputStream = getResources().openRawResource(resID);
        InputStreamReader inputReader = new InputStreamReader(inputStream);

        try (BufferedReader buffreader = new BufferedReader(inputReader);
             PrintWriter pw = new PrintWriter(file)) {

            String curLine;

            while ((curLine = buffreader.readLine()) != null) {
                pw.println(curLine);
                pw.flush();
            }
        }
    }

    /**
     * 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();
    }

}
\$\endgroup\$
  • 2
    \$\begingroup\$ Welcome to Code Review! Can you elaborate a bit on what the code in your question does? What purpose does it accomplish? Having that context in plain English will help reviewers help you. \$\endgroup\$ – RubberDuck Apr 10 '15 at 17:38
  • 2
    \$\begingroup\$ Everyone here wants cleaner code. We prefer that you title your question according to the purpose of your application. See How to Ask. \$\endgroup\$ – 200_success Apr 10 '15 at 17:40
7
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ another thing to add is to restrict the visibility of things if possible. In this case, unless needed somewhere else, all constants can have a private visibility. \$\endgroup\$ – Vince Apr 10 '15 at 22:41
  • \$\begingroup\$ @Vince - that's a good observation, why not add it as an answer? \$\endgroup\$ – rolfl Apr 10 '15 at 22:46
  • \$\begingroup\$ I don't think this small comment is worth an answer, I let you add it to your answer. \$\endgroup\$ – Vince Apr 10 '15 at 22:59

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.