Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm looking to achieve a time format that looks like this: 2d 4h remaining; or 1h 36m remaining; or 35s remaining, etc. So that it only displays the two largest values of time (this is how Clash of Clans and some other mobile games format wait times.)

I would like to format the wait time value as simply 5h or 5h 0m depending on whether I am displaying the total wait time or currently counting down. My code is below… But is there a slightly simpler way of writing it without so many returns?

(This question was originally asked here: Is there a simpler way to format this timespan? )

public string FormatRushTime ( System.TimeSpan span, boolean removeZeros )
{
    if ( span.Days > 0 )
    {
        if ( span.Hours > 0 || !removeZeros )
            return String.Format("{0:d}d {1:d}h", span.Days, span.Hours);
        else
            return String.Format("{0:d}d", span.Days);
    }
    if ( span.Hours > 0 )
    {
        if ( span.Minutes > 0 || !removeZeros )
            return String.Format("{0:h}h {1:m}m", span.Hours, span.Minutes);
        else
            return String.Format("{0:h}h", span.Hours);
    }
    if ( span.Minutes > 0 )
    {
        if ( span.Seconds > 0 || !removeZeros )
            return String.Format("{0:m}m {1:s}s", span.Minutes, span.Seconds);
        else
            return String.Format("{0:m}m", span.Minutes);
    }
    return String.Format("{0:d}s", span.Seconds);
}
share|improve this question
add comment

4 Answers

If your goal is to reduce returns, you could use a single string.Format call and not make use of additional arguments in certain cases:

// Changed removeZeroes to includeZeroes since it read kind of like a double negative.
string format = includeZeros ? "{0:d}d {1:d}h" : "{0:d}d";

// span.Hours will not get used if there's nowhere to put it in the format string:
return string.Format(format, span.Days, span.Hours);
share|improve this answer
add comment

This code is right on the tipping-point between abstracting the values to a loop, and manually unwinding the loop. To show what I mean, this is a different way of writing the function:

private static readonly string[] oneformats = {"{0:d}d", "{0:d}h", "{0:d}m", "{0:d}s"};
private static readonly string[] twoformats = {"{0:d}d {1:d}h", "{0:d}h {1:d}m", "{0:d}m {1:d}s"};

static string FormatRushTime (TimeSpan span, bool reportZero )
{
    int[] times = new int[]{span.Days, span.Hours, span.Minutes, span.Seconds};
    for (int i = 0; i < times.Length; i++)
    {
        if (times[i] != 0)
        {
            if (((i + 1) < times.Length) && (times[i+1] != 0 || reportZero))
            {
                return String.Format(twoformats[i], times[i], times[i+1]);
            }
            return String.Format(oneformats[i], times[i]);
        }
    }
    return String.Empty;
}

In essence, it looks in order through the array of times, and then makes a decision on whether to print just one, or the next sub-time as well.

In this way, your code overhead in terms of setting up the array, is more, but the code structure is a 'simple' loop, and does not have any 'magic' properties.

I have it running with some simple test data on ideone.

share|improve this answer
add comment

Another possible solution.

private string FormatRushTime( TimeSpan dt, bool includeZeros ) {
string[] dispSegments = new[] {string.Empty, "0s"};

if (dt.TotalSeconds > 0) {
    Stack<Tuple<int, string>> timeSegments = new Stack<Tuple<int, string>>();

    PushTimeSegment( timeSegments, dt.Seconds, "s" );
    PushTimeSegment( timeSegments, dt.Minutes, "m" );
    PushTimeSegment( timeSegments, dt.Hours, "h" );
    PushTimeSegment( timeSegments, dt.Days, "d" );

    int takenSegments = 0;

    // Clear out leading zeros
    while (timeSegments.Peek().Item1 == 0) {
        timeSegments.Pop();
    }

    while (timeSegments.Count > 0 && takenSegments < 2) {
        Tuple<int, string> tuple = timeSegments.Pop();
        if (tuple.Item1 > 0 || includeZeros) {
            dispSegments[takenSegments] = tuple.Item2;
            takenSegments++;
        }
    }
}
return string.Format( "{0} {1}", dispSegments[0], dispSegments[1] ).Trim();
}

private static void PushTimeSegment( Stack<Tuple<int, string>> timeSegments, int timePeriod, string timeDelimiter ) {
if (timePeriod > 0) {
    timeSegments.Push( new Tuple<int, string>( timePeriod, string.Format( "{0:d}{1}", timePeriod, timeDelimiter ) ) );
}
}
share|improve this answer
    
Please give some explanation of what you did, it helps make this a review –  Malachi 29 mins ago
add comment

I think you should use some else if statements instead of multiple if statements because you aren't going to hit the other statements if one of the previous statements is entered anyway because you are using returns in all of them.

so like this:

    if ( span.Days > 0 ) {
        if ( span.Hours > 0 || !removeZeros ) {
            return String.Format("{0:d}d {1:d}h", span.Days, span.Hours);
        } else {
            return String.Format("{0:d}d", span.Days);
        }
    } else if(span.Hours > 0) {
        if ( span.Minutes > 0 || !removeZeros ) {
            return String.Format("{0:h}h {1:m}m", span.Hours, span.Minutes);
        } else {
            return String.Format("{0:h}h", span.Hours);
        }
    } else if ( span.Minutes > 0 ) {
        if ( span.Seconds > 0 || !removeZeros ) {
            return String.Format("{0:m}m {1:s}s", span.Minutes, span.Seconds);
        } else {
            return String.Format("{0:m}m", span.Minutes);
        }
    } else {
        return String.Format("{0:d}s", span.Seconds);
    }

doesn't really make this any shorter or anything but every statement Returns a value so there is no reason to initiate a separate if block for each one, might as well be one if block, I am sure there is some benefit from the compilation of one if block versus 3 if blocks.


Don't lose the Curl(y).

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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