KyleSchoonover commented on a change in pull request #1626:
URL: https://github.com/apache/avro/pull/1626#discussion_r838127734



##########
File path: lang/csharp/src/apache/main/Util/TimeMicrosecond.cs
##########
@@ -51,16 +51,29 @@ public override object ConvertToBaseValue(object 
logicalValue, LogicalSchema sch
         {
             var time = (TimeSpan)logicalValue;
 
-            if (time >= _exclusiveUpperBound)
-                throw new ArgumentOutOfRangeException(nameof(logicalValue), "A 
'time-micros' value can only have the range '00:00:00' to '23:59:59'.");
+            ThrowIfOutOfRange(time, nameof(logicalValue));
 
+            // Note: UnixEpochDateTime.TimeOfDay is '00:00:00'. This could be 
'return time.Ticks / _ticksPerMicrosecond';
             return (time - UnixEpochDateTime.TimeOfDay).Ticks / 
_ticksPerMicrosecond;
         }
 
         /// <inheritdoc/>
         public override object ConvertToLogicalValue(object baseValue, 
LogicalSchema schema)
         {
-            return 
UnixEpochDateTime.TimeOfDay.Add(TimeSpan.FromTicks((long)baseValue * 
_ticksPerMicrosecond));
+            var time = TimeSpan.FromTicks((long)baseValue * 
_ticksPerMicrosecond);
+
+            ThrowIfOutOfRange(time, nameof(baseValue));
+
+            // Note: UnixEpochDateTime.TimeOfDay is '00:00:00', so the Add is 
meaningless. This could be 'return time;'
+            return UnixEpochDateTime.TimeOfDay.Add(time);
+        }
+
+        private static void ThrowIfOutOfRange(TimeSpan time, string paramName)
+        {
+            if (time.Ticks < 0 || time >= _exclusiveUpperBound)
+            {
+                throw new ArgumentOutOfRangeException(paramName, $"A 
'{LogicalTypeName}' value must be at least '00:00:00' and less than 
'1.00:00:00'.");

Review comment:
       `throw new ArgumentOutOfRangeException(paramName, $"A 
'{LogicalTypeName}' value must be at least '{TimeSpan.Zero}' and less than 
'{_exclusiveUpperBound}'.");`
   
   Use the constants when possible for messaging.

##########
File path: lang/csharp/src/apache/main/Util/TimeMillisecond.cs
##########
@@ -50,17 +50,29 @@ public override object ConvertToBaseValue(object 
logicalValue, LogicalSchema sch
         {
             var time = (TimeSpan)logicalValue;
 
-            if (time >= _exclusiveUpperBound)
-                throw new ArgumentOutOfRangeException(nameof(logicalValue), "A 
'time-millis' value can only have the range '00:00:00' to '23:59:59'.");
+            ThrowIfOutOfRange(time, nameof(logicalValue));
 
+            // Note: UnixEpochDateTime.TimeOfDay is '00:00:00'. This could be 
'return time.TotalMilliseconds;
             return (int)(time - UnixEpochDateTime.TimeOfDay).TotalMilliseconds;
         }
 
         /// <inheritdoc/>
         public override object ConvertToLogicalValue(object baseValue, 
LogicalSchema schema)
         {
-            var noMs = (int)baseValue;
-            return 
UnixEpochDateTime.TimeOfDay.Add(TimeSpan.FromMilliseconds(noMs));
+            var time = TimeSpan.FromMilliseconds((int)baseValue);
+
+            ThrowIfOutOfRange(time, nameof(baseValue));
+
+            // Note: UnixEpochDateTime.TimeOfDay is '00:00:00'. This could be 
'return time;'
+            return UnixEpochDateTime.TimeOfDay.Add(time);
+        }
+
+        private static void ThrowIfOutOfRange(TimeSpan time, string paramName)
+        {
+            if (time.Ticks < 0 || time >= _exclusiveUpperBound)
+            {
+                throw new ArgumentOutOfRangeException(paramName, $"A 
'{LogicalTypeName}' value must be at least '00:00:00' and less than 
'1.00:00:00'.");

Review comment:
       `throw new ArgumentOutOfRangeException(paramName, $"A 
'{LogicalTypeName}' value must be at least '{TimeSpan.Zero}' and less than 
'{_exclusiveUpperBound}'.");`
   
   Use the constants when possible for messaging.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


Reply via email to