simplified MapUtil.copy (#796)

* simplified the MapUtil copy method since all callers were checking if source was null/empty.
microsoft · Dec 27, 2018 · 55637e1 · 55637e1
1 parent 9c04d50
commit 55637e1
Show file tree

Hide file tree

Showing 4 changed files with 47 additions and 34 deletions.
diff --git a/core/src/main/java/com/microsoft/applicationinsights/TelemetryClient.java b/core/src/main/java/com/microsoft/applicationinsights/TelemetryClient.java
@@ -138,13 +138,8 @@ public void trackEvent(String name, Map<String, String> properties, Map<String,
 
         EventTelemetry et = new EventTelemetry(name);
 
-        if (properties != null && properties.size() > 0) {
-            MapUtil.copy(properties, et.getContext().getProperties());
-        }
-
-        if (metrics != null && metrics.size() > 0) {
-            MapUtil.copy(metrics, et.getMetrics());
-        }
+        MapUtil.copy(properties, et.getContext().getProperties());
+        MapUtil.copy(metrics, et.getMetrics());
 
         this.track(et);
     }
@@ -182,9 +177,7 @@ public void trackTrace(String message, SeverityLevel severityLevel, Map<String,
 
         TraceTelemetry et = new TraceTelemetry(message, severityLevel);
 
-        if (properties != null && properties.size() > 0) {
-            MapUtil.copy(properties, et.getContext().getProperties());
-        }
+        MapUtil.copy(properties, et.getContext().getProperties());
 
         this.track(et);
     }
@@ -252,9 +245,7 @@ public void trackMetric(String name, double value, Integer sampleCount, Double m
         mt.setMin(min);
         mt.setMax(max);
         mt.setStandardDeviation(stdDev);
-        if (properties != null && properties.size() > 0) {
-            MapUtil.copy(properties, mt.getProperties());
-        }
+        MapUtil.copy(properties, mt.getProperties());
         this.track(mt);
     }
 
@@ -289,13 +280,8 @@ public void trackException(Exception exception, Map<String, String> properties,
 
         ExceptionTelemetry et = new ExceptionTelemetry(exception);
 
-        if (properties != null && properties.size() > 0) {
-            MapUtil.copy(properties, et.getContext().getProperties());
-        }
-
-        if (metrics != null && metrics.size() > 0) {
-            MapUtil.copy(metrics, et.getMetrics());
-        }
+        MapUtil.copy(properties, et.getContext().getProperties());
+        MapUtil.copy(metrics, et.getMetrics());
 
         this.track(et);
     }

diff --git a/core/src/main/java/com/microsoft/applicationinsights/internal/util/MapUtil.java b/core/src/main/java/com/microsoft/applicationinsights/internal/util/MapUtil.java
@@ -38,18 +38,20 @@ public class MapUtil
     /**
      * Copies entries from the source map to the target map, overwrites any values in target.
      * Filters out null values if target is a {@link ConcurrentHashMap}.
-     * @param source the source map. Cannot be null.
+     * @param source the source map. If null or empty, this is a nop.
      * @param target the target map. Cannot be null.
      * @param <Value> The type of the values in both maps
      * @throws IllegalArgumentException if either {@code source} or {@code target} are null.
      */
     public static <Value> void copy(Map<String, Value> source, Map<String, Value> target) {
-        if (source == null) {
-            throw new IllegalArgumentException("source must not be null");
-        }
         if (target == null) {
             throw new IllegalArgumentException("target must not be null");
         }
+
+        if (source == null || source.isEmpty()) {
+            return;
+        }
+
         for (Map.Entry<String,Value> entry : source.entrySet()) {
             String key = entry.getKey();
             if (Strings.isNullOrEmpty(key)) {

diff --git a/core/src/main/java/com/microsoft/applicationinsights/telemetry/TelemetryContext.java b/core/src/main/java/com/microsoft/applicationinsights/telemetry/TelemetryContext.java
@@ -200,12 +200,8 @@ public void initialize(TelemetryContext source) {
         if (Strings.isNullOrEmpty(this.instrumentationKey) && !Strings.isNullOrEmpty(source.getInstrumentationKey()))
             setInstrumentationKey(source.getInstrumentationKey());
 
-        if (source.tags != null && source.tags.size() > 0) {
-            MapUtil.copy(source.tags, this.tags);
-        }
-        if (source.properties != null && source.properties.size() > 0) {
-            MapUtil.copy(source.properties, this.properties);
-        }
+        MapUtil.copy(source.tags, this.tags);
+        MapUtil.copy(source.properties, this.properties);
     }
 
     public InternalContext getInternal() {

diff --git a/core/src/test/java/com/microsoft/applicationinsights/internal/util/MapUtilTest.java b/core/src/test/java/com/microsoft/applicationinsights/internal/util/MapUtilTest.java
@@ -1,14 +1,43 @@
 package com.microsoft.applicationinsights.internal.util;
 
-import org.junit.Assert;
+import org.junit.*;
+import org.junit.rules.ExpectedException;
+
 import java.util.HashMap;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
 
-import org.junit.Test;
+import static org.junit.Assert.*;
+import static org.mockito.Mockito.*;
 
 public class MapUtilTest {
 
+    @Rule
+    public ExpectedException expected = ExpectedException.none();
+
+    @Test
+    public void targetCannotBeNullInCopy() {
+        expected.expect(IllegalArgumentException.class);
+        MapUtil.copy(new HashMap<String, String>(), null);
+    }
+
+    @Test
+    public void copyIsNoOpIfSourceIsNullOrEmpty() {
+        Map<String, String> source = mock(Map.class);
+        Map<String, String> target = mock(Map.class);
+        when(source.size()).thenReturn(0);
+
+        MapUtil.copy(source, target);
+        // nothing should be put into target
+        verify(target, never()).put(anyString(), anyString());
+        verify(source, never()).get(any());
+
+        reset(target);
+
+        MapUtil.copy(null, target);
+        verify(target, never()).put(anyString(), anyString());
+    }
+
     @Test
     public void testCopyIntoHashMap() {
         Map<String, String> source = new HashMap<>();
@@ -18,7 +47,7 @@ public void testCopyIntoHashMap() {
         source.put("key2", null);
 
         MapUtil.copy(source, target);
-        Assert.assertEquals(2, target.size());
+        assertEquals(2, target.size());
     }
 
     @Test
@@ -30,6 +59,6 @@ public void testCopyIntoConcurrentHashMap() {
         source.put("key2", null);
 
         MapUtil.copy(source, target);
-        Assert.assertEquals(1, target.size());
+        assertEquals(1, target.size());
     }
 }