Searching database, filtering results, adding longs

Question

My needs outpace my abilities. This code (mostly) works, but it is really ugly, doesn't always add correctly, and is in desperate need of some help refactoring. I can't wrap my head around what needs done with it.

I have a SQLite table WorkLog which contains rows of int, long, long, long, and long. They are _id, unused, dayStartTimestamp, activityStartTime, and activityStopTime. Timestamps are stored as epoch time longs. It is possible to have multiple activities on the same day. I am trying to calculate the total time working in a time period defined by lookBackHours.

Time should be the difference between dayStartTime and activityStopTime + 15 minutes for each day. Since there can be many rows with the same dayStartTime, I want to discard entries with duplicate dayStartTimes, only keeping the row with the longest time between activityStartTime and activityStopTime. I am assuming rows in the db/cursor are sorted by dayStartTime and activityStopTime.

In the code below, activityStartTime is represented by TrackerContract.WorkLog.BLOCK_OUT and activityStopTime is rep by TrackerContract.WorkLog.BLOCK_IN. currentTimeMilliMinutes() can be assumed to be the same as System.currentTimeMillis(), and unrelated to the problem of kludgey, buggy code. It simply takes a System.currentTimeMillis() call, constructs a Calendar, then returns a millisecond time constructed from the calendar's Year, Month, Date, Hour, and Minute (stripping seconds and milliseconds from the time).

Here is the basic logic:

• This method searches a db for entries matching the criteria "anything that ended in lookbackHours+15 minutes, or more recently". It puts them into a List of long[2] arrays, storing the dayStartTime and activityStopTime in position 0 and 1 in the array.

• The next loop goes through and attempts to throw away a list entry if the next one has the same dayStartTime and a higher (more recent) activityStopTime.

• The final loop takes the surviving entries and sums the difference in [0] and [1] in millis:

public long timeInHours(long lookbackHours) {
    //returns total time in past hours up to minute
    long sumTotal = 0;
    //search DB for activityStopTime lookBackHours + 15 minutes 
    long timeStamp = currentTimeMilliMinutes() - ((ONE_MINUTE*60L*lookbackHours) + (ONE_MINUTE*15L));
    SQLiteDatabase db = dbHelper.getReadableDatabase();
    Cursor result = db.rawQuery("SELECT * FROM WorkLog WHERE " +
    TrackerContract.WorkLog.COLUMN_BLOCK_IN + ">=" + timeStamp, null);
    result.moveToFirst();
    //define paramaters for loops below
    int fdpStartIndex = result.getColumnIndex(TrackerContract.WorkLog.COLUMN_FDP_BEGIN);
    int blockInIndex = result.getColumnIndex(TrackerContract.WorkLog.COLUMN_BLOCK_IN);

    //insert contents of cursor into list of arrays
    List<long[]> list = new ArrayList<long[]>();
    while (!result.isAfterLast()) {
        long[] temp = new long[2];
        temp[0] = result.getLong(fdpStartIndex);
        temp[1] = result.getLong(blockInIndex);
        list.add(temp);
        result.moveToNext();
    }

    //process list of arrays, if current and next start times are the same, and next end time is bigger, drop current row
    //multiple entries may have same start time, but multiple end times
    //trying to remove all but the longest period per start time
    int length = list.size();
    Calendar c1 = new GregorianCalendar();
    Calendar c2 = new GregorianCalendar();
    for (int i = 0; i < length; i++) {
        if ((i+1) >= length) {

        } else {
            c1.setTimeInMillis(list.get(i)[0]);
            c2.setTimeInMillis(list.get(i+1)[0]);
            if (c1.get(Calendar.MINUTE) == c2.get(Calendar.MINUTE) && 
            c1.get(Calendar.HOUR_OF_DAY) == c2.get(Calendar.HOUR_OF_DAY) && 
            c1.get(Calendar.DAY_OF_YEAR) == c2.get(Calendar.DAY_OF_YEAR) && 
            c1.get(Calendar.YEAR) == c2.get(Calendar.YEAR)) {
            if (list.get(i)[1] < list.get(i+1)[1]) {
                list.remove(i);
                length--;
                }
            }
        }
    }

    //for each remaining entry, find total duration in millis from start to end, add 15 minutes, add to total millis.
    int size = list.size();
    for (int i = 0; i < size; i++) {
        sumTotal += list.get(i)[1] - list.get(i)[0] + (ONE_MINUTE*15L);
    }

        return sumTotal;
}

I'm not explicitly asking for someone to rewrite this, but just pointing out the egregious errors and suggesting ways to make it more manageable/maintainable.

Answer 1

Your code is a bit too messy to understand what you are doing - or why you are doing it, but here are some pointers that can get you started:

• Your method is way too long. Break it up into a couple of sub-methods.

• Using a List<long[]> and using the indexes 0 and 1 for that long[] like you are doing tells me that you should instead use a class to represent your values in the long[]. This will make it a lot clearer about what the 0 index actually is. (Assuming you use proper naming for it)

• Your indentation is a bit off. All the code in your method should be indented one step.

• In your for-loop, you can use:

  for (int i = 0; i < length; i++) {
      if ((i+1) >= length)
          continue;
  

  To get rid of the else statement entirely.

As I said, this is just a start. I am still not sure about what it actually is that you are doing, but this will be more clear once you divide your method into a couple of sub-methods.

Answer 2

I suggest you follow everything what Simon said in his answer. I would like to point out some additional issues:

• Don't ever concatenate strings to build an SQL query (unless you have meticulously checked that there is no possibility of injection attacks, e.g. by properly escaping everything). While it is probably safe in this case, it is a massive code smell. Use parameterized queries instead.

• SELECT * and then extracting those columns you are interested in is totally unecessary. Only ask for what you need.

• When using a C-style for loop over an index `i', you will usually want to specify the upper bound directly and not introduce an extra variable. Instead of

  int length = list.size();
  for (int i = 0; i < length; i++) {
  

  just do for (int i = 0; i < list.size; i++). Of couse this shouldn't be done for more complex bounds (because performance), but here introducing variables is just obfuscation. It also avoids useless names like:

  int length = list.size();
  int size = list.size();
  

• Talking about names: list isn't a good name for a List<…> – I can see from the type what it is. Would timeIntervals be a better name?

• Your three loops can be mostly joined into a single loop, once you view the timeIntervals list as a stack: On every step, you pop the previous element off from the stack, compare the previous and the current element, and if they are sufficiently different, push the both onto the stack.

  The next step is to see that you don't need a whole stack and only the previous interval, and the sum. As an extra, this saves you some memory :-)

• IIRC, the Calendar API shouldn't be used. You might want to look into JodaTime.

• Please don't rely on the ordering of rows from an SQL query. If you need some order, specify an ORDER BY clause in the SQL.

• The 15-Minute extra shouldn't be hardcoded – take it as a parameter to your function and ultimately from a configuration file. Business rules don't belong into compiled code.