checkstyle and refactoring

BrandonArp · BrandonArp · commit 0e7bbb7c8f1d · 2018-03-02T09:55:00.000-08:00
diff --git a/src/main/java/com/arpnetworking/utility/RegexAndMapReplacer.java b/src/main/java/com/arpnetworking/utility/RegexAndMapReplacer.java
@@ -15,8 +15,6 @@
  */
 package com.arpnetworking.utility;
 
-import scala.Int;
-
 import java.util.Map;
 import java.util.regex.Matcher;
 
@@ -33,7 +31,7 @@ public final class RegexAndMapReplacer {
      * @param input input string to match against
      * @param replace replacement string
      * @param variables map of variables to include
-     * @return
+     * @return a string with replacement tokens replaced
      */
     public static String replaceAll(final Matcher matcher, final String input, final String replace, final Map<String, String> variables) {
 
@@ -59,73 +57,87 @@ public static String replaceAll(final Matcher matcher, final String input, final
 
     private static void appendReplacement(final Matcher matcher, final String replacement, final StringBuilder replacementBuilder,
                                           final Map<String, String> variables) {
-        final StringBuilder tokenStringBuilder = new StringBuilder();
-        boolean inEscape = false;
-        boolean readingReplaceToken = false;
-        boolean inReplaceBrackets = false;
-        boolean replaceTokenNumeric = true; // assume token is numeric, any non-numeric character will set this to false
-        for (int x = 0; x < replacement.length(); x++) {
+        final StringBuilder tokenBuilder = new StringBuilder();
+        int x = -1;
+        while (x < replacement.length() - 1) {
+            x++;
             final char c = replacement.charAt(x);
-            if (!inEscape && c == '\\') {
-                inEscape = true;
-                continue;
-            }
-            if (inEscape) {
-                final StringBuilder builder;
-                if (readingReplaceToken) {
-                    builder = tokenStringBuilder;
+            if (c == '\\') {
+                x++;
+                processEscapedCharacter(replacement, x, replacementBuilder);
+            } else {
+                if (c == '$') {
+                    x += writeReplacementToken(replacement, x, replacementBuilder, matcher, variables, tokenBuilder);
                 } else {
-                    builder = replacementBuilder;
+                    replacementBuilder.append(c);
                 }
-                if (c == '\\' || c == '$' || c == '}') {
-                    builder.append(c);
-                    inEscape = false;
+            }
+        }
+    }
+
+    private static void processEscapedCharacter(final String replacement, final int x, final StringBuilder builder) {
+        if (x >= replacement.length()) {
+            throw new IllegalArgumentException(
+                    String.format("Improper escaping in replacement, must not have trailing '\\' at col %d: %s", x, replacement));
+        }
+        final Character c = replacement.charAt(x);
+        if (c == '\\' || c == '$' || c == '}') {
+            builder.append(c);
+        } else {
+            throw new IllegalArgumentException(
+                    String.format("Improperly escaped '%s' in replacement at col %d: %s", c, x, replacement));
+        }
+    }
+
+    private static int writeReplacementToken(final String replacement, final int offset, final StringBuilder output,
+            final Matcher matcher, final Map<String, String> variables, final StringBuilder tokenBuilder) {
+        boolean inReplaceBrackets = false;
+        boolean tokenNumeric = true;
+        tokenBuilder.setLength(0);  // reset the shared builder
+        int x = offset + 1;
+        char c = replacement.charAt(x);
+
+        // Optionally consume the opening brace
+        if (c == '{') {
+            inReplaceBrackets = true;
+            x++;
+            c = replacement.charAt(x);
+        }
+
+        if (inReplaceBrackets) {
+            // Consume until we hit the }
+            while (x < replacement.length() - 1 && c != '}') {
+                if (c == '\\') {
+                    x++;
+                    processEscapedCharacter(replacement, x, tokenBuilder);
                 } else {
-                    throw new IllegalArgumentException("Improperly escaped '" + String.valueOf(c) + "' in replacement at col " + x + ": " + replacement);
+                    tokenBuilder.append(c);
                 }
-            } else if (readingReplaceToken) {
-                if (c == '{') {
-                    inReplaceBrackets = true;
-                    continue;
-                } else if (c == '}' && inReplaceBrackets) {
-                        final String replaceToken = tokenStringBuilder.toString();
-                        replacementBuilder.append(getReplacement(matcher, replaceToken, replaceTokenNumeric, variables));
-                        tokenStringBuilder.setLength(0);
-                        inReplaceBrackets = false;
-                        readingReplaceToken = false;
-                        continue;
-                } else if (!Character.isDigit(c)) {
-                    if (inReplaceBrackets) {
-                        replaceTokenNumeric = false;
-                        tokenStringBuilder.append(c);
-                    } else {
-                        final String replaceToken = tokenStringBuilder.toString();
-                        if (replaceToken.isEmpty()) {
-                            throw new IllegalArgumentException("Non-numeric replacements must be of the form ${val}. Missing '{' at col "
-                                    + x + ": " + replacement);
-                        }
-                        replacementBuilder.append(getReplacement(matcher, replaceToken, replaceTokenNumeric, variables));
-                        tokenStringBuilder.setLength(0);
-                        readingReplaceToken = false;
-                        x--; // We can't process the character because we are no longer in the numeric group syntax, we need to process the
-                             // $n replacement and then evaluate this character again.
-                        continue;
-                    }
-                } else {
-                    tokenStringBuilder.append(c);
+                if (tokenNumeric && !Character.isDigit(c)) {
+                    tokenNumeric = false;
                 }
-            } else {
-                if (c == '$') {
-                    readingReplaceToken = true;
+                x++;
+                c = replacement.charAt(x);
+            }
+            if (c != '}') {
+                throw new IllegalArgumentException("Invalid replacement token, expected '}' at col " + x + ": " + replacement);
+            }
+            x++; // Consume the }
+            output.append(getReplacement(matcher, tokenBuilder.toString(), tokenNumeric, variables));
+        } else {
+            // Consume until we hit a non-digit character
+            while (x < replacement.length()) {
+                c = replacement.charAt(x);
+                if (Character.isDigit(c)) {
+                    tokenBuilder.append(c);
                 } else {
-                    replacementBuilder.append(c);
+                    break;
                 }
+                x++;
             }
+            output.append(getReplacement(matcher, tokenBuilder.toString(), true, variables));
         }
-        if (tokenStringBuilder.length() > 0 && replaceTokenNumeric) {
-            final String replaceToken = tokenStringBuilder.toString();
-            replacementBuilder.append(getReplacement(matcher, replaceToken, true, variables));
-        }
+        return x - offset - 1;
     }
 
     private static String getReplacement(final Matcher matcher, final String replaceToken, final boolean numeric,
@@ -142,5 +154,5 @@ private static String getReplacement(final Matcher matcher, final String replace
         }
     }
 
-    private RegexAndMapReplacer () { }
+    private RegexAndMapReplacer() { }
 }
diff --git a/src/test/java/com/arpnetworking/metrics/mad/sources/MappingSourceTest.java b/src/test/java/com/arpnetworking/metrics/mad/sources/MappingSourceTest.java
@@ -54,7 +54,7 @@ public void setUp() {
                         "foo/([^/]*)/bar", ImmutableList.of("foo/bar"),
                         "cat/([^/]*)/dog", ImmutableList.of("cat/dog", "cat/dog/$1"),
                         "tagged/([^/]*)/dog", ImmutableList.of("tagged/dog;animal=$1"),
-                        "tagged/([^/]*)/dog", ImmutableList.of("tagged/$animal/dog")))
+                        "tagged/([^/]*)/animal", ImmutableList.of("tagged/$animal/animal")))
                 .setSource(_mockSource);
     }
 
diff --git a/src/test/java/com/arpnetworking/utility/RegexAndMapBenchmarkTest.java b/src/test/java/com/arpnetworking/utility/RegexAndMapBenchmarkTest.java
@@ -1,18 +1,35 @@
+/**
+ * Copyright 2018 Inscope Metrics, Inc
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
 package com.arpnetworking.utility;
 
 import com.carrotsearch.junitbenchmarks.BenchmarkOptions;
 import com.carrotsearch.junitbenchmarks.BenchmarkRule;
-import com.google.common.collect.ImmutableMap;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.TestRule;
 
 import java.util.Collections;
 import java.util.regex.Pattern;
 
+/**
+ * Benchmark tests comparing the RegexAndMapReplacer class against the built-in regex replacement.
+ *
+ * @author Brandon Arp (brandon dot arp at inscopemetrics dot com)
+ */
 public class RegexAndMapBenchmarkTest {
-    @Rule
-    public TestRule benchmarkRun = new BenchmarkRule();
 
     @BenchmarkOptions(benchmarkRounds = 2000000, warmupRounds = 50000)
     @Test
@@ -26,7 +43,9 @@ public void testRegex() {
         final String result = PATTERN.matcher(INPUT).replaceAll(REPLACE);
     }
 
+    @Rule
+    public TestRule _benchmarkRun = new BenchmarkRule();
     private static final String REPLACE = "this is a ${g1} pattern called ${g2}";
-    private static Pattern PATTERN = Pattern.compile("(?<g1>test)/pattern/(?<g2>foo)");
+    private static final Pattern PATTERN = Pattern.compile("(?<g1>test)/pattern/(?<g2>foo)");
     private static final String INPUT = "test/pattern/foo";
 }
diff --git a/src/test/java/com/arpnetworking/utility/RegexAndMapReplacerTest.java b/src/test/java/com/arpnetworking/utility/RegexAndMapReplacerTest.java
@@ -1,3 +1,18 @@
+/**
+ * Copyright 2018 Inscope Metrics, Inc
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
 package com.arpnetworking.utility;
 
 import com.google.common.collect.ImmutableMap;
@@ -8,6 +23,11 @@
 import java.util.Map;
 import java.util.regex.Pattern;
 
+/**
+ * Tests for the {@link RegexAndMapReplacer} class.
+ *
+ * @author Brandon Arp (brandon dot arp at inscopemetrics dot com)
+ */
 public class RegexAndMapReplacerTest {
     @Test
     public void testNoMatch() {
@@ -26,6 +46,22 @@ public void testInvalidEscape() {
         final String result = RegexAndMapReplacer.replaceAll(pattern.matcher(input), input, replace, Collections.emptyMap());
     }
 
+    @Test(expected = IllegalArgumentException.class)
+    public void testMissingClosingCurly() {
+        final Pattern pattern = Pattern.compile("test");
+        final String input = "test";
+        final String replace = "${0"; // no ending }
+        final String result = RegexAndMapReplacer.replaceAll(pattern.matcher(input), input, replace, Collections.emptyMap());
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void testInvalidEscapeAtEnd() {
+        final Pattern pattern = Pattern.compile("test");
+        final String input = "test";
+        final String replace = "${0}\\"; // trailing \
+        final String result = RegexAndMapReplacer.replaceAll(pattern.matcher(input), input, replace, Collections.emptyMap());
+    }
+
     @Test
     public void testNumericWithClosingCurly() {
         final Pattern pattern = Pattern.compile("test");
@@ -158,6 +194,6 @@ private void testExpression(final Pattern pattern, final String input, final Str
         try {
             final String stockResult = pattern.matcher(input).replaceAll(replace);
             Assert.assertEquals(expected, stockResult);
-        } catch (final IllegalArgumentException ignored) {}
+        } catch (final IllegalArgumentException ignored) { }
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -54,7 +54,7 @@ public void setUp() {`
`54`	`54`	`"foo/([^/]*)/bar", ImmutableList.of("foo/bar"),`
`55`	`55`	`"cat/([^/]*)/dog", ImmutableList.of("cat/dog", "cat/dog/$1"),`
`56`	`56`	`"tagged/([^/]*)/dog", ImmutableList.of("tagged/dog;animal=$1"),`
`57`		`- "tagged/([^/]*)/dog", ImmutableList.of("tagged/$animal/dog")))`
	`57`	`+ "tagged/([^/]*)/animal", ImmutableList.of("tagged/$animal/animal")))`
`58`	`58`	`.setSource(_mockSource);`
`59`	`59`	`}`
`60`	`60`