Repository: graphhopper/graphhopper
Pull request #1194 (2017-10-19 23:27:09)

Fixed Exponential Notation for GPX export
[Kurviger](https://kurviger.de/en) received a [bug report](https://groups.google.com/d/msg/kurviger/b4TdQQSzkT8/nTGrpmTwAQAJ) that there can be an issue with the GPX export if coordinates are close to 0. The default formatting of Java for values like `0.000123` is `1.23E-4` which makes sense for readability, but some programs like Garmin Basecamp have issues importing these numbers.

This PR fixes this issue. I tried to stay as close to the current behaviour of GH as possible, but removing the exponential notation. For coordinates like 15.0 it prints 15.0 but not just 15 or 15.000000. Also values like 0.1 are printed as 0.1. 

karussell: Hmmh, I would like to avoid this complexity just because other programs have bugs. E.g. according to [this SO](https://stackoverflow.com/questions/4387170/decimalformat-formatdouble-in-different-threads) the thread safety is not guaranteed.
karussell: What about String.format("%.0f", priceValue) or BigDecimal.toPlainString()?
boldtrn: > Hmmh, I would like to avoid this complexity just because other programs have bugs.

I did some more research about this. The GPX Schema [defines lats and longs](http://www.topografix.com/gpx/1/1/#type_latitudeType) to be xsd:decimal. [xsd:decimal does not allow scientific notation](http://books.xmlschemata.org/relaxng/ch19-77057.html). A lot of GPS programs accept the scientific notification, probably because most String to Number converters accept the scientific notation.

> What about String.format

The issue with String.format is that it is rather inflexible when it comes to different numbers of digits. We could build a formatting string per input or accept that all values have six digits after the decimal point, like: `1.000000`. I would not mind, but this would change the values that we currently create in the GPX and also the resulting GPX track might be a bit bigger. Alternatively, we could count the number of required digits, and create a formatting string per double, but this seems to be a bit of an overhead to me.

[Here is the Java doc](https://docs.oracle.com/javase/7/docs/api/java/util/Formatter.html#dndec):
> The number of digits in the result for the fractional part of m or a is equal to the precision. If the precision is not specified then the default value is 6. If the precision is less than the number of digits which would appear after the decimal point in the string returned by Float.toString(float) or Double.toString(double) respectively, then the value will be rounded using the round half up algorithm. Otherwise, zeros may be appended to reach the precision. For a canonical representation of the value, use Float.toString(float) or Double.toString(double) as appropriate.

```
BigDecimal bigDecimal = new BigDecimal(0.1);
System.out.println(bigDecimal.toPlainString());

Prints: 0.1000000000000000055511151231257827021181583404541015625
``` 

``` 
BigDecimal bigDecimal = new BigDecimal(0.1);
bigDecimal = bigDecimal.round(new MathContext(6));
System.out.println(bigDecimal.toPlainString());

Prints: 0.100000
```

> E.g. according to this SO the thread safety is not guaranteed.

How about, we create the number formatting object inside the print method? I wasn't aware of thread issues here. So we could change the code to:

```
    public static final String print6(double value) {
        DecimalFormat decimalFormat = new DecimalFormat("#");
        decimalFormat.setMinimumFractionDigits(1);
        decimalFormat.setMaximumFractionDigits(6);
        decimalFormat.setMinimumIntegerDigits(1);
        return decimalFormat.format(value);
    }
```
karussell: Ok, if the spec says this and it is indeed a but. Thanks for pointing this out.

> we create the number formatting object inside the print method? 

It probably would be better to create one formatter per request and reuse it `print6(DecimalFormat, double)`. I'm not sure about the perf implications if such a formatter is created per method call.
boldtrn: > It probably would be better to create one formatter per request and reuse it print6(DecimalFormat, double). I'm not sure about the perf implications if such a formatter is created per method call.

The constructor looks a bit complicated, so there might be a performance implication, when calling the constructor often. Since I cannot see a benefit of the function in the Helper class when only calling the formatter and only using it on file, I decided the easiest would be to only do this in the InstructionList.
boldtrn: How should we handle the code climate issues? Should we change the allowed line length? Should I break the lines?
karussell: Hmmh, this check style stuff is overwhelming and to be honest: not so useful - or what do you think? (we could deactivate the not so useful warnings for now)
karussell: BTW: looks good now :)

Patch:
diff --git a/core/src/main/java/com/graphhopper/util/Helper.java b/core/src/main/java/com/graphhopper/util/Helper.java
index d4c4da024..79fe42130 100644
--- a/core/src/main/java/com/graphhopper/util/Helper.java
+++ b/core/src/main/java/com/graphhopper/util/Helper.java
@@ -37,6 +37,7 @@ import java.security.AccessController;
 import java.security.PrivilegedActionException;
 import java.security.PrivilegedExceptionAction;
 import java.text.DateFormat;
+import java.text.DecimalFormat;
 import java.text.NumberFormat;
 import java.text.SimpleDateFormat;
 import java.util.*;
diff --git a/core/src/main/java/com/graphhopper/util/InstructionList.java b/core/src/main/java/com/graphhopper/util/InstructionList.java
index 331aea5a0..657914550 100644
--- a/core/src/main/java/com/graphhopper/util/InstructionList.java
+++ b/core/src/main/java/com/graphhopper/util/InstructionList.java
@@ -20,6 +20,7 @@ package com.graphhopper.util;
 import com.fasterxml.jackson.annotation.JsonValue;
 
 import java.text.DateFormat;
+import java.text.DecimalFormat;
 import java.util.*;
 
 /**
@@ -159,10 +160,10 @@ public class InstructionList extends AbstractList<Instruction> {
         return createGPX(trackName, startTimeMillis, includeElevation, true, true, true);
     }
 
-    private void createWayPointBlock(StringBuilder output, Instruction instruction) {
+    private void createWayPointBlock(StringBuilder output, Instruction instruction, DecimalFormat decimalFormat) {
         output.append("\n<wpt ");
-        output.append("lat=\"").append(Helper.round6(instruction.getFirstLat()));
-        output.append("\" lon=\"").append(Helper.round6(instruction.getFirstLon())).append("\">");
+        output.append("lat=\"").append(decimalFormat.format(instruction.getFirstLat()));
+        output.append("\" lon=\"").append(decimalFormat.format(instruction.getFirstLon())).append("\">");
         String name;
         if (instruction.getName().isEmpty())
             name = instruction.getTurnDescription(tr);
@@ -176,6 +177,11 @@ public class InstructionList extends AbstractList<Instruction> {
     public String createGPX(String trackName, long startTimeMillis, boolean includeElevation, boolean withRoute, boolean withTrack, boolean withWayPoints) {
         DateFormat formatter = Helper.createFormatter();
 
+        DecimalFormat decimalFormat = new DecimalFormat("#");
+        decimalFormat.setMinimumFractionDigits(1);
+        decimalFormat.setMaximumFractionDigits(6);
+        decimalFormat.setMinimumIntegerDigits(1);
+
         String header = "<?xml version=\"1.0\" encoding=\"UTF-8\" standalone=\"no\" ?>"
                 + "<gpx xmlns=\"http://www.topografix.com/GPX/1/1\" xmlns:xsi=\"http://www.w3.org/2001/XMLSchema-instance\""
                 + " creator=\"Graphhopper version " + Constants.VERSION + "\" version=\"1.1\""
@@ -192,12 +198,12 @@ public class InstructionList extends AbstractList<Instruction> {
         StringBuilder gpxOutput = new StringBuilder(header);
         if (!isEmpty()) {
             if (withWayPoints) {
-                createWayPointBlock(gpxOutput, instructions.get(0));   // Start 
+                createWayPointBlock(gpxOutput, instructions.get(0), decimalFormat);   // Start
                 for (Instruction currInstr : instructions) {
                     if ((currInstr.getSign() == Instruction.REACHED_VIA) // Via
                             || (currInstr.getSign() == Instruction.FINISH)) // End
                     {
-                        createWayPointBlock(gpxOutput, currInstr);
+                        createWayPointBlock(gpxOutput, currInstr, decimalFormat);
                     }
                 }
             }
@@ -206,11 +212,11 @@ public class InstructionList extends AbstractList<Instruction> {
                 Instruction nextInstr = null;
                 for (Instruction currInstr : instructions) {
                     if (null != nextInstr)
-                        createRteptBlock(gpxOutput, nextInstr, currInstr);
+                        createRteptBlock(gpxOutput, nextInstr, currInstr, decimalFormat);
 
                     nextInstr = currInstr;
                 }
-                createRteptBlock(gpxOutput, nextInstr, null);
+                createRteptBlock(gpxOutput, nextInstr, null, decimalFormat);
                 gpxOutput.append("\n</rte>");
             }
         }
@@ -219,8 +225,8 @@ public class InstructionList extends AbstractList<Instruction> {
 
             gpxOutput.append("<trkseg>");
             for (GPXEntry entry : createGPXList()) {
-                gpxOutput.append("\n<trkpt lat=\"").append(Helper.round6(entry.getLat()));
-                gpxOutput.append("\" lon=\"").append(Helper.round6(entry.getLon())).append("\">");
+                gpxOutput.append("\n<trkpt lat=\"").append(decimalFormat.format(entry.getLat()));
+                gpxOutput.append("\" lon=\"").append(decimalFormat.format(entry.getLon())).append("\">");
                 if (includeElevation)
                     gpxOutput.append("<ele>").append(Helper.round2(entry.getEle())).append("</ele>");
                 gpxOutput.append("<time>").append(formatter.format(startTimeMillis + entry.getTime())).append("</time>");
@@ -235,9 +241,9 @@ public class InstructionList extends AbstractList<Instruction> {
         return gpxOutput.toString();
     }
 
-    public void createRteptBlock(StringBuilder output, Instruction instruction, Instruction nextI) {
-        output.append("\n<rtept lat=\"").append(Helper.round6(instruction.getFirstLat())).
-                append("\" lon=\"").append(Helper.round6(instruction.getFirstLon())).append("\">");
+    public void createRteptBlock(StringBuilder output, Instruction instruction, Instruction nextI, DecimalFormat decimalFormat) {
+        output.append("\n<rtept lat=\"").append(decimalFormat.format(instruction.getFirstLat())).
+                append("\" lon=\"").append(decimalFormat.format(instruction.getFirstLon())).append("\">");
 
         if (!instruction.getName().isEmpty())
             output.append("<desc>").append(simpleXMLEscape(instruction.getTurnDescription(tr))).append("</desc>");


Test patch:
diff --git a/core/src/test/java/com/graphhopper/util/InstructionListTest.java b/core/src/test/java/com/graphhopper/util/InstructionListTest.java
index 06d7aba54..244d3c5cf 100644
--- a/core/src/test/java/com/graphhopper/util/InstructionListTest.java
+++ b/core/src/test/java/com/graphhopper/util/InstructionListTest.java
@@ -409,6 +409,25 @@ public class InstructionListTest {
         verifyGPX(gpxStr);
     }
 
+    @Test
+    public void testCreateGPXCorrectFormattingSmallNumbers() {
+        InstructionList instructions = new InstructionList(usTR);
+
+        PointList pl = new PointList();
+        pl.add(0.000001, 0.000001);
+        pl.add(-0.000123, -0.000125);
+        Instruction instruction = new Instruction(0, "do it", null, pl);
+        instructions.add(instruction);
+        instructions.add(new FinishInstruction(0.000852, 0.000852, 0));
+
+        String gpxStr = instructions.createGPX("test", 0, true, true, true, true);
+
+        assertFalse(gpxStr, gpxStr.contains("E-"));
+        assertTrue(gpxStr, gpxStr.contains("0.000001"));
+        assertTrue(gpxStr, gpxStr.contains("-0.000125"));
+        verifyGPX(gpxStr);
+    }
+
     @Test
     public void testCreateGPXWithEle() {
         final List<GPXEntry> fakeList = new ArrayList<GPXEntry>();