Merge pull request #156 from martincostello/Comments-Review

Comments review

src/JustEat.StatsD/Buffered/StatsDUtf8Formatter.cs

@@ -4,7 +4,7 @@
 
 namespace JustEat.StatsD.Buffered
 {
-    internal class StatsDUtf8Formatter
+    internal sealed class StatsDUtf8Formatter
     {
         private readonly byte[] _utf8Prefix;
 

src/JustEat.StatsD/DisposableTimer.cs

@@ -10,18 +10,18 @@ internal sealed class DisposableTimer : IDisposableTimer
         private IStatsDPublisher _publisher;
         private Stopwatch _stopwatch;
 
-        public string StatName { get; set; }
+        public string Bucket { get; set; }
 
-        public DisposableTimer(IStatsDPublisher publisher, string statName)
+        public DisposableTimer(IStatsDPublisher publisher, string bucket)
         {
             _publisher = publisher ?? throw new ArgumentNullException(nameof(publisher));
 
-            if (string.IsNullOrEmpty(statName))
+            if (string.IsNullOrEmpty(bucket))
             {
-                throw new ArgumentNullException(nameof(statName));
+                throw new ArgumentNullException(nameof(bucket));
             }
 
-            StatName = statName;
+            Bucket = bucket;
             _stopwatch = Stopwatch.StartNew();
         }
 
@@ -32,12 +32,12 @@ public void Dispose()
                 _disposed = true;
                 _stopwatch.Stop();
 
-                if (string.IsNullOrEmpty(StatName))
+                if (string.IsNullOrEmpty(Bucket))
                 {
-                    throw new InvalidOperationException($"The {nameof(StatName)} property must have a value.");
+                    throw new InvalidOperationException($"The {nameof(Bucket)} property must have a value.");
                 }
 
-                _publisher.Timing(_stopwatch.Elapsed, StatName);
+                _publisher.Timing(_stopwatch.Elapsed, Bucket);
 
                 _stopwatch = null;
                 _publisher = null;

src/JustEat.StatsD/EndpointLookups/CachedEndpointSource.cs

@@ -22,9 +22,18 @@ public class CachedEndpointSource : IEndPointSource
         /// <exception cref="ArgumentNullException">
         /// <paramref name="inner"/> is <see langword="null"/>.
         /// </exception>
+        /// <exception cref="ArgumentOutOfRangeException">
+        /// <paramref name="cacheDuration"/> is less than or equal to <see cref="TimeSpan.Zero"/>.
+        /// </exception>
         public CachedEndpointSource(IEndPointSource inner, TimeSpan cacheDuration)
         {
             _inner = inner ?? throw new ArgumentNullException(nameof(inner));
+
+            if (cacheDuration <= TimeSpan.Zero)
+            {
+                throw new ArgumentOutOfRangeException(nameof(cacheDuration), cacheDuration, "The end point cache duration must be a positive TimeSpan value.");
+            }
+
             _cachedValue = null;
             _cacheDuration = cacheDuration;
         }

src/JustEat.StatsD/EndpointLookups/DnsLookupIpEndpointSource.cs

@@ -7,7 +7,7 @@ namespace JustEat.StatsD.EndpointLookups
 {
     /// <summary>
     /// A class representing an implementation of <see cref="IEndPointSource"/> that looks up
-    /// the <see cref="EndPoint"/> for DNS hostname to resolve its IP address.
+    /// the <see cref="EndPoint"/> for a DNS hostname to resolve its IP address.
     /// </summary>
     public class DnsLookupIpEndpointSource : IEndPointSource
     {

src/JustEat.StatsD/IDisposableTimer.cs

@@ -8,8 +8,8 @@ namespace JustEat.StatsD
     public interface IDisposableTimer : IDisposable
     {
         /// <summary>
-        /// Gets or sets the name of the StatsD bucket associated with the timer.
+        /// Gets or sets the StatsD bucket associated with the timer.
         /// </summary>
-        string StatName { get; set; }
+        string Bucket { get; set; }
     }
 }

src/JustEat.StatsD/SocketProtocol.cs

@@ -1,9 +1,11 @@
+using System.Net.Sockets;
+
 namespace JustEat.StatsD
 {
     /// <summary>
-    /// The subset of ProtocolType that are supported by SocketTransport
-    /// UDP or IP.
-    /// UDP is the default, but IP transport is required for AWS Lambdas.
+    /// An enumeration defining the subset of <see cref="ProtocolType"/> values that are supported by <see cref="SocketTransport"/>.
+    /// <para />
+    /// UDP is the default, but IP transport is required for some environments such as AWS Lambda functions.
     /// </summary>
     public enum SocketProtocol
     {

src/JustEat.StatsD/TimerExtensions.cs

@@ -18,6 +18,9 @@ public static class TimerExtensions
         /// <returns>
         /// An <see cref="IDisposableTimer"/> that publishes the metric when the instance is disposed of.
         /// </returns>
+        /// <exception cref="ArgumentNullException">
+        /// <paramref name="publisher"/> or <paramref name="bucket"/> is <see langword="null"/>.
+        /// </exception>
         public static IDisposableTimer StartTimer(this IStatsDPublisher publisher, string bucket)
         {
             return new DisposableTimer(publisher, bucket);
@@ -30,8 +33,16 @@ public static IDisposableTimer StartTimer(this IStatsDPublisher publisher, strin
         /// <param name="publisher">The <see cref="IStatsDPublisher"/> to publish with.</param>
         /// <param name="bucket">The bucket to publish the timer for.</param>
         /// <param name="action">A delegate to a method whose invocation should be timed.</param>
+        /// <exception cref="ArgumentNullException">
+        /// <paramref name="publisher"/>, <paramref name="bucket"/> or <paramref name="action"/> is <see langword="null"/>.
+        /// </exception>
         public static void Time(this IStatsDPublisher publisher, string bucket, Action action)
         {
+            if (action == null)
+            {
+                throw new ArgumentNullException(nameof(action));
+            }
+
             using (StartTimer(publisher, bucket))
             {
                 action();
@@ -45,8 +56,16 @@ public static void Time(this IStatsDPublisher publisher, string bucket, Action a
         /// <param name="publisher">The <see cref="IStatsDPublisher"/> to publish with.</param>
         /// <param name="bucket">The bucket to publish the timer for.</param>
         /// <param name="action">A delegate to a method whose invocation should be timed.</param>
+        /// <exception cref="ArgumentNullException">
+        /// <paramref name="publisher"/>, <paramref name="bucket"/> or <paramref name="action"/> is <see langword="null"/>.
+        /// </exception>
         public static void Time(this IStatsDPublisher publisher, string bucket, Action<IDisposableTimer> action)
         {
+            if (action == null)
+            {
+                throw new ArgumentNullException(nameof(action));
+            }
+
             using (var timer = StartTimer(publisher, bucket))
             {
                 action(timer);
@@ -63,8 +82,16 @@ public static void Time(this IStatsDPublisher publisher, string bucket, Action<I
         /// <returns>
         /// A <see cref="Task"/> representing the asynchronous operation to time.
         /// </returns>
+        /// <exception cref="ArgumentNullException">
+        /// <paramref name="publisher"/>, <paramref name="bucket"/> or <paramref name="action"/> is <see langword="null"/>.
+        /// </exception>
         public static async Task Time(this IStatsDPublisher publisher, string bucket, Func<Task> action)
         {
+            if (action == null)
+            {
+                throw new ArgumentNullException(nameof(action));
+            }
+
             using (StartTimer(publisher, bucket))
             {
                 await action().ConfigureAwait(false);
@@ -82,8 +109,16 @@ public static async Task Time(this IStatsDPublisher publisher, string bucket, Fu
         /// <returns>
         /// A <see cref="Task"/> representing the asynchronous operation to time.
         /// </returns>
+        /// <exception cref="ArgumentNullException">
+        /// <paramref name="publisher"/>, <paramref name="bucket"/> or <paramref name="action"/> is <see langword="null"/>.
+        /// </exception>
         public static async Task Time(this IStatsDPublisher publisher, string bucket, Func<IDisposableTimer, Task> action)
         {
+            if (action == null)
+            {
+                throw new ArgumentNullException(nameof(action));
+            }
+
             using (var timer = StartTimer(publisher, bucket))
             {
                 await action(timer).ConfigureAwait(false);
@@ -101,8 +136,16 @@ public static async Task Time(this IStatsDPublisher publisher, string bucket, Fu
         /// <returns>
         /// The value from invoking <paramref name="func"/>.
         /// </returns>
+        /// <exception cref="ArgumentNullException">
+        /// <paramref name="publisher"/>, <paramref name="bucket"/> or <paramref name="func"/> is <see langword="null"/>.
+        /// </exception>
         public static T Time<T>(this IStatsDPublisher publisher, string bucket, Func<T> func)
         {
+            if (func == null)
+            {
+                throw new ArgumentNullException(nameof(func));
+            }
+
             using (StartTimer(publisher, bucket))
             {
                 return func();
@@ -120,8 +163,16 @@ public static T Time<T>(this IStatsDPublisher publisher, string bucket, Func<T>
         /// <returns>
         /// The value from invoking <paramref name="func"/>.
         /// </returns>
+        /// <exception cref="ArgumentNullException">
+        /// <paramref name="publisher"/>, <paramref name="bucket"/> or <paramref name="func"/> is <see langword="null"/>.
+        /// </exception>
         public static T Time<T>(this IStatsDPublisher publisher, string bucket, Func<IDisposableTimer, T> func)
         {
+            if (func == null)
+            {
+                throw new ArgumentNullException(nameof(func));
+            }
+
             using (var timer = StartTimer(publisher, bucket))
             {
                 return func(timer);
@@ -139,8 +190,16 @@ public static T Time<T>(this IStatsDPublisher publisher, string bucket, Func<IDi
         /// <returns>
         /// A <see cref="Task"/> representing the asynchronous operation to time.
         /// </returns>
+        /// <exception cref="ArgumentNullException">
+        /// <paramref name="publisher"/>, <paramref name="bucket"/> or <paramref name="func"/> is <see langword="null"/>.
+        /// </exception>
         public static async Task<T> Time<T>(this IStatsDPublisher publisher, string bucket, Func<Task<T>> func)
         {
+            if (func == null)
+            {
+                throw new ArgumentNullException(nameof(func));
+            }
+
             using (StartTimer(publisher, bucket))
             {
                 return await func().ConfigureAwait(false);
@@ -158,8 +217,16 @@ public static async Task<T> Time<T>(this IStatsDPublisher publisher, string buck
         /// <returns>
         /// A <see cref="Task"/> representing the asynchronous operation to time.
         /// </returns>
+        /// <exception cref="ArgumentNullException">
+        /// <paramref name="publisher"/>, <paramref name="bucket"/> or <paramref name="func"/> is <see langword="null"/>.
+        /// </exception>
         public static async Task<T> Time<T>(this IStatsDPublisher publisher, string bucket, Func<IDisposableTimer, Task<T>> func)
         {
+            if (func == null)
+            {
+                throw new ArgumentNullException(nameof(func));
+            }
+
             using (var timer = StartTimer(publisher, bucket))
             {
                 return await func(timer).ConfigureAwait(false);

tests/JustEat.StatsD.Tests/EndpointLookups/CachedEndpointSourceTests.cs

@@ -62,6 +62,21 @@ public static async Task CachedValueIsReturnedAgainAfterExpiry()
             mockInner.Verify(x => x.GetEndpoint(), Times.Exactly(2));
         }
 
+        [Theory]
+        [InlineData(0)]
+        [InlineData(-1)]
+        public static void ConstructorThrowsIfCacheDurationIsInvalid(long ticks)
+        {
+            // Arrange
+            var inner = Mock.Of<IEndPointSource>();
+            var cacheDuration = TimeSpan.FromTicks(ticks);
+
+            // Act and Assert
+            var exception = Assert.Throws<ArgumentOutOfRangeException>("cacheDuration", () => new CachedEndpointSource(inner, cacheDuration));
+
+            exception.ActualValue.ShouldBe(cacheDuration);
+        }
+
         private static IPEndPoint MakeTestIpEndPoint()
         {
             return new IPEndPoint(new IPAddress(new byte[] { 1, 2, 3, 4 }), 8125);

tests/JustEat.StatsD.Tests/Extensions/ExtensionsTests.cs

@@ -29,7 +29,7 @@ public static void CanChangeStatName()
             using (var timer = publisher.StartTimer("defaultName"))
             {
                 Delay();
-                timer.StatName = "otherStat";
+                timer.Bucket = "otherStat";
             }
 
             PublisherAssertions.SingleStatNameIs(publisher, "otherStat");
@@ -120,7 +120,7 @@ public static void CanChangeStatNameInAction()
             publisher.Time("defaultName", t =>
                 {
                     Delay();
-                    t.StatName = "otherStat";
+                    t.Bucket = "otherStat";
                 });
 
             PublisherAssertions.SingleStatNameIs(publisher, "otherStat");
@@ -182,7 +182,7 @@ public static async Task CanChangeStatNameInAsyncFunction()
             await publisher.Time("defaultName", async t =>
                 {
                     var result = await DelayedAnswerAsync();
-                    t.StatName = "afterTheAwait";
+                    t.Bucket = "afterTheAwait";
                     return result;
                 });
 

tests/JustEat.StatsD.Tests/Extensions/SimpleTimerStatNameTests.cs

@@ -28,7 +28,7 @@ public static void CanChangeStatNameDuringOperation()
             using (var timer = publisher.StartTimer("initialStat"))
             {
                 Delay();
-                timer.StatName = "changedValue";
+                timer.Bucket = "changedValue";
             }
 
             PublisherAssertions.SingleStatNameIs(publisher, "changedValue");
@@ -42,7 +42,7 @@ public static void StatNameCanBeAppended()
             using (var timer = publisher.StartTimer("Some."))
             {
                 Delay();
-                timer.StatName += "More";
+                timer.Bucket += "More";
             }
 
             PublisherAssertions.SingleStatNameIs(publisher, "Some.More");
@@ -69,7 +69,7 @@ public static void StatWithoutNameAtEndThrows()
                 using (var timer = publisher.StartTimer("valid.Stat"))
                 {
                     Delay();
-                    timer.StatName = null;
+                    timer.Bucket = null;
                 }
             });
 
@@ -88,7 +88,7 @@ public static void StatNameIsInitialValueWhenExceptionIsThrown()
                 using (var timer = publisher.StartTimer("initialStat"))
                 {
                     Fail();
-                    timer.StatName = "changedValue";
+                    timer.Bucket = "changedValue";
                 }
             }
             catch (Exception)