Code review Request for Issue 20: SAX-style importer

[GoogleCodeExporter]

Mercurial clone with changes to be reviewed:
http://code.google.com/r/steffenhorlacher-stracks/

Revisions to be pulled from that clone (or "all"):
a6708a2a35  

Purpose of code changes on the clone:
This is for Issue 20: SAX style GPX-Importer. I need to import large GPX files 
and as this is not 
possible with the current importer I created a SAX based on. Tested with files 
larger than 2MB. 
The bug fix for issue 57 is also included.

When reviewing my code changes, please focus on:
Please verify the importer with different GPX file sources/formats as I don't 
have much test data.

Steffen

Original issue reported on code.google.com by steffen.horlacher@gmail.com on 23 May 2010 at 9:56

[GoogleCodeExporter]

did some minor corrections, you may use revision dc166ddde3

Original comment by steffen.horlacher@gmail.com on 23 May 2010 at 11:03

[GoogleCodeExporter]

Cool, our first contribution :) Do you mind enabling us to review code on your 
clone,
by going into Administer, Source, then "Allow non-members to review code"? (my 
fault,
I forgot to mention that in the wiki)

Original comment by rdama...@google.com on 23 May 2010 at 7:55

• Changed state: Accepted
• Added labels: Component-Persistence, OpSys-Android, Performance

[GoogleCodeExporter]

OK, I did change the settings...

Original comment by steffen.horlacher@gmail.com on 23 May 2010 at 11:19

[GoogleCodeExporter]

Reviewing

Original comment by rdama...@google.com on 28 May 2010 at 5:51

• Changed state: Started

[GoogleCodeExporter]

Thanks for taking the time to review, I wished my code was always reviewed that 
carefully! I did change most 
of the things you commented on and made a new clone from latest mytracks code:

http://code.google.com/r/steffenhorlacher-mytracks/

Let me reply to some of the comments:

-----------------
64:     private static final String TAG_TRACK_POINT = "trkpt";
How come you're not dealing with trkseg at all?

I was not sure how to handle trkseg, I should have mentioned that in the review 
request, sorry. The old 
importer did this:
if (nSegments > 0) {
  // Add a segment separator:
  Location location = new Location("gps");
  location.setLatitude(100.0);
  location.setLongitude(100.0);
  location.setAltitude(0);
  if (locations.size() > 0) {
    long pointTime = locations.get(locations.size() - 1).getTime();
    location.setTime(pointTime);
  }
  track.addLocation(location);
  lastLocation = null;
}
nSegments++;

I added the same behavior to the new importer now. But it looks not really 
nice. Can the segment handling 
not be done separately (without using locations)? This makes the handling of 
tracks and locations a bit 
complicated as segments are written in the db as location but not used for the 
stats. Maybe the complete 
segment handling in mytracks could be refactored in some way?

-----------------
EasyMock has some known problems with Android, specially when you need to use 
class extension. Any 
chance you could use AndroidMock instead? It's based on EasyMock so it 
shouldn't be too hard to change.

I did change it to AndroidMock but reverted later to EasyMock again. As some of 
the existing test cases are 
based on EasyMock, I would have had to rewrite those test cases also but didn't 
want to mess up your code. I 
also was not sure if it's OK to mix the AndroidMock with some of the EasyMock 
stuff, for example 
createStrictControl and Capture are not part of the AndroidMock package but 
work in combination with 
EasyMock. Feel free to change everything to AndroidMock, as you already 
mentioned it's not that big of a 
change, I just was not sure if I did it the right way.

-----------------
106: providerUtils.insertTrackPoint(capture(locParam),capture(idParam)))
I think Location also has equals() implemented, so you can just pass in your 
expected location instead of 
needing captures + asserts.
110: providerUtils.updateTrack(capture(trackParam));
You're capturing trackParam twice. Ideally, avoid capture at all - add equals() 
to Track so you can use the 
default comparison (which is eq()). I'll be happy to add that if you'd like.

Actually I tried equals in the first place, but it looks like Location has 
equals not implemented. Still using 
capture. Also still using capture for Track, as equals makes no sense there as 
long as Location has no equals, 
I think, right? 

-----------------
About the coding style:
I hope the format matches now, I'm not sure :) The wiki suggests to use the 
mytracks-style.xml, but I couldn't 
find it... Is it really in the source tree?

-----------------
In addition to your comments I added time zone handling (this is also a bug in 
the old importer), a bug fix for 
multiple tracks in one file and a check for negative time changes.

Original comment by steffen.horlacher@gmail.com on 5 Jun 2010 at 12:57

[GoogleCodeExporter]

Steffen, just thought I'd apologize for not getting back on this before - we're 
working on something a big (mytracks-related) project that's taking most of our 
time. We'll be back to normal soon :)

Original comment by rdama...@google.com on 22 Jun 2010 at 11:35

[GoogleCodeExporter]

Thanks, but there is no need for you to apologize, I'm busy with "real" work 
stuff too....  and good look for the big project! I'm very curious :)

Original comment by steffen.horlacher@gmail.com on 23 Jun 2010 at 6:56

[GoogleCodeExporter]

Since you're curious:
http://www.highroadsports.com/news/612-High-Road-Sports-and-Google-Announce-New-
Marketing-Agreement

Original comment by rdama...@google.com on 1 Jul 2010 at 8:54

[GoogleCodeExporter]

I vote for this since it seems to be blocking 45, which is the one I really 
wanna see implemented... 

Original comment by christopher.wanko on 5 Aug 2010 at 9:22

[GoogleCodeExporter]

Finally getting back to this after Tour de France + vacations :) Sorry again 
for the long delay.

Original comment by rdama...@google.com on 12 Aug 2010 at 8:20

[GoogleCodeExporter]

Review done. Now on to pulling, testing, and pushing. :) Thanks!

Original comment by rdama...@google.com on 12 Aug 2010 at 8:30

• Changed state: Done