patterncsharpMinor
Displaying TimeSpan as largest interval (with units)
Viewed 0 times
unitswithintervaldisplayinglargesttimespan
Problem
The following method is used in a call center application to display an approximation of remaining time. The call center telephone operator would inform the caller that they could perform their desired action (like, for example, place another order) after N time has elapsed. You'll note some arbitrary rules revolving around the interpretation of time embedded within the method, e.g., an interval greater than 50 minutes is considered to be "1 hour"; these business rules need to remain as is.
With that in mind, are there any suggestions on how this routine might be enhanced and/or improved (i.e., made more efficient)?
N.B. Obviously, the millisecond case (the "else") is not practical, but only included now for the sake of completeness. It may, in the end, be converted to something like 'less than 3 seconds' equals "now". For the sake of this question, however, let's go with the routine as written.
Edit: Subsequent refactored code block, that was added after answers were posted, has been removed per users' suggestions. (This note h
With that in mind, are there any suggestions on how this routine might be enhanced and/or improved (i.e., made more efficient)?
N.B. Obviously, the millisecond case (the "else") is not practical, but only included now for the sake of completeness. It may, in the end, be converted to something like 'less than 3 seconds' equals "now". For the sake of this question, however, let's go with the routine as written.
public static string LargestIntervalWithUnits(TimeSpan interval)
{
if (interval 22.0)
{
timeValue = (int)Math.Ceiling(interval.TotalDays);
timeUnits = " day";
}
else if (interval.TotalMinutes > 50.0)
{
timeValue = (int)Math.Ceiling(interval.TotalHours);
timeUnits = " hour";
}
else if (interval.TotalSeconds > 40.0)
{
timeValue = (int)Math.Ceiling(interval.TotalMinutes);
timeUnits = " minute";
}
else if (interval.TotalMilliseconds > 500.0)
{
timeValue = (int)Math.Ceiling(interval.TotalSeconds);
timeUnits = " second";
}
else
{
timeValue = (int)Math.Ceiling(interval.TotalMilliseconds);
timeUnits = " millisecond";
}
return string.Format("{0:#,##0}{1}{2}",
timeValue,
timeUnits,
(timeValue == 1 ? string.Empty : "s"));
}Edit: Subsequent refactored code block, that was added after answers were posted, has been removed per users' suggestions. (This note h
Solution
The numbers
Since they're not commonly used with time, thus are not obvious, you should make the reader aware of their meaning. This will also improve readability.
Two options for doing this include:
-
making them into constants with appropriate names
-
providing comments or full documentation on their meaning
22.0, 50.0, 40.0, and 500.0 are "magic numbers" (non-obvious hard-coded numbers with no given meaning).Since they're not commonly used with time, thus are not obvious, you should make the reader aware of their meaning. This will also improve readability.
Two options for doing this include:
-
making them into constants with appropriate names
private static readonly double someConstant = 22.0;
// remaining constants-
providing comments or full documentation on their meaning
Code Snippets
private static readonly double someConstant = 22.0;
// remaining constantsContext
StackExchange Code Review Q#53803, answer score: 8
Revisions (0)
No revisions yet.