4
\$\begingroup\$

I have a switch case statement containing enum values as cases. These enum values are time units. Now I want to generate a timespan by using an enum value and a long value so like this:

2 (long) Minutes (Enum value)

1 (long) Year (Enum value)

... and so on.

This is the switch-case statement:

private const double yearValue = 365.2425;

public TimeSpan ResetTimeSpan
            {
                get
                {
                    switch (ResetTimeUnitEnumValue)
                    {
                        case TimeUnit.Seconds:
                            return new TimeSpan(0,0,0, (int)ResetTime);
                        case TimeUnit.Minutes:
                            return new TimeSpan(0, 0, (int)ResetTime, 0);
                        case TimeUnit.Hours:
                            return new TimeSpan(0, (int)ResetTime, 0);
                        case TimeUnit.Days:
                            return new TimeSpan((int)ResetTime, 0, 0, 0);
                        case TimeUnit.Months:
                            return new TimeSpan((int)(ResetTime * (yearValue / 12)), 0, 0, 0);
                        case TimeUnit.Years:
                            return new TimeSpan((int)(ResetTime * yearValue), 0, 0, 0);
                        default:
                            throw new ArgumentOutOfRangeException();
                    }
                }
            }

But I don't like it and it feels very double. Could anyone review this and refactor it, since I'm not able to see it.

\$\endgroup\$
7
\$\begingroup\$
private const double yearMultiplier = 365.2425;

private Dictionary<TimeUnit, Func<double, TimeSpan>> timespanConverters = new Dictionary<TimeUnit, Func<double, TimeSpan>>
{
    { TimeUnit.Seconds, TimeSpan.FromSeconds },
    { TimeUnit.Minutes, TimeSpan.FromMinutes },
    { TimeUnit.Hours,   TimeSpan.FromHours },
    { TimeUnit.Days,    TimeSpan.FromDays },
    { TimeUnit.Months, t => TimeSpan.FromDays((int)t * (yearMultiplier / 12))) },
    { TimeUnit.Years,  t => TimeSpan.FromDays((int)t * yearMultiplier)) }
}

// ....

public TimeSpan ResetTimeSpan
{
    get
    {
        return timespanConverters[ResetTimeUnitEnumValue](ResetTime);
    }
}

Bit of a shame that System.TimeSpan doesn't have FromMonths or FromYears methods otherwise this would look really neat :)

\$\endgroup\$
  • 2
    \$\begingroup\$ TimeSpan always represents a specific duration, e.g. x seconds. How many seconds are there in a month or year? Keep in mind that expressions like date + TimeSpan.FromMonths(x) should behave reasonably. \$\endgroup\$ – svick Jun 19 '13 at 16:39
  • \$\begingroup\$ @svick good point, months and years are not consistent so it's not possible to measure without approximating them, like in the OP's sample. \$\endgroup\$ – MattDavey Jun 19 '13 at 16:41
  • \$\begingroup\$ It does not need to be precise so its perfect this way! \$\endgroup\$ – Nick N. Jun 19 '13 at 20:40
  • \$\begingroup\$ As an easy optimisation, you could also think about memoizing the functions in the dictionary :) \$\endgroup\$ – MattDavey Jun 20 '13 at 8:17
4
\$\begingroup\$

I don't think you're going to get too much cleaner, but I'll offer up a few alternatives and thoughts.

First, you could use the TimeSpan convenience methods (TimeSpan.FromSeconds, FromMinutes, FromHours, and FromDays) to at least get up to days. FWIW, I think that's more readable than using the constructor parameters. Alternatively, using named arguments to the constructor would help readability.

You could abstract out the calculation to a map:

private const double yearValue = 365.2425;

static readonly timeUnitInSeconds = new Dictonary<TimeUnit, double>() {
   { TimeUnit.Seconds, 1 },
   { TimeUnit.Minutes, 60 },
   { TimeUnit.Hours, 60 * 60 },
   { TimeUnit.Days, 60 * 60 * 24 },
   { TimeUnit.Months, 60 * 60 * 24 * (yearValue / 12) }, 
   { TimeUnit.Years, 60 * 60 * 24 * yearValue }
};

get { 
   return TimeSpan.FromSeconds(timeUnitInSeconds[ResetTimeUnitEnumValue] * (int)ResetTime);
}

which is a little cleaner, and makes the calcs easier to read.

Or, you could go for the whole shebang, and make the TimeUnit enum itself the value in seconds:

private const double yearValue = 365.2425;

enum TimeUnit {
   // in seconds
   Seconds = 1,
   Minutes = 60,
   Hours = 60 * 60,
   Days = 60 * 60 * 24,
   Months = (int)(60 * 60 * 24 * (yearValue / 12))),
   Years = (int)(60 * 60 * 24 * yearValue)
}

get {
   return TimeSpan.FromSeconds((int)ResetTimeUnitEnumValue * (int)ResetTime);
}

That's probably bit too clever by half, though. Also note the int conversion is done first there, which could lead to slightly different results if you don't have a whole # of seconds. Your month value is a bit odd (it should be closer to 30.5 if it's intended to be the average month length in days) so you may want to double check that one.

\$\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.