5
\$\begingroup\$

I have a pretty big method where I read data from files and then put them into a combined file in a specific format based on an ID, first and end timestamp.

I already refactored some of my code to reduce the code complexity of my method, but it's still over 11 right now. So I was wondering if I should leave the method as is or try to find other ways to make it less complex.

This code will run on hardware which is designed by the company I work for so it's important to be as efficient with memory usage as possible.

Can you have a look at the code and help me out with reducing the code complexity by extracting pieces of code and putting them into separate methods?

protected File getSensorData(String sensorId, long first, long end) throws IOException, NumberFormatException {
  /*
   * This method gets data collected from files named after timestamps in seconds
   */
  String[] idArray = new String[0];
  String[] valArray = new String[0];
  long timestamp = 0;
  File formatFile = File.createTempFile("temp", "bufferedsensordata");
  PrintWriter formatPWriter = getFilePrintWriter(formatFile);

  // This sorts numerical instead of alphabetical
  File[] fileArray = sortFiles(file.listFiles());

  for(File f : fileArray) {
    long longTimestamp = getFileTimestamp(f); // Converts filename into timestamp
    if(longTimestamp >= first && longTimestamp <= end) {
      BufferedReader br = new BufferedReader(new FileReader(f));
      String line;
      while(null != (line = br.readLine())) {
        StringTokenizer tokens = new StringTokenizer(line, ";");
        String timestampString = tokens.nextToken();
        String id = tokens.nextToken();
        String val = tokens.nextToken();
        boolean foundId = false;
        int index = 0;

        if(id.startsWith(sensorId) && (Long.parseLong(timestampString) >= first && Long.parseLong(timestampString) <= end)) {
          // id should be used and timestamp falls between the requested range
          if(!Arrays.asList(idArray).contains(id)) {
            idArray = growArray(idArray); // Increases the array size by 1 to make room for missing id
            valArray = growArray(valArray);
          } else {
            foundId = true;
            index = getIdIndex(idArray, id); // Returns location of the id in the array
          }

          if(foundId) {
            valArray[index] = val; // Puts value in valArray at same spot as id in idArray
          } else {
            idArray[idArray.length -1] = id; // Adds new id & val to end of both arrays
            valArray[valArray.length -1] = val;
          }

          // Timestamps are based on seconds so data should only be written if more than a second has passed
          // But data is recorded per millisecond so there can be multiple pieces of data per second
          if(timestamp != Long.parseLong(timestampString)) {
            if(timestamp != 0) {
              String dataLine = String.valueOf(timestamp);
              for(int i = 0; i < valArray.length; i++) {
                dataLine = dataLine + ";" + valArray[i];
                valArray[i] = "";
              }
              formatPWriter.println(dataLine);
            }

            // Timestamp is update to the next second
            timestamp = Long.parseLong(timestampString);
          }
        }
      }
    }
  }

  File finalFile = newFinalFile();
  PrintWriter finalPWriter = getFilePrintWriter(finalFile);
  // Write idArray as first line (header)
  writeHeader(idArray, finalPWriter);
  // Write valArray as the rest of the file (body)
  writeContent(formatFile, finalPWriter);

  formatFile.delete();

  return finalFile;
}
\$\endgroup\$
  • \$\begingroup\$ Is there a reason why you use arrays instead of lists? \$\endgroup\$ – mheinzerling Jul 4 '13 at 8:29
  • \$\begingroup\$ Are this you usual comments or did you add them for the question only? \$\endgroup\$ – mheinzerling Jul 4 '13 at 8:31
  • \$\begingroup\$ @mnhg I added them for the question only and I'm using arrays because they told me to use arrays, I think because they wanted to keep the memory usage to a minimum, not quite sure to be honest. \$\endgroup\$ – JREN Jul 4 '13 at 8:34
  • 1
    \$\begingroup\$ Who is "they" and are there really memory constrains (embedded environment?) Usually I recommend to optimize readability/maintainability over abstract memory/performance goals. \$\endgroup\$ – mheinzerling Jul 4 '13 at 8:38
  • 1
    \$\begingroup\$ You are right, but this is an important detail we need to know while reviewing the code. That's why I ask. \$\endgroup\$ – mheinzerling Jul 4 '13 at 8:43
5
\$\begingroup\$

I have to scroll to read it all, so without looking at the code: Yes, split it.

And now with looking at the code: I would try to extract the content of your for loop and give the method a nice name.

Afterward you can check how to split this method.

Maybe you want to have a look at one-thing-extract-till-you-drop (also check the comments)

\$\endgroup\$
  • \$\begingroup\$ Thanks for that link, I'll check it out. Definitely looks interesting. \$\endgroup\$ – JREN Jul 4 '13 at 8:41
  • \$\begingroup\$ +1 for the link. The "A-Ha" here is that software principles are fractal; certainly single responsibility principle (SRP) applies at the method level. And note that applying any principle inherently brings other guidelines into play. In this case encapsulate complexity, appropriate abstraction, modular construction, readability, understandability. \$\endgroup\$ – radarbob Jul 4 '13 at 17:39
3
\$\begingroup\$

Using ArrayList should not be more inefficient than the code above. As your code above contains a Time and Space inefficient implementation of it.

 String[] idArray = new String[0];
 // ...
 // Increases the array size by 1 to make room for missing id
 idArray = growArray(idArray); 
 // ...
 idArray[idArray.length -1] = id;

Java arrays are of fixed size. Only way to "grow" and array is to allocate a new array of larger size, copy its contents to the new array, and update all references to the old array with the references to the new one. You need more than twice the size of an array to grow it by only one. Adding an element is O(n). Filling an array with n elements takes O(n^2) time and ignoring GC O(n^2) space. If you really operate in the large n border cases, where hand coded data structures come into the picture at all, you will hit GC more and more often until you hit it for every insertion.

Whereas ArrayList which grows by a constant factor instead of a constant amount, operates more efficiently and degrades more gracefully, as it hits GC less frequenty until it reaches OutOfMemmoryError.

For efficiency (both time and space), you should allocate enough space right in the beginning, whether you use an array or ArrayList, anyway. If you know you have all the space you need, using an array could save you a few instructions per access.

Also the following two conditionals test for the same thing and rearranging it will make it more readable:

boolean foundId = false; // Declared out of the scoped it is used...
// ....
if (!Arrays.asList(idArray).contains(id)) {
    idArray = growArray(idArray);
    valArray = growArray(valArray);
} else {
    foundId = true;
    index = getIdIndex(idArray, id);
}

if (foundId) {
    valArray[index] = val;
} else {
    idArray[idArray.length - 1] = id;
    valArray[valArray.length - 1] = val;
}

can be rewritten as:

if (Arrays.asList(idArray).contains(id)) {
    // scanning the array for the same string for a second time below:
    index = getIdIndex(idArray, id); 
    valArray[index] = val;
} else {
    idArray = growArray(idArray);
    idArray[idArray.length - 1] = id;

    valArray = growArray(valArray);
    valArray[valArray.length - 1] = val;
}

which in turn can be rewritten as:

int index = Arrays.asList(idArray).indexOf(id);

if (index >= 0) {
    valArray[index] = val;
} else {
   // ... same as above
}

Of course you could just use a LinkedHashMap and optimize only if necessary. (How do you decide if you need to optimize?)

LinkedHashMap<String, String> dict = new LinkedHashMap<String, String>(largeEnough);
// ...     
dict.put(id, val);

Also this code does not do any proper resource management.

\$\endgroup\$
3
\$\begingroup\$

If you split a method into smaller ones you have one great benefit: you can give a name to the section that you extracted. And if you can give it a name you can give it a meaning.

If a method is very large it means that you are mixing a lot different aspects that the code is about. You can find these aspects easily while you implement a method. Every time you come to the point when you think "Hmm, I need to do this/that now" you have found one aspect (it is the "this/that").

Now you have 4 ways of handling it:

  1. Ignore it.
  2. Introduce a local variable.
  3. Introduce a method.
  4. Introduce a class.

To make it concrete an example will show it. The example contains the thoughts of the developer as comments. Note that this is only to demonstrate how the methodology I explained above works. The names you give the variables, methods and classes should make these comments needless. If so... you have written clean code.

protected File getSensorData(String sensorId, long fromTime, long toTime)
        throws IOException, NumberFormatException {
    // Hmm, I need to get all files that contain sensor data
    File[] sensorDataFiles = sensorDataDir.listFiles();

    // Hmm, I need to filter the sensor data by their timestamp
    File[] inTimeRangeSensorDataFiles = 
                 filterSensorDataFiles(sensorDataFiles, fromTime, toTime);

    // and so on
}


private File[] filterSensorDataFiles(File[] sensorDataFiles, long fromTime, long toTime){
    List<File> filteredSensorData = new ArrayList<File>();
    for (File sensorData : sensorDataFiles) {

        // Hmm, I need to know if the sensorData is in the time range
        long longTimestamp = getFileTimestamp(sensorData);
        boolean inTimeRange = longTimestamp >= fromTime && longTimestamp <= toTime;

        if (inTimeRange) {
            filteredSensorData.add(sensorData);
        }
    }

    // Hmm, I need to return it as an File array
    File[] filteredSensorDataArray = (File[]) filteredSensorData.toArray(new File[filteredSensorData.size()]);
    return filteredSensorDataArray;
}
\$\endgroup\$
0
\$\begingroup\$

Reducing the size of your method and making not only it specialized as you should plan your classes is a great way to make your code more readable, so the next guy that has to make some change on it has a easier life.

Read Clean code if you wan't to get better at it, it´s a great book

\$\endgroup\$
  • \$\begingroup\$ + Clean Coder + Pragmatic Programmer \$\endgroup\$ – mheinzerling Jul 5 '13 at 5:16

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.