From 55637e106573b3b2993dbb930f045b5d59c146d9 Mon Sep 17 00:00:00 2001 From: Arthur Little <1690572+littleaj@users.noreply.github.com> Date: Thu, 27 Dec 2018 15:53:42 -0800 Subject: [PATCH] simplified MapUtil.copy (#796) * simplified the MapUtil copy method since all callers were checking if source was null/empty. --- .../applicationinsights/TelemetryClient.java | 26 +++---------- .../internal/util/MapUtil.java | 10 +++-- .../telemetry/TelemetryContext.java | 8 +--- .../internal/util/MapUtilTest.java | 37 +++++++++++++++++-- 4 files changed, 47 insertions(+), 34 deletions(-) diff --git a/core/src/main/java/com/microsoft/applicationinsights/TelemetryClient.java b/core/src/main/java/com/microsoft/applicationinsights/TelemetryClient.java index b2525459e84..227517fd2a9 100644 --- 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 properties, Map 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 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 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 index 70248745700..e46bad92f4b 100644 --- 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 The type of the values in both maps * @throws IllegalArgumentException if either {@code source} or {@code target} are null. */ public static void copy(Map source, Map 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 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 index 83da5fe8d7c..7f60f184607 100644 --- 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 index a8de007c9a4..2a3dd97be50 100644 --- 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(), null); + } + + @Test + public void copyIsNoOpIfSourceIsNullOrEmpty() { + Map source = mock(Map.class); + Map 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 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()); } }