Long methods in background task

Question

My code in my main background async task is a mess. Everything is everywhere. I'm afraid of moving things around without something breaking but I definitely think it needs work.

What I can do to make it more condensed, cleaner and refactored?

Here's the bulk of what I want to have judged as code review/ refactoring:

public class RetrieveInfoTask extends AsyncTask<Void, Void, Void> {

        //Called on the UI thread to execute progress bar
        @Override
        protected void onPreExecute() {
            super.onPreExecute();

            progressBar = new ProgressDialog(context);
            progressBar.setIndeterminate(true);
            progressBar.setCancelable(false);
            progressBar.setMessage(MainActivity.this.getString(R.string.retrieve_info));
            progressBar.show();
        }

        //Methods that retrieves information. This is performed in the Background thread
        private void retrieveInfoFromDevice() {

            StringBuilder text = new StringBuilder();
            SQLiteDatabase sqLiteDatabase;
            sqLiteDatabase = dbHandler.getWritableDatabase();
            sqLiteDatabase.beginTransaction();
            String PATIENT_TABLE = "patient";

            try {

               //Take in the encrypted string and perform checksum
               Uri uri = Uri.parse("file:///storage/emulated/0/Download/information.csv");
               File file = new File(uri.getPath());

               //Read the file
               BufferedReader br = new BufferedReader(new FileReader(file));
               String line;
               Long orig_checksum = null;
               Long new_checksum = null;
               boolean firstLine = true;

                while ((line = br.readLine()) != null) {
                    if (firstLine) {

                        orig_checksum = Long.parseLong(line);
                        System.out.println("first line: " + line);
                        firstLine = false;
                        continue;

                    } else {

                        System.out.println("Print the next line :" + line);

                        //Get the checksum
                        new_checksum = Checksum.getChecksum(line);
                        System.out.println(new_checksum);

                        if(new_checksum.equals(orig_checksum)) {

                            System.out.println("Checksum works!");

                            //If values are correct, decrypt the encrypted string
                            Encryption.decrypt(line);

                            //deserialize the string toObject
                            TransferData tdBack = TransferData.fromString(Encryption.decrypt(line));

                            //Create a new map of values, where column names are the keys
                            ContentValues values = new ContentValues();

                            //Save object in db
                            values.put("primary_id", tdBack.getPrimaryID());
                            values.put("region", tdBack.getRegion());
                            values.put("date", tdBack.getKeyDate());
                            values.put("clinic", tdBack.getClinic());

                            //insert the new row
                            sqLiteDatabase.insert(PATIENT_TABLE, null, values);
                            Cursor cursor = sqLiteDatabase.rawQuery("Select * FROM patient", null);
                            Log.d("MainActivity", DatabaseUtils.dumpCursorToString(cursor));

                        }

                    }

                }

            }
            catch(FileNotFoundException e) {
                e.printStackTrace();
            }
            catch(IOException e){
                e.printStackTrace();
            }
            catch(ClassNotFoundException e) {
                e.printStackTrace();
            }

           sqLiteDatabase.setTransactionSuccessful();
           sqLiteDatabase.endTransaction();
        }

        //Methods that retrieves information. This is performed in the Background thread
        private void retrieveCollectionInfo() {


            StringBuilder text = new StringBuilder();
            SQLiteDatabase sqLiteDatabase;
            sqLiteDatabase = dbHandler.getWritableDatabase();
            sqLiteDatabase.beginTransaction();
            String COLLECTION_TABLE = "collection";

            try {

                Uri uri = Uri.parse("file:///storage/emulated/0/Download/information.csv");
                File file = new File(uri.getPath());

                BufferedReader br = new BufferedReader(new FileReader(file));
                String line;
                Long orig_checksum = null;
                Long new_checksum = null;
                boolean firstLine = true;
                String timeOfCollection = null;
                int collectionInterval = 0;
                int numberOfReminders = 0;

                while ((line = br.readLine()) != null) {

                    if (firstLine) {

                        orig_checksum = Long.parseLong(line);
                        System.out.println("first line: " + line);
                        firstLine = false;
                        continue;

                    } else {

                        //Get the checksum
                        new_checksum = Checksum.getChecksum(line);
                        System.out.println(new_checksum);

                        if (new_checksum.equals(orig_checksum)) {

                            System.out.println("Print the next line :" + line);

                            System.out.println("Checksum works!");

                            //If values are correct, decrypt the encrypted string
                            Encryption.decrypt(line);
                            System.out.println("decrypted: " + line);

                            //deserialize the string to Object
                            TransferData tdBack = TransferData.fromString(Encryption.decrypt(line));

                            //Create a new map of values, where column names are the keys
                            ContentValues values = new ContentValues();

                            for (Collection collection : tdBack.getCollections()) {

                                values.put("reminders", collection.getReminders());
                                values.put("region", collection.getRegion().getId());
                                values.put("time_of_collection", collection.getTimeOfCollection());
                                values.put("collection_interval", collection.getCollectionInterval().getId());


                                reminders = collection.getNumberOfReminders();
                                collection = collection.getTimeOfFirstCollection();
                                collectionInterval = collection.getCollectionInterval().getId();

                                //insert the new row
                                sqLiteDatabase.insert(COLLECTION_TABLE, null, values);
                                Cursor cursor = sqLiteDatabase.rawQuery("Select * FROM collection", null);
                                Log.d("MainActivity", DatabaseUtils.dumpCursorToString(cursor));

                            }

                                /* Retrieve a PendingIntent that will perform a broadcast */
                                Intent alarmIntent = new Intent(MainActivity.this, AlarmReceiver.class);
                                pendingIntent = PendingIntent.getBroadcast(MainActivity.this, 0, alarmIntent, 0);

                                AlarmManager manager = (AlarmManager) getSystemService(Context.ALARM_SERVICE);

                                //determine interval from device string
                                if(collectionInterval == 0) {

                                    interval = 3600000;
                                }
                                else if (collectionInterval == 1) {

                                    interval = 86400000;
                                }
                                else if (collectionInterval == 2) {
                                    interval = 604800000;
                                }
                                else if(collectionInterval == 6) {

                                    interval = 43200000;
                                }
                                else if (collectionInterval == 4) {

                                    interval = 900000;
                                }

                                //If the date is set to HHmm, then add current date time
                                SimpleDateFormat format = new SimpleDateFormat("HH:mm");
                                String time = timeOfCollection;

                                long timeOfCollectionInMillis = format.parse(time).getTime();
                                System.out.println("Time in Milis: " + timeOfCollectionInMillis);

                                Calendar now = Calendar.getInstance();
                                now.setTime(new Date());

                                Calendar cal = Calendar.getInstance();
                                Date timedate = format.parse(time);
                                cal.setTime(timedate); // thinks 1970
                                cal.set(Calendar.YEAR, now.get(Calendar.YEAR));
                                cal.set(Calendar.MONTH, now.get(Calendar.MONTH));
                                cal.set(Calendar.DAY_OF_MONTH, now.get(Calendar.DAY_OF_MONTH));

                                //If the time from the db is before now (That is no date set but time set), then set it for tomorrow
                                if (cal.before(now)) {

                                    Date tomorrow = cal.getTime();
                                    cal.setTime(tomorrow);
                                    cal.add(Calendar.DATE, 1);
                                    tomorrow = cal.getTime();
                                    System.out.println("TimeDate for Tomorrow: " + tomorrow);

                                    //convert date to milis
                                    long timeInMilis = (tomorrow.getTime());

                                    //Set Alarm to Repeat
                                    manager.setRepeating(AlarmManager.RTC_WAKEUP, timeInMilis, interval, pendingIntent);


                                    //else, set the alarm for today
                                } else {
                                    timedate = cal.getTime();
                                    System.out.println("TimeDate: " + timedate);

                                    //convert date to milis
                                    long timeInMilis = (timedate.getTime());

                                    //Set Alarm to Repeat
                                    manager.setRepeating(AlarmManager.RTC_WAKEUP, timeInMilis, interval, pendingIntent);


                                }

                            Intent intent = new Intent("com.example.myapp.MainActivity");
                            intent.putExtra("Interval", interval);
                            sendBroadcast(intent);

                        }
                    }
                }

                //Create a new map of values, where column names are the keys
                ContentValues values = new ContentValues();

                    values.put("reminders_remaining",remindersRemaining);
                    values.put("date", new SimpleDateFormat("yyyy-MM-dd").format(new java.util.Date()));
                    values.put("time", new SimpleDateFormat("HH:mm:ss").format(new java.util.Date()));
                    values.put("collection",1);

                //insert the new row
                sqLiteDatabase.insert("alerts", null, values);
                Cursor cursor = sqLiteDatabase.rawQuery("Select * FROM alerts", null);
                Log.d("MainActivity", DatabaseUtils.dumpCursorToString(cursor));


            } catch (IOException e) {
                e.printStackTrace();
            } catch (ClassNotFoundException e) {
                e.printStackTrace();
            } catch (ParseException e) {
                e.printStackTrace();
            }
            sqLiteDatabase.setTransactionSuccessful();
            sqLiteDatabase.endTransaction();
        }

I have a "setup" button that the user clicks from the main page. When they click on the button, a background task is performed.

Here's what is supposed to going on in the background:

Grabs an encrypted file from SD Card, performs a checksum on it first. If the checksum passes, it starts to decrypt the file. If not, it does not proceed, checksum fails.

Once it checksums and decrypts the file, it reads in a serializable object and converts it to a string and saves the values into the database.

One more I noticed is I am redoing all the encrypt, checksum stuff again in another method because I want separate information from that same file. Maybe that needs to be refactored for DRY?

After all that stuff, I want to set an alarm up based on the information in the file. So I take the information from the file and set up an alarm for a specific regions grabbed from the file.

Answer 1

retrieveInfoFromDevice method

You should get rid of unused objects like StringBuilder text = new StringBuilder();

You should get rid of the firstLine stuff by refactoring this out of the while loop. If you use a guard condition you could remove one more indention.

if (line = br.readLine()) == null) { return; }
orig_checksum = Long.parseLong(line);
System.out.println("first line: " + line);

while ((line = br.readLine()) != null) {

    System.out.println("Print the next line :" + line);

    //Get the checksum
    new_checksum = Checksum.getChecksum(line);
    System.out.println(new_checksum);

    if(!new_checksum.equals(orig_checksum)) { continue; }


}

What happens if there aren't any "valid" data in the meaning that the checksums not match for any line ? You are getting a database, starting a transaction and ending the transaction.

It would be better to get all the data of the file and build a List<ContentValues>. So let us refactor this to a separate method.

private List<ContentValues> readPatienInformation(File file) {

    List<ContentValues> contentValues = new ArrayList<ContentValues>();

    BufferedReader br = new BufferedReader(new FileReader(file));
    String line;
    if (line = br.readLine()) == null) { return contentValues; }

    Long originalChecksum = Long.parseLong(line);
    System.out.println("first line: " + line);

    while ((line = br.readLine()) != null){

        if (!originalChecksum.equals(Checksum.getChecksum(line))) { continue; }

        TransferData tdBack = TransferData.fromString(Encryption.decrypt(line));

        ContentValues values = new ContentValues();

        values.put("primary_id", tdBack.getPrimaryID());
        values.put("region", tdBack.getRegion());
        values.put("date", tdBack.getKeyDate());
        values.put("clinic", tdBack.getClinic());

        contentValues.add(values);
    }

    return contentValues;
}

Now, as we have the list of ContentValues let us refactor the former method

private final String fileName = "file:///storage/emulated/0/Download/information.csv";
private void retrieveInfoFromDevice() {

    Uri uri = Uri.parse(fileName);
    File file = new File(uri.getPath());

    List<ContentValues> patientInformation = readPatienInformation(file);

    if (patientInformation.isEmpty()) { return; }

    SQLiteDatabase sqLiteDatabase;
    sqLiteDatabase = dbHandler.getWritableDatabase();
    sqLiteDatabase.beginTransaction();

    final String PATIENT_TABLE = "patient";

    for (ContentValues currentInformation : patientInformation){

        sqLiteDatabase.insert(PATIENT_TABLE, null, currentInformation);
        Cursor cursor = sqLiteDatabase.rawQuery("Select * FROM patient", null);
        Log.d("MainActivity", DatabaseUtils.dumpCursorToString(cursor));

    }

    sqLiteDatabase.setTransactionSuccessful();
    sqLiteDatabase.endTransaction();
}  

You need to add the try..catch yourself as I don't know which exceptions could be thrown.

Removing boilerplate comments will make your code more readable.

Comments should

• used to explain why something is done
• not be used to explain what is done
• should be true

Code should explain by itself what is done.

Some examples

//If values are correct, decrypt the encrypted string
Encryption.decrypt(line);  

looking at the line of code, it is obvious that the content of line should be decrypted and for sure this can only happen if the content is encrypted. "If values are correct" implies that it is possible at this position that the values (which btw.) could be incorrect.

//deserialize the string toObject
TransferData tdBack = TransferData.fromString(Encryption.decrypt(line));

oops, you are again decrypting the same line. Would have been better to store the first result in a string variable. But again the comment "the string" is obvious as the methods name is fromString. Next it isn't deserialized neither toObject nor to Object, but to TransferData.

Answer 2

The program is not decomposed well. Try to split up the large methods to smaller ones. Each method should do one thing and do it well.

Large try blocks are especially bad. When a try block is too large, you might unintentionally catch exceptions that are thrown not in the code written in that block, but in code in other methods you call to from within the block. To have better control over things that can go wrong, try to minimize the size of try blocks always.

Even worse, the exception handling is practically non-existent: when catching an exception, you just print the stack trace. That's not really handling anything, there's no graceful recovery, no message to the user. Worst of all is this:

// ...
catch(IOException e){
    e.printStackTrace();
}
catch(ClassNotFoundException e) {
    e.printStackTrace();
}

sqLiteDatabase.setTransactionSuccessful();   
sqLiteDatabase.endTransaction();

That is, committing the transaction regardless of the exceptions that may have been thrown. If the exceptions are safe to ignore, then you should add a comment to explain, and then you don't need a transaction.

To improve, I suggest to separate the file reading and database writing logic if possible. Whether this is possible or not depends on the size of the csv file, because to separate these operations, you would first have to read in the contents of the csv files in memory. This may seem like a bit of a waste, when you could write data directly to the database without using a buffer, it would significantly improve the readability and also the flow control in the implementation. Because for example, if an error happened during file processing, you obviously don't need to do any database operations.