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

5 Answers 5

up vote 6 down vote accepted

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
    
I like this solution the best. I didn't realize String.Format would ignore secondary variables if they were missing. This reduces the size but keeps it easily understandable. :) –  Essential Jun 7 at 12:36
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

I see you reposted. My solution from http://codereview.stackexchange.com/a/52666

public static string FormatRushTime(System.TimeSpan span, bool removeZeroes)
{
    var time = new[] 
        {
            new { Unit = "d", Value = span.Days },
            new { Unit = "h", Value = span.Hours},
            new { Unit = "m", Value = span.Minutes},
            new { Unit = "s", Value = span.Seconds}
        };

    var filtred = time.SkipWhile(t => t.Value == 0)                             // Skip until zero values 
                      .Take(2)                                                  // How many units take
                      .Where(t => ! removeZeroes || t.Value != 0)               // Show zero values?        
                      .Select(t => String.Format("{0}{1} ", t.Value, t.Unit));

    return String.Concat(filtred);
}
share|improve this answer
add comment

Another possible solution.

In this solution I'm pushing all of the formatted time segments onto a stack. Then, I pop up any leading zero entries from the stack. Once I've cleared the leading zeros I then grab the top two (if they exist) from the stack and use those checking for zero on the second one. I put the two selected time segments into a length two array that is used to build the final formatted string.

This solution avoids the multi-faceted if blocks. It could be made more readable by defining your own struct to hold the time segment value and formatted string instead of the Tuple.

private string FormatRushTime( TimeSpan dt, bool includeZeros ) {

string[] dispSegments = new[] {string.Empty, "0s"};

    // Only do the work if there's an actual time span value passed in
if (dt.TotalSeconds > 0) {

            // Push the time segments from the TimeSpan onto a stack along with their
            // related string format
    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;  // The number of segments already found

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

            // Get the first two segments from the stack
    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 ) {
timeSegments.Push( new Tuple<int, string>( timePeriod, string.Format( "0:d}{1}", timePeriod, timeDelimiter ) ) );
}
share|improve this answer
3  
Please give some explanation of what you did, it helps make this a review –  Malachi Jun 6 at 22:00
1  
Please see my edited answer. Hopefully that explains it well enough. Let me know if you have any questions. –  Mike Parkhill Jun 8 at 0:17
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.