Repository: graphhopper/graphhopper
Pull request #1804 (2019-11-22 11:32:21)

Avoid throwing NumberFormatException in the OSMValueExtractor
I've modified OSMValueExtractor to return `Double.NaN` for invalid values instead of throwing exceptions. Having a control flow which doesn't rely on unchecked exceptions is one point and the overhead of creating the exceptions is the other one:

![Screenshot_20191122_104055](https://user-images.githubusercontent.com/22315436/69421960-b9793900-0d22-11ea-9ecd-5284d8b25c93.png)

It's on par with the memory allocation of the pbf parsing logic only because it's gathering the stacktraces which we don't even use later on.

Note that `Double.parseDouble()` might still throw a `NumberFormatException` for some cases but i'm not sure if it's worth to add an extra test method before invoking it.

karussell: Ah, yeah. This is ugly if used as control flow.

Patch:
diff --git a/core/src/main/java/com/graphhopper/routing/util/parsers/OSMMaxAxleLoadParser.java b/core/src/main/java/com/graphhopper/routing/util/parsers/OSMMaxAxleLoadParser.java
index eddc4de49..3abfd6914 100644
--- a/core/src/main/java/com/graphhopper/routing/util/parsers/OSMMaxAxleLoadParser.java
+++ b/core/src/main/java/com/graphhopper/routing/util/parsers/OSMMaxAxleLoadParser.java
@@ -32,15 +32,13 @@ import java.util.List;
 public class OSMMaxAxleLoadParser implements TagParser {
 
     private final DecimalEncodedValue maxAxleLoadEncoder;
-    private final boolean enableLog;
 
     public OSMMaxAxleLoadParser() {
-        this(MaxAxleLoad.create(), false);
+        this(MaxAxleLoad.create());
     }
-
-    public OSMMaxAxleLoadParser(DecimalEncodedValue maxAxleLoadEncoder, boolean enableLog) {
+    
+    public OSMMaxAxleLoadParser(DecimalEncodedValue maxAxleLoadEncoder) {
         this.maxAxleLoadEncoder = maxAxleLoadEncoder;
-        this.enableLog = enableLog;
     }
 
     @Override
@@ -49,10 +47,8 @@ public class OSMMaxAxleLoadParser implements TagParser {
     }
 
     @Override
-    public IntsRef handleWayTags(IntsRef edgeFlags, ReaderWay way, EncodingManager.Access access,
-                                 IntsRef relationFlags) {
-        OSMValueExtractor.extractTons(edgeFlags, way, maxAxleLoadEncoder,
-                Collections.singletonList("maxaxleload"), enableLog);
+    public IntsRef handleWayTags(IntsRef edgeFlags, ReaderWay way, EncodingManager.Access access, IntsRef relationFlags) {
+        OSMValueExtractor.extractTons(edgeFlags, way, maxAxleLoadEncoder, Collections.singletonList("maxaxleload"));
         return edgeFlags;
     }
 }
diff --git a/core/src/main/java/com/graphhopper/routing/util/parsers/OSMMaxHeightParser.java b/core/src/main/java/com/graphhopper/routing/util/parsers/OSMMaxHeightParser.java
index 341cb6d10..7f4ae85c3 100644
--- a/core/src/main/java/com/graphhopper/routing/util/parsers/OSMMaxHeightParser.java
+++ b/core/src/main/java/com/graphhopper/routing/util/parsers/OSMMaxHeightParser.java
@@ -32,15 +32,13 @@ import java.util.List;
 public class OSMMaxHeightParser implements TagParser {
 
     private final DecimalEncodedValue heightEncoder;
-    private final boolean enableLog;
 
     public OSMMaxHeightParser() {
-        this(MaxHeight.create(), false);
+        this(MaxHeight.create());
     }
-
-    public OSMMaxHeightParser(DecimalEncodedValue heightEncoder, boolean enableLog) {
+    
+    public OSMMaxHeightParser(DecimalEncodedValue heightEncoder) {
         this.heightEncoder = heightEncoder;
-        this.enableLog = enableLog;
     }
 
     @Override
@@ -51,7 +49,7 @@ public class OSMMaxHeightParser implements TagParser {
     @Override
     public IntsRef handleWayTags(IntsRef edgeFlags, ReaderWay way, EncodingManager.Access access, IntsRef relationFlags) {
         List<String> heightTags = Arrays.asList("maxheight", "maxheight:physical"/*, the OSM tag "height" is not used for the height of a road, so omit it here! */);
-        OSMValueExtractor.extractMeter(edgeFlags, way, heightEncoder, heightTags, enableLog);
+        OSMValueExtractor.extractMeter(edgeFlags, way, heightEncoder, heightTags);
         return edgeFlags;
     }
 }
diff --git a/core/src/main/java/com/graphhopper/routing/util/parsers/OSMMaxLengthParser.java b/core/src/main/java/com/graphhopper/routing/util/parsers/OSMMaxLengthParser.java
index 57f5ef2df..bacc4e8fe 100644
--- a/core/src/main/java/com/graphhopper/routing/util/parsers/OSMMaxLengthParser.java
+++ b/core/src/main/java/com/graphhopper/routing/util/parsers/OSMMaxLengthParser.java
@@ -32,15 +32,13 @@ import java.util.List;
 public class OSMMaxLengthParser implements TagParser {
 
     private final DecimalEncodedValue lengthEncoder;
-    private final boolean enableLog;
 
     public OSMMaxLengthParser() {
-        this(MaxLength.create(), false);
+        this(MaxLength.create());
     }
-
-    public OSMMaxLengthParser(DecimalEncodedValue lengthEncoder, boolean enableLog) {
+    
+    public OSMMaxLengthParser(DecimalEncodedValue lengthEncoder) {
         this.lengthEncoder = lengthEncoder;
-        this.enableLog = enableLog;
     }
 
     @Override
@@ -49,10 +47,8 @@ public class OSMMaxLengthParser implements TagParser {
     }
 
     @Override
-    public IntsRef handleWayTags(IntsRef edgeFlags, ReaderWay way, EncodingManager.Access access,
-                                 IntsRef relationFlags) {
-        OSMValueExtractor.extractMeter(edgeFlags, way, lengthEncoder,
-                Collections.singletonList("maxlength"), enableLog);
+    public IntsRef handleWayTags(IntsRef edgeFlags, ReaderWay way, EncodingManager.Access access, IntsRef relationFlags) {
+        OSMValueExtractor.extractMeter(edgeFlags, way, lengthEncoder, Collections.singletonList("maxlength"));
         return edgeFlags;
     }
 }
diff --git a/core/src/main/java/com/graphhopper/routing/util/parsers/OSMMaxWeightParser.java b/core/src/main/java/com/graphhopper/routing/util/parsers/OSMMaxWeightParser.java
index daeb60dce..228e27593 100644
--- a/core/src/main/java/com/graphhopper/routing/util/parsers/OSMMaxWeightParser.java
+++ b/core/src/main/java/com/graphhopper/routing/util/parsers/OSMMaxWeightParser.java
@@ -32,15 +32,13 @@ import java.util.List;
 public class OSMMaxWeightParser implements TagParser {
 
     private DecimalEncodedValue weightEncoder;
-    private boolean enableLog;
 
     public OSMMaxWeightParser() {
-        this(MaxWeight.create(), false);
+        this(MaxWeight.create());
     }
-
-    public OSMMaxWeightParser(DecimalEncodedValue weightEncoder, boolean enableLog) {
+    
+    public OSMMaxWeightParser(DecimalEncodedValue weightEncoder) {
         this.weightEncoder = weightEncoder;
-        this.enableLog = enableLog;
     }
 
     @Override
@@ -52,7 +50,7 @@ public class OSMMaxWeightParser implements TagParser {
     public IntsRef handleWayTags(IntsRef edgeFlags, ReaderWay way, EncodingManager.Access access, IntsRef relationFlags) {
         // do not include OSM tag "height" here as it has completely different meaning (height of peak)
         List<String> weightTags = Arrays.asList("maxweight", "maxgcweight");
-        OSMValueExtractor.extractTons(edgeFlags, way, weightEncoder, weightTags, enableLog);
+        OSMValueExtractor.extractTons(edgeFlags, way, weightEncoder, weightTags);
         return edgeFlags;
     }
 }
diff --git a/core/src/main/java/com/graphhopper/routing/util/parsers/OSMMaxWidthParser.java b/core/src/main/java/com/graphhopper/routing/util/parsers/OSMMaxWidthParser.java
index f95a99386..b1be50936 100644
--- a/core/src/main/java/com/graphhopper/routing/util/parsers/OSMMaxWidthParser.java
+++ b/core/src/main/java/com/graphhopper/routing/util/parsers/OSMMaxWidthParser.java
@@ -32,15 +32,13 @@ import java.util.List;
 public class OSMMaxWidthParser implements TagParser {
 
     private final DecimalEncodedValue widthEncoder;
-    private final boolean enableLog;
 
     public OSMMaxWidthParser() {
-        this(MaxWidth.create(), false);
+        this(MaxWidth.create());
     }
-
-    public OSMMaxWidthParser(DecimalEncodedValue widthEncoder, boolean enableLog) {
+    
+    public OSMMaxWidthParser(DecimalEncodedValue widthEncoder) {
         this.widthEncoder = widthEncoder;
-        this.enableLog = enableLog;
     }
 
     @Override
@@ -51,7 +49,7 @@ public class OSMMaxWidthParser implements TagParser {
     @Override
     public IntsRef handleWayTags(IntsRef edgeFlags, ReaderWay way, EncodingManager.Access access, IntsRef relationFlags) {
         List<String> widthTags = Arrays.asList("maxwidth", "maxwidth:physical", "width");
-        OSMValueExtractor.extractMeter(edgeFlags, way, widthEncoder, widthTags, enableLog);
+        OSMValueExtractor.extractMeter(edgeFlags, way, widthEncoder, widthTags);
         return edgeFlags;
     }
 }
diff --git a/core/src/main/java/com/graphhopper/routing/util/parsers/helpers/OSMValueExtractor.java b/core/src/main/java/com/graphhopper/routing/util/parsers/helpers/OSMValueExtractor.java
index ac758e357..92fc6a7cc 100644
--- a/core/src/main/java/com/graphhopper/routing/util/parsers/helpers/OSMValueExtractor.java
+++ b/core/src/main/java/com/graphhopper/routing/util/parsers/helpers/OSMValueExtractor.java
@@ -1,19 +1,15 @@
 package com.graphhopper.routing.util.parsers.helpers;
 
-import com.graphhopper.reader.ReaderWay;
-import com.graphhopper.routing.profiles.DecimalEncodedValue;
-import com.graphhopper.storage.IntsRef;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
+import static com.graphhopper.util.Helper.toLowerCase;
 
 import java.util.List;
 import java.util.regex.Pattern;
 
-import static com.graphhopper.util.Helper.toLowerCase;
+import com.graphhopper.reader.ReaderWay;
+import com.graphhopper.routing.profiles.DecimalEncodedValue;
+import com.graphhopper.storage.IntsRef;
 
 public class OSMValueExtractor {
-
-    private static final Logger LOG = LoggerFactory.getLogger(OSMValueExtractor.class);
     
     private static final Pattern TON_PATTERN    = Pattern.compile("tons?");
     private static final Pattern MGW_PATTERN    = Pattern.compile("mgw");
@@ -27,24 +23,24 @@ public class OSMValueExtractor {
         // utility class
     }
 
-    public static void extractTons(IntsRef edgeFlags, ReaderWay way, DecimalEncodedValue valueEncoder, List<String> keys, boolean enableLog) {
-        String value = way.getFirstPriorityTag(keys);
-        try {
-            double val = stringToTons(value);
-            if (val > valueEncoder.getMaxDecimal())
-                val = valueEncoder.getMaxDecimal();
-            valueEncoder.setDecimal(false, edgeFlags, val);
-        } catch (Exception ex) {
-            if (enableLog)
-                LOG.warn("Unable to extract tons from malformed road attribute '{}' for way (OSM_ID = {}).", value, way.getId());
+    public static void extractTons(IntsRef edgeFlags, ReaderWay way, DecimalEncodedValue valueEncoder, List<String> keys) {
+        final String rawValue = way.getFirstPriorityTag(keys);
+        double value = stringToTons(rawValue);
+        
+        if (Double.isNaN(value)) {
+            return;
         }
+        
+        if (value > valueEncoder.getMaxDecimal())
+            value = valueEncoder.getMaxDecimal();
+        valueEncoder.setDecimal(false, edgeFlags, value);
     }
 
     public static double stringToTons(String value) {
         value = TON_PATTERN.matcher(toLowerCase(value)).replaceAll("t");
         value = MGW_PATTERN.matcher(value).replaceAll("").trim();
         if (isInvalid(value))
-            throw new NumberFormatException("Cannot parse value for 'tons': " + value);
+            return Double.NaN;
 
         double factor = 1;
         if (value.endsWith("t")) {
@@ -57,20 +53,24 @@ public class OSMValueExtractor {
             factor = 0.001;
         }
 
-        return Double.parseDouble(value) * factor;
+        try {
+            return Double.parseDouble(value) * factor;
+        } catch (NumberFormatException e) {
+            return Double.NaN;
+        }
     }
 
-    public static void extractMeter(IntsRef edgeFlags, ReaderWay way, DecimalEncodedValue valueEncoder, List<String> keys, boolean enableLog) {
-        String value = way.getFirstPriorityTag(keys);
-        try {
-            double val = stringToMeter(value);
-            if (val > valueEncoder.getMaxDecimal())
-                val = valueEncoder.getMaxDecimal();
-            valueEncoder.setDecimal(false, edgeFlags, val);
-        } catch (Exception ex) {
-            if (enableLog)
-                LOG.warn("Unable to extract meter from malformed road attribute '{}' for way (OSM_ID = {}).", value, way.getId());
+    public static void extractMeter(IntsRef edgeFlags, ReaderWay way, DecimalEncodedValue valueEncoder, List<String> keys) {
+        final String rawValue = way.getFirstPriorityTag(keys);
+        double value = stringToMeter(rawValue);
+        
+        if (Double.isNaN(value)) {
+            return;
         }
+
+        if (value > valueEncoder.getMaxDecimal())
+            value = valueEncoder.getMaxDecimal();
+        valueEncoder.setDecimal(false, edgeFlags, value);
     }
 
     public static double stringToMeter(String value) {
@@ -79,7 +79,7 @@ public class OSMValueExtractor {
         value = INCH_PATTERN.matcher(value).replaceAll("in");
         value = FEET_PATTERN.matcher(value).replaceAll("ft");
         if (isInvalid(value))
-            throw new NumberFormatException("Cannot parse value for 'meter': " + value);
+            return Double.NaN;
         double factor = 1;
         double offset = 0;
         if (value.startsWith("~") || value.contains("approx")) {
@@ -98,7 +98,11 @@ public class OSMValueExtractor {
 
             inchValue = value.substring(startIndex, value.length() - 2);
             value = value.substring(0, startIndex);
-            offset = Double.parseDouble(inchValue) * 0.0254;
+            try {
+                offset = Double.parseDouble(inchValue) * 0.0254;
+            } catch (NumberFormatException e) {
+                return Double.NaN;
+            }
         }
 
         if (value.endsWith("ft")) {
@@ -113,8 +117,12 @@ public class OSMValueExtractor {
 
         if (value.isEmpty()) {
             return offset;
-        } else {
+        }
+        
+        try {
             return Double.parseDouble(value) * factor + offset;
+        } catch (NumberFormatException e) {
+            return Double.NaN;
         }
     }
 


Test patch:
diff --git a/core/src/test/java/com/graphhopper/routing/util/parsers/OSMValueExtractorTest.java b/core/src/test/java/com/graphhopper/routing/util/parsers/OSMValueExtractorTest.java
index 11686b4ff..ab1b012ce 100644
--- a/core/src/test/java/com/graphhopper/routing/util/parsers/OSMValueExtractorTest.java
+++ b/core/src/test/java/com/graphhopper/routing/util/parsers/OSMValueExtractorTest.java
@@ -4,6 +4,7 @@ import com.graphhopper.routing.util.parsers.helpers.OSMValueExtractor;
 import org.junit.Test;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
 
 public class OSMValueExtractorTest {
 
@@ -25,14 +26,14 @@ public class OSMValueExtractorTest {
         assertEquals(6, OSMValueExtractor.stringToTons("6t mgw"), DELTA);
     }
 
-    @Test(expected = NumberFormatException.class)
-    public void stringToTonsException() {
-        OSMValueExtractor.stringToTons("weight limit 1.5t");
+    @Test
+    public void stringToTonsNaN() {
+        assertTrue(Double.isNaN(OSMValueExtractor.stringToTons("weight limit 1.5t")));
     }
 
-    @Test(expected = NumberFormatException.class)
-    public void stringToTonsException2() {
-        OSMValueExtractor.stringToTons("");
+    @Test
+    public void stringToTonsNaN2() {
+        assertTrue(Double.isNaN(OSMValueExtractor.stringToTons("")));
     }
 
     @Test
@@ -63,18 +64,18 @@ public class OSMValueExtractorTest {
         assertEquals(1.5, OSMValueExtractor.stringToMeter("150 cm"), DELTA);
     }
 
-    @Test(expected = NumberFormatException.class)
-    public void stringToMeterException() {
-        OSMValueExtractor.stringToMeter("height limit 1.5m");
+    @Test
+    public void stringToMeterNaN() {
+        assertTrue(Double.isNaN(OSMValueExtractor.stringToMeter("height limit 1.5m")));
     }
 
-    @Test(expected = NumberFormatException.class)
-    public void stringToMeterException2() {
-        OSMValueExtractor.stringToMeter("");
+    @Test
+    public void stringToMeterNaN2() {
+        assertTrue(Double.isNaN(OSMValueExtractor.stringToMeter("")));
     }
 
-    @Test(expected = NumberFormatException.class)
-    public void stringToMeterException3() {
-        OSMValueExtractor.stringToMeter("default");
+    @Test
+    public void stringToMeterNaN3() {
+        assertTrue(Double.isNaN(OSMValueExtractor.stringToMeter("default")));
     }
 }
\ No newline at end of file