Review comments for revision fd70af575b77 addressed.

MyTracks/src/com/google/android/apps/mytracks/services/sensors/StrideReadings.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2010 Google Inc.
+ * Copyright 2011 Google Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License"); you may not
  * use this file except in compliance with the License. You may obtain a copy of
@@ -22,19 +22,21 @@ import java.util.List;
  * A history of Zephyr stride counter reading.
  * These can be used as an alternate method to calculate the correct cadence.
  * This is a work around for an issue with some HxM firmware.
+ *
  * @author Dominik Ršttsches
  */
 public class StrideReadings {
 
-  private static final int NUM_READINGS_FOR_AVERAGE = 10;
-  private static final int MIN_READINGS_FOR_AVERAGE = 5;
+  // visible for testing
+  protected static final int NUM_READINGS_FOR_AVERAGE = 10;
+  protected static final int MIN_READINGS_FOR_AVERAGE = 5;
 
   protected static final int CADENCE_NOT_AVAILABLE = -1;
 
   // TODO: Check whether 1Hz assumption is okay for cadence calculation
   // otherwise add heart beat timestamp to this list and compute
   // cadence from these timestamps.
-  private List<Integer> strideReadingsHistory;
+  private final List<Integer> strideReadingsHistory;
 
   public StrideReadings() {
     strideReadingsHistory = new LinkedList<Integer>();
@@ -65,9 +67,14 @@ public class StrideReadings {
         timeSinceOldestReadingSecs * 60);
   }
 
-  private int mod(int x, int y)
+  /**
+   * Modulo operation with positive return values, Java's remainder operator doesn't change sign.
+   *
+   * @return x mod y
+   */
+  private static int mod(int x, int y)
   {
       int result = x % y;
-      return result < 0? result + y : result;
+      return result < 0 ? result + y : result;
   }
 }

MyTracks/src/com/google/android/apps/mytracks/services/sensors/ZephyrMessageParser.java

@@ -62,22 +62,20 @@ public class ZephyrMessageParser implements MessageParser {
     // a workaround based on the stride counter.
     // Firmware values range from field 3 to 10 (inclusive) of the byte buffer.
     byte[] hardwareFirmwareId = Arrays.copyOfRange(buffer, 3, 11);
-    boolean computeFromStrides = Arrays.equals(hardwareFirmwareId, CADENCE_BUG_FW_ID);
 
     Sensor.SensorData.Builder cadence = Sensor.SensorData.newBuilder();
 
-    if(computeFromStrides) {
-      if(strideReadings == null) {
+    if (Arrays.equals(hardwareFirmwareId, CADENCE_BUG_FW_ID)) {
+      if (strideReadings == null) {
         strideReadings = new StrideReadings();
       }
       strideReadings.updateStrideReading(buffer[54] & 0xFF);
       
-      if(strideReadings.getCadence() != StrideReadings.CADENCE_NOT_AVAILABLE) {
-        cadence = cadence.setValue(strideReadings.getCadence())
-          .setState(Sensor.SensorState.SENDING);
+      if (strideReadings.getCadence() != StrideReadings.CADENCE_NOT_AVAILABLE) {
+        cadence.setValue(strideReadings.getCadence()).setState(Sensor.SensorState.SENDING);
       }
     } else {
-      cadence = cadence
+      cadence
       .setValue(SensorUtils.unsignedShortToIntLittleEndian(buffer, 56) / 16)
       .setState(Sensor.SensorState.SENDING);
     }

MyTracksTest/src/com/google/android/apps/mytracks/services/sensors/StrideReadingsTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2010 Google Inc.
+ * Copyright 2011 Google Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License"); you may not
  * use this file except in compliance with the License. You may obtain a copy of
@@ -15,41 +15,13 @@
  */
 package com.google.android.apps.mytracks.services.sensors;
 
-import java.lang.reflect.Field;
-
-import junit.framework.Assert;
 import junit.framework.TestCase;
 
-
 /**
  * @author Dominik Ršttsches
  */
 public class StrideReadingsTest extends TestCase {
   
-  /**
-   * Provides access to private members in classes.
-   * from http://onjava.com/pub/a/onjava/2003/11/12/reflection.html
-   */
-  private Object getPrivateField (Object o, String fieldName) {
-    /* Check we have valid arguments */
-    Assert.assertNotNull(o);
-    Assert.assertNotNull(fieldName);
-    /* Go and find the private field... */
-    final Field fields[] = o.getClass().getDeclaredFields();
-    for (int i = 0; i < fields.length; ++i) {
-      if (fieldName.equals(fields[i].getName())) {
-        try {
-          fields[i].setAccessible(true);
-          return fields[i].get(o);
-        } catch (IllegalAccessException ex) {
-          Assert.fail ("IllegalAccessException accessing " + fieldName);
-        }
-      }
-    }
-    Assert.fail ("Field '" + fieldName + "' not found");
-    return null;
-  }
-
   public void testNoReadingOnStartup() {
     StrideReadings strideReadings = new StrideReadings();
     assertEquals(StrideReadings.CADENCE_NOT_AVAILABLE, strideReadings.getCadence());
@@ -58,28 +30,33 @@ public class StrideReadingsTest extends TestCase {
   public void testAverageCadenceAvailable() {
     StrideReadings strideReadings = new StrideReadings();
     // 2 steps / second => Cadence is 120 / minute
-    for(int i=1;i<=30;i++) {
+    for (int i=1;i<=30;i++) {
       strideReadings.updateStrideReading(i*2);
-      if(i > (Integer)getPrivateField(strideReadings, "NUM_READINGS_FOR_AVERAGE")) {
+      if (i >= StrideReadings.MIN_READINGS_FOR_AVERAGE) {
         assertEquals(120, strideReadings.getCadence());
+      } else {
+        assertEquals(StrideReadings.CADENCE_NOT_AVAILABLE, strideReadings.getCadence());
       }
     }
   }
   
-  /** Tests for correct calculation after rolling over at 128 strides,
-   * just like the HxM seems to do it. */
+  /**
+   * Tests for correct calculation after rolling over at 128 strides,
+   * just like the HxM seems to do it.
+   */
   public void testRollover() {
     StrideReadings strideReadings = new StrideReadings();
     // 1 step per second => Cadence is 60 / minute
     // Updating readings counting upwards from initialStrides -
     // initialStrides set to a value below 128 to ensure rollover.
-    int numReadingsRequired = (Integer)getPrivateField(strideReadings, "NUM_READINGS_FOR_AVERAGE");
-    int initialStrides = 128 - numReadingsRequired  - 5;
+    int initialStrides = 128 - StrideReadings.NUM_READINGS_FOR_AVERAGE - 5;
 
-    for(int i=1;i<=numReadingsRequired+10;i++) {
+    for (int i=1;i<=StrideReadings.NUM_READINGS_FOR_AVERAGE+10;i++) {
       strideReadings.updateStrideReading((initialStrides + i) % 128);
-      if(i > numReadingsRequired) {
+      if (i >= StrideReadings.MIN_READINGS_FOR_AVERAGE) {
         assertEquals(60, strideReadings.getCadence());
+      } else {
+        assertEquals(StrideReadings.CADENCE_NOT_AVAILABLE, strideReadings.getCadence());
       }
     }
   }