3
\$\begingroup\$

[This is a follow-up question to: Displaying TimeSpan as largest interval (with units). The code listed here has been refactored since the original question was posed. Please note that the scope of this question is limited to the declaration and implementation of the listed method.]

The method listed below is used in a call center application to display an approximation of remaining time. For example, a call center telephone operator might inform a caller that they could perform their desired action after N time has elapsed, e.g., "Yes, that's right, you can order samples again in 2 days." Some arbitrary rules revolving around the interpretation of time are embedded within the method, e.g., an interval greater than 50 minutes is considered to be one hour; these business rules need to remain as is.

With that in mind, are there any suggestions on how this routine might be further enhanced and/or made more efficient?

public static string LargestIntervalWithUnits(TimeSpan interval)
{
    if (interval < TimeSpan.Zero)
    {
        throw new ArgumentOutOfRangeException("interval",
                                              "Negative Timespans are not supported.");
    }

    // Numeric literals defined per business rules
    const double MinimumHoursDeemedOneDay = 22.0;
    const double MinimumMinutesDeemedOneHour = 50.0;
    const double MinimumSecondsDeemedOneMinute = 40.0;
    const double MinimumMillisecondsDeemedOneSecond = 500.0;

    int timeValue;
    string timeUnits;

    if (interval.TotalHours > MinimumHoursDeemedOneDay)
    {
        timeValue = (int)Math.Ceiling(interval.TotalDays);
        timeUnits = "day";
    }
    else if (interval.TotalMinutes > MinimumMinutesDeemedOneHour)
    {
        timeValue = (int)Math.Ceiling(interval.TotalHours);
        timeUnits = "hour";
    }
    else if (interval.TotalSeconds > MinimumSecondsDeemedOneMinute)
    {
        timeValue = (int)Math.Ceiling(interval.TotalMinutes);
        timeUnits = "minute";
    }
    else if (interval.TotalMilliseconds > MinimumMillisecondsDeemedOneSecond)
    {
        timeValue = (int)Math.Ceiling(interval.TotalSeconds);
        timeUnits = "second";
    }
    else
    {
        return "now";
    }

    return string.Format("{0:#,##0} {1}{2}",
                         timeValue,
                         timeUnits,
                         (timeValue == 1 ? string.Empty : "s"));
}
\$\endgroup\$
2
\$\begingroup\$

Your code is looking good, but I find the control flow in this version a bit easier to follow:

// Defined per business rules.
private static readonly TimeSpan MinimumIntervalDeemedOneDay = TimeSpan.FromHours(22);
private static readonly TimeSpan MinimumIntervalDeemedOneHour = TimeSpan.FromMinutes(50);
private static readonly TimeSpan MinimumIntervalDeemedOneMinute = TimeSpan.FromSeconds(40);
private static readonly TimeSpan MinimumIntervalDeemedOneSecond = TimeSpan.FromMilliseconds(500);

public static string LargestIntervalWithUnits(TimeSpan interval)
{
    if (interval < TimeSpan.Zero)
    {
        throw new ArgumentOutOfRangeException("interval",
                                              "Negative Timespans are not supported.");
    }

    if (interval > MinimumIntervalDeemedOneDay)
    {
        return FormatTime(interval.TotalDays, "day");
    }

    if (interval > MinimumIntervalDeemedOneHour)
    {
        return FormatTime(interval.TotalHours, "hour");
    }

    if (interval > MinimumIntervalDeemedOneMinute)
    {
        return FormatTime(interval.TotalMinutes, "minute");
    }

    if (interval > MinimumIntervalDeemedOneSecond)
    {
        return FormatTime(interval.TotalSeconds, "second");
    }

    return "now";
}

private static string FormatTime(double value, string units)
{
    var ceiling = (int)Math.Ceiling(value);
    return string.Format("{0:#,##0} {1}{2}",
                         ceiling,
                         units,
                         (ceiling == 1 ? string.Empty : "s"));
}
| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

I don't know about you, but I prefer to read things left to right, including numeric comparisons. I like to try to make all my greater-than/less-than comparisons using the less-than operator.

3 < 5 is easier to read than 5 > 3, to me.

Additionally, your interval business rules use exclusive comparisons. I'm wondering if you want to use inclusive comparisons?

const double MinimumHoursDeemedOneDay = 22.0; yet your comparison doesn't kick in as true until .TotalHours is at least 22.01.
Probably doesn't matter, but if intent matters...

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.