6
\$\begingroup\$

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.

public static string LargestIntervalWithUnits(TimeSpan interval)
{
    if (interval < TimeSpan.Zero)
    {
        throw new ArgumentOutOfRangeException("interval");
    }

    if (interval == TimeSpan.Zero)
    {
        return "now";
    }

    int timeValue;
    string timeUnits;

    if (interval.TotalHours > 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 has been added to clarify comments posted by users.)

Edit 2: The code listed above has been refactored and reposted for further review: Displaying TimeSpan as largest interval (with units) [Part II]

\$\endgroup\$
2
  • \$\begingroup\$ you should post a new question with the updated code. and Jamal changed the code in his answer, you should check it out. \$\endgroup\$
    – Malachi
    Jun 9, 2014 at 18:35
  • 1
    \$\begingroup\$ What Malachi said. Adding improved code blocks, especially more than one, will complicate the review process if someone reviews the new code. You may post a new follow-up question whenever you're ready. \$\endgroup\$
    – Jamal
    Jun 9, 2014 at 18:37

3 Answers 3

8
\$\begingroup\$

The numbers 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:

  1. making them into constants with appropriate names

    private static readonly double someConstant = 22.0;
    // remaining constants
    
  2. providing comments or full documentation on their meaning

\$\endgroup\$
3
  • 2
    \$\begingroup\$ +1 for abstracting magic values with a meaningful identifier. However I'd make it private static readonly instead of const; see this SO answer (and notice emphasis on "if the value will never change") ;) \$\endgroup\$ Jun 9, 2014 at 18:08
  • \$\begingroup\$ @Mat'sMug: I must've missed this post. I think I was aware of readonly, but I also never knew that C# uses that and const. Hopefully this is correct. \$\endgroup\$
    – Jamal
    Jun 9, 2014 at 18:14
  • \$\begingroup\$ Personally I would use const here... \$\endgroup\$ Sep 14, 2015 at 19:12
6
\$\begingroup\$

Depending on how you are calling this piece of code, this if statement will almost never be evaluated as true.

if (interval == TimeSpan.Zero)
{
    return "now";
}

I don't think it is worth the time to type this out, it should be grouped into the else statement

else {
    timeValue = (int)Math.Ceiling(interval.TotalMilliseconds);
    timeUnits = " millisecond";
}

or rather

else {
    return "now";
}
\$\endgroup\$
5
\$\begingroup\$

Minor issue, but timeUnit should not hold " " + timeUnit. The space is a formatting concern, and thus should be placed in the format string used by string.Format.

Also, the exception thrown here could be a little more descriptive otherwise the caller may be confused what was wrong with the interval:

if (interval < TimeSpan.Zero)
{
    throw new ArgumentOutOfRangeException("interval", "reason why it's out of range");
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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