[PATCH] Ticket #423 - Import GPX file from SD card

[Aurélien PRALONG]

Hello Venkatesh,

> I am waiting for Aurélien to review this and my earlier patch
> involving addition of mappicker.
A bit late, but here it is.

This looks promising. But I found some flaws.

Code comments
-----------------------
- Do not use normal comments ("//") on top of methods. Prefer Javadoc style
("/** ... */")
- Make attributes private in classes, especially if you don't use them in
other classes. It helps auto-complete.
- SimpleDateFormat are not thread-safe. Do not make them final. Instanciate
them each time it is necessary
- I guess it is due to your editor, but why do you make "return" for each
setter in GpxFileInfo ? They are not necessary.
- GpxParser should not be in org.subsurface package, as it is not an
activity
- Do not use [Exception].printStackTrace. Never. Use Log.X("Error", e)
instead.
- Do not use "magic" variables, define constants (found for
bundle.get("...") in PickLocationMap and PickGpx)
- Keep constants and attributes on the top of the class (cf. PickGpx)

For code style, I refer to normal java guidelines, which are quite readable
:
http://www.oracle.com/technetwork/java/javase/documentation/codeconvtoc-136057.html.
Maybe should I write a checkstyle template for the formatting stuff.

Usability comments
----------------------------
The GPX importer works well (tested with a file on
http://www.topografix.com/gpx_sample_files.asp). But from what I see, "sym"
should be used in place of name when it exists, as it has a more explicit
text. Other enhancement, could you add a "check / uncheck all" menu ? It is
easier for big files.

The map picker works well too, but has a problem : you do not see where you
touched the screen. A mark should be added on map (long) click, so before
confirming, you are sure you chose this position.
Other problem : I did not understand what meant "Minutes past after dive",
until I looked in the code. I think it would be better to use a date/time
selector for the dive time instead.

This mail is long, but with minor problems, so do not worry : the result is
still good. Please tell me if you have questions.

Aurélien
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.hohndel.org/pipermail/subsurface/attachments/20140309/a48de8a3/attachment.html>