Optimizing the importer a little

MyTracks/src/com/google/android/apps/mytracks/io/GpxImporter.java

@@ -44,8 +44,11 @@ import org.xml.sax.helpers.DefaultHandler;
 /**
  * Imports GPX XML files to the my tracks provider.
  *
+ * TODO: Show progress indication to the user.
+ *
  * @author Leif Hendrik Wilden
  * @author Steffen Horlacher
+ * @author Rodrigo Damazio
  */
 public class GpxImporter extends DefaultHandler {
 
@@ -65,6 +68,7 @@ public class GpxImporter extends DefaultHandler {
    */
   private static final String TAG_TRACK = "trk";
   private static final String TAG_TRACK_POINT = "trkpt";
+  private static final Object TAG_TRACK_SEGMENT = "trkseg";
   private static final String TAG_NAME = "name";
   private static final String TAG_DESCRIPTION = "desc";
   private static final String TAG_ALTITUDE = "ele";
@@ -83,7 +87,7 @@ public class GpxImporter extends DefaultHandler {
   /**
    * Contains the current elements content.
    */
-  private StringBuilder content;
+  private String content;
 
   /**
    * Currently reading location.
@@ -95,6 +99,12 @@ public class GpxImporter extends DefaultHandler {
    */
   private Location lastLocation;
 
+  /**
+   * URI of the last point inserted into the database.
+   * We parse point IDs out of this when necessary.
+   */
+  private Uri lastPointIdUri;
+  
   /**
    * Currently reading track.
    */
@@ -117,8 +127,9 @@ public class GpxImporter extends DefaultHandler {
   private boolean isCurrentTrackRollbackable;
 
   /**
-   * Flag to indicate if we in a track xml element some sub elements like name
+   * Flag to indicate if we're inside a track's xml element.
-   * may be used in other parts of the gpx file - ignore them.
+   * Some sub elements like name may be used in other parts of the gpx file,
+   * and we use this to ignore them.
    */
   private boolean isInTrackElement;
 
@@ -154,6 +165,8 @@ public class GpxImporter extends DefaultHandler {
     try {
       parser.parse(is, handler);
       trackIds = handler.getImportedTrackIds();
+    } catch (SAXException e) {
+      throw e;
     } finally {
       // delete track if not finished
       handler.rollbackUnfinishedTracks();
@@ -168,72 +181,74 @@ public class GpxImporter extends DefaultHandler {
   public GpxImporter(MyTracksProviderUtils providerUtils) {
     this.providerUtils = providerUtils;
     tracksWritten = new ArrayList<Long>();
-    content = new StringBuilder();
   }
 
   @Override
   public void characters(char[] ch, int start, int length) throws SAXException {
-    content.append(ch, start, length);
+    String newContent = new String(ch, start, length);
+    if (content == null) {
+      content = newContent;
+    } else {
+      // In 99% of the cases, a single call to this method will be made for each
+      // sequence of characters we're interested in, so we'll rarely be
+      // concatenating strings, thus not justifying the use of a StringBuilder.
+      content += newContent;
+    }
   }
 
   @Override
   public void startElement(String uri, String localName, String name,
       Attributes attributes) throws SAXException {
-    // reset element content
+    if (isInTrackElement) {
-    content.setLength(0);
+      trackChildDepth++;
-
+      if (localName.equals(TAG_TRACK_POINT)) {
-    if (localName.equalsIgnoreCase(TAG_TRACK)) {
+        onTrackPointElementStart(attributes);
-      // test if we are already in a track element - abort in this case
+      } else if (localName.equals(TAG_TRACK_SEGMENT)) {
-      if (isInTrackElement) {
+        onTrackSegmentElementStart();
+      } else if (localName.equals(TAG_TRACK)) {
         String msg = createErrorMessage("Invalid GPX-XML detected");
         throw new SAXException(msg);
       }
-
+    } else if (localName.equals(TAG_TRACK)) {
       isInTrackElement = true;
       trackChildDepth = 0;
       onTrackElementStart();
-
-      // process this element only as sub-elements of track
-    } else if (isInTrackElement) {
-      trackChildDepth++;
-      if (localName.equalsIgnoreCase(TAG_TRACK_POINT)) {
-        onTrackPointElementStart(attributes);
-      }
     }
   }
 
   @Override
   public void endElement(String uri, String localName, String name)
       throws SAXException {
-    if (localName.equalsIgnoreCase(TAG_TRACK)) {
+    if (!isInTrackElement) { return; }
+
+    // process these elements only as sub-elements of track
+    if (localName.equals(TAG_TRACK_POINT)) {
+      onTrackPointElementEnd();
+    } else if (localName.equals(TAG_ALTITUDE)) {
+      onAltitudeElementEnd();
+    } else if (localName.equals(TAG_TIME)) {
+      onTimeElementEnd();
+    } else if (localName.equals(TAG_NAME)) {
+      // we are only interested in the first level name element
+      if (trackChildDepth == 1) {
+        onNameElementEnd();
+      }
+    } else if (localName.equals(TAG_DESCRIPTION)) {
+      // we are only interested in the first level description element
+      if (trackChildDepth == 1) {
+        onDescriptionElementEnd();
+      }
+    } else if (localName.equals(TAG_TRACK_SEGMENT)) {
+      onTrackSegmentElementEnd();
+    } else if (localName.equals(TAG_TRACK)) {
       onTrackElementEnd();
       isInTrackElement = false;
       trackChildDepth = 0;
-
-      // process these elements only as sub-elements of track
-    } else if (isInTrackElement) {
-      if (localName.equalsIgnoreCase(TAG_TRACK_POINT)) {
-        onTrackPointElementEnd();
-      } else if (localName.equalsIgnoreCase(TAG_ALTITUDE)) {
-        onAltitudeElementEnd();
-      } else if (localName.equalsIgnoreCase(TAG_TIME)) {
-        onTimeElementEnd();
-      } else if (localName.equalsIgnoreCase(TAG_NAME)) {
-        // we are only interested in the first level name element
-        if (trackChildDepth == 1) {
-          onNameElementEnd();
-        }
-      } else if (localName.equalsIgnoreCase(TAG_DESCRIPTION)) {
-        // we are only interested in the first level description element
-        if (trackChildDepth == 1) {
-          onDescriptionElementEnd();
-        }
-      }
-      trackChildDepth--;
     }
+    trackChildDepth--;
 
     // reset element content
-    content.setLength(0);
+    content = null;
   }
 
   @Override
@@ -255,25 +270,50 @@ public class GpxImporter extends DefaultHandler {
     isCurrentTrackRollbackable = true;
   }
 
+  private void onDescriptionElementEnd() {
+    track.setDescription(content.toString().trim());
+  }
+
+  private void onNameElementEnd() {
+    track.setName(content.toString().trim());
+  }
+
+  /**
+   * Track segment started.
+   */
+  private void onTrackSegmentElementStart() {
+    // TODO Auto-generated method stub
+    
+  }
+
   /**
    * Reads trackpoint attributes and assigns them to the current location.
    *
    * @param attributes xml attributes
    */
-  private void onTrackPointElementStart(Attributes attributes) {
+  private void onTrackPointElementStart(Attributes attributes) throws SAXException {
+    if (location != null) {
+      String errorMsg = createErrorMessage("Found a track point inside another one.");
+      throw new SAXException(errorMsg);
+    }
+
     location = createLocationFromAttributes(attributes);
   }
 
-  private Location createLocationFromAttributes(Attributes attributes) {
+  /**
-    String latitude = null;
+   * Creates and returns a location with the position parsed from the given
-    String longitude = null;
+   * attributes.
+   *
+   * @param attributes the attributes to parse
+   * @return the created location
+   * @throws SAXException if the attributes cannot be parsed
+   */
+  private Location createLocationFromAttributes(Attributes attributes) throws SAXException {
+    String latitude = attributes.getValue(ATT_LAT);
+    String longitude = attributes.getValue(ATT_LON);
 
-    for (int i = 0; i < attributes.getLength(); i++) {
+    if (latitude == null || longitude == null) {
-      if (attributes.getLocalName(i).equals(ATT_LAT)) {
+      throw new SAXException(createErrorMessage("Point with no longitude or latitude"));
-        latitude = attributes.getValue(i);
-      } else if (attributes.getLocalName(i).equals(ATT_LON)) {
-        longitude = attributes.getValue(i);
-      }
     }
 
     // create new location and set attributes
@@ -283,14 +323,6 @@ public class GpxImporter extends DefaultHandler {
     return loc;
   }
 
-  private void onDescriptionElementEnd() {
-    track.setDescription(content.toString().trim());
-  }
-
-  private void onNameElementEnd() {
-    track.setName(content.toString().trim());
-  }
-
   /**
    * Track point finished, write in database.
    * 
@@ -301,22 +333,15 @@ public class GpxImporter extends DefaultHandler {
       statsBuilder.addLocation(location, location.getTime());
 
       // insert in db
-      Uri trackPointIdUri = providerUtils.insertTrackPoint(location,
+      lastPointIdUri = providerUtils.insertTrackPoint(location, track.getId());
-          track.getId());
-
-      // set start and stop id for track
-      long trackPointId = Long.parseLong(trackPointIdUri.getLastPathSegment());
 
       // first track point?
       if (lastLocation == null) {
-        track.setStartId(trackPointId);
+        track.setStartId(getLastPointId());
       }
 
-      // location has no setId method
-      // updating stop id on track every time...
-      track.setStopId(trackPointId);
-
       lastLocation = location;
+      location = null;
       numberOfLocations++;
     } else {
       // invalid location - abort import
@@ -325,6 +350,14 @@ public class GpxImporter extends DefaultHandler {
     }
   }
   
+  /**
+   * Track segment finished.
+   */
+  private void onTrackSegmentElementEnd() {
+    // TODO Auto-generated method stub
+    
+  }
+
   /**
    * Track finished - update in database.
    */
@@ -332,6 +365,7 @@ public class GpxImporter extends DefaultHandler {
     if (lastLocation != null) {
       // Calculate statistics for the imported track and update
       statsBuilder.pauseAt(lastLocation.getTime());
+      track.setStopId(getLastPointId());
       track.setNumberOfPoints(numberOfLocations);
       track.setStatistics(statsBuilder.getStatistics());
       providerUtils.updateTrack(track);
@@ -340,7 +374,7 @@ public class GpxImporter extends DefaultHandler {
       lastLocation = null;
       statsBuilder = null;
     } else {
-      // track contains no track points makes not really
+      // track contains no track points makes no real
       // sense to import it as we have no location
       // information -> roll back
       rollbackUnfinishedTracks();
@@ -355,23 +389,16 @@ public class GpxImporter extends DefaultHandler {
    * @throws SAXException on parsing errors
    */
   private void onTimeElementEnd() throws SAXException {
-    long time = parseTimeForAllFormats(content.toString().trim());
+    if (location == null) { return; }
+
+    long time = parseTimeForAllFormats(content);
+    if (lastLocation != null) {
+      long timeDifference = time - lastLocation.getTime();
 
-    if (location != null) {
       // check for negative time change
-      if (lastLocation != null) {
+      if (timeDifference < 0) {
-        long timeDifference = time - lastLocation.getTime();
+        String msg = createErrorMessage("Found negative time change.");
-        if (timeDifference < 0) {
+        throw new SAXException(msg);
-          String msg = createErrorMessage("Found negative time change.");
-          throw new SAXException(msg);
-        }
-      }
-
-      location.setTime(time);
-      // initialize start time with time of first track point
-      if (statsBuilder == null) {
-        statsBuilder = new TripStatisticsBuilder();
-        statsBuilder.resumeAt(time);
       }
 
       // We don't have a speed and bearing in GPX, make something up from
@@ -379,27 +406,27 @@ public class GpxImporter extends DefaultHandler {
       // TODO GPS points tend to have some inherent imprecision,
       // speed and bearing will likely be off, so the statistics for things like
       // max speed will also be off.
-      if (lastLocation != null) {
+      float speed = location.distanceTo(lastLocation) * 1000.0f / timeDifference;
-        final long dt = location.getTime() - lastLocation.getTime();
+      location.setSpeed(speed);
-        if (dt > 0) {
+      location.setBearing(lastLocation.bearingTo(location));
-          final float speed = location.distanceTo(lastLocation) / (dt / 1000);
+    }
-          location.setSpeed(speed);
+
-        }
+    location.setTime(time);
-        location.setBearing(lastLocation.bearingTo(location));
+    // initialize start time with time of first track point
-      }
+    if (statsBuilder == null) {
+      statsBuilder = new TripStatisticsBuilder();
+      statsBuilder.resumeAt(time);
     }
   }
 
   private void onAltitudeElementEnd() {
     if (location != null) {
-      String altitude = content.toString().trim();
+      location.setAltitude(Double.parseDouble(content));
-      location.setAltitude(Double.parseDouble(altitude));
     }
   }
 
   /**
-   * If a exception is thrown during the import callers must execute this method
+   * Deletes the last track if it was not completely imported.
-   * in the catch (also finally is ok) clause to avoid inconsistent data.
    */
   public void rollbackUnfinishedTracks() {
     if (isCurrentTrackRollbackable) {
@@ -430,7 +457,8 @@ public class GpxImporter extends DefaultHandler {
    * @throws SAXException on time parsing errors
    */
   private long parseTimeForAllFormats(String timeContents) throws SAXException {
-    long time = -1;
+    long time;
+    timeContents = timeContents.trim();
 
     // 1st try with time zone at end a la "+0000"
     time = parseTime(timeContents, DATE_FORMAT1);
@@ -457,9 +485,15 @@ public class GpxImporter extends DefaultHandler {
       format.setTimeZone(UTC_TIMEZONE);
       return format.parse(timeContents).getTime();
     } catch (ParseException ex) {
-      // do nothing
+      return -1;
     }
-    return -1;
+  }
+
+  /**
+   * Returns the ID of the last point inserted into the database.
+   */
+  private long getLastPointId() {
+    return Long.parseLong(lastPointIdUri.getLastPathSegment());
   }
 
   /**

MyTracksTest/src/com/google/android/apps/mytracks/io/GpxImporterTest.java

@@ -111,8 +111,8 @@ public class GpxImporterTest extends AndroidTestCase {
    */
   public void testImportSuccess() throws Exception {
     Capture<Track> trackParam = new Capture<Track>();
-    Capture<Location> locParam1 = new MyLocationCaptue();
+    Capture<Location> locParam1 = new MyLocationCapture();
-    Capture<Location> locParam2 = new MyLocationCaptue();
+    Capture<Location> locParam2 = new MyLocationCapture();
 
     AndroidMock.expect(
         providerUtils.insertTrack(AndroidMock.capture(trackParam))).andReturn(
@@ -214,7 +214,7 @@ public class GpxImporterTest extends AndroidTestCase {
    * /tracker/?func=detail&aid=2617107&group_id=82958&atid=567837
    */
   @SuppressWarnings("serial")
-  class MyLocationCaptue extends Capture<Location> {
+  class MyLocationCapture extends Capture<Location> {
     @Override
     public void setValue(Location value) {
       if (!hasCaptured()) {