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 need to get the total milliseconds to the next mid-day (12:00:00) to signal a timer that will run once a day.

Assuming the application can be shut down and started anytime, is this code to get the milliseconds left to midday (used to set the timer) correct? (It doesn't need to be exact, it can be off by a few seconds)

TimeSpan now = DateTime.Now.TimeOfDay;
TimeSpan target = new DateTime(DateTime.Now.Year, DateTime.Now.Month, DateTime.Now.Day, 12, 0, 0).TimeOfDay;

double r = target.TotalMilliseconds - now.TotalMilliseconds;

if (r > 0) // It's before noon
    ;
else // It's after noon
    r = TimeSpan.FromTicks(TimeSpan.TicksPerDay).TotalMilliseconds + r;

t = new Timer(DoWork, null, r, TimeSpan.FromTicks(TimeSpan.TicksPerDay));

Can it be done more efficiently or in less lines of code?

share|improve this question
    
If it can be off by a few seconds, why are you worried about millisecond resolution? –  hometoast Apr 19 '11 at 12:57
2  
I'm not worried, it's just how I started to solve it... also, it can be off, but if I can avoid it, it's better –  jmfsg Apr 19 '11 at 12:59
add comment

6 Answers

up vote 3 down vote accepted

In terms of efficiency, you should hardly be worrying about that. But I think you could make this a little more human readable / self-explanatory and a little less complicated than it is / looks.

/// <summary>
/// Returns the period of time left before the specified hour is due to elapse
/// </summary>
/// <param name="hour">And integer representing an hour, 
/// where 0 is midnight, 12 is midday, 23 is eleven et cetera</param>
/// <returns>A TimeSpan representing the calculated time period</returns>
public static TimeSpan GetTimeUntilNextHour(int hour)
{
    var currentTime = DateTime.Now;
    var desiredTime = new DateTime(DateTime.Now.Year,
        DateTime.Now.Month, DateTime.Now.Day, hour, 0, 0);
    var timeDifference = (currentTime - desiredTime);
    var timePeriod = currentTime.Hour >= hour ?
        (desiredTime.AddDays(1) - currentTime) :
        -timeDifference;
    return timePeriod;
}

In hindsight and largely due to my attention being drawn by comments, this could be shorter, by omitting the timeDifference variable assignment and calculating in-line, further, we don't need to negate the value of either; and also we can return rather than assign timePeriod:

var currentTime = DateTime.Now;
var desiredTime = new DateTime(DateTime.Now.Year,
    DateTime.Now.Month, DateTime.Now.Day, hour, 0, 0);
return currentTime.Hour >= hour ?
    (desiredTime.AddDays(1) - currentTime) :
    desiredTime - currentTime;

A few notes here to qualify my statement...

  • Lines aren't expensive, but if you must have this on eight lines you could still do so - I've purposely dropped desiredTime to cover two lines, not strictly to stay on the safe side of history (though it helps), but just to maintain flow - where line endings are relative rather than sporadic, think of a paperback book.

  • I'd recommend naming things aptly, that way sometimes people don't necessarily need examine how the code does what it does, which allows them to either A) scan and find what they're looking or B) figure it out without having to wonder what was going through that guys head.

  • Use of implicitly typed variables (var), these are available from C# 3.0 onwards; this feature allows us to omit the specific type name completely when assigning a value / reference as part of the declaration - just be sure to use this wisely, clarity should maintained for the reader. You didn't specify which language version you're using, so this may not be available to you, but bear it in mind for the future.

  • DateTime provides operators to that allow us to work with them directly in this instance, so no need to bother with FromTicks, TotalMilliseconds, TotalTicks, TimeOfDay et cetera.

  • As a final note: an empty statement (;) produces a compiler warning, these shouldn't be ignored and, in any place I know, you'd at least have to explain yourself if someone came across this; it could also produce a compile-time error depending on the environment - that and the preceding condition are not only redundant in this case but neither are they indicative of anything, so much so that even you felt obliged to leave a comment.

share|improve this answer
    
your currentTime.Hour > hour doesn't take minutes/seconds into account, i.e. 12:55 is also after midday –  Snowbear Apr 19 '11 at 7:18
    
Oops, good spot - misinterpreted your comment at first, a simple mistake and fix, a >= check would suffice, in that case. Let me update that. –  Grant Thomas Apr 19 '11 at 7:47
    
I don’t get why you declare that extra variable timeDifference for one of the ?: cases but not the other; and why you declare it negative. Why not just var timePeriod = currentTime.Hour >= hour ? (desiredTime.AddDays(1) - currentTime) : (desiredTime - currentTime)? –  Timwi Apr 19 '11 at 15:30
    
@Timwi: Early hours of morn obviously had their way - I agree this could be shorter, by a single line. –  Grant Thomas Apr 19 '11 at 15:33
    
@Timwi: Or by two lines, if you return instead of assigning timePeriod. –  Grant Thomas Apr 19 '11 at 15:38
add comment

First, because I like a challenge (at least, one that I'm not completely lost at):

long now = DateTime.Now.Ticks;
const long noon = TimeSpan.TicksPerHour*12;
long r = ((noon - now + TimeSpan.TicksPerDay) % TimeSpan.TicksPerDay)
                   / TimeSpan.TicksPerMillisecond

No branches, minimal conversions, and no floating point. Not too much "magic", if you know why the modulo works and why the added TicksPerDay is necessary.

Second - I'll point out that this question seems like a form of premature (and wrongheaded) optimization, unless it's a purely theoretical exercise (a form of code golf?) - it's probably better to leave it in the form you understand, especially if this code only runs once (or once per day). If you're doing this a million times, then you're creating a million Timers.

share|improve this answer
add comment
if (r > 0) // It's before noon
   ;
else // It's after noon
   r = TimeSpan.FromTicks(TimeSpan.TicksPerDay).TotalMilliseconds + r; 

Change to:

if (r <= 0) // It's after noon
   r = TimeSpan.FromTicks(TimeSpan.TicksPerDay).TotalMilliseconds + r;

No need to have an if that does nothing.

share|improve this answer
    
I actually like to leave that for readability and to let the next person that reads the code know I considered the other part of the condition –  jmfsg Apr 18 '11 at 20:36
    
@Juan: If you like to indicate that, change your comment likewise. "Only update when ..." No need to indicate all other possibilities. –  Steven Jeuris Apr 18 '11 at 23:17
add comment

I would leave your code mostly as-is, at least the logic, but IMO there are some readability issues:

  • empty if statement. As already mentioned there are other ways to show that you've taken both scenarios were taken into account.
  • r variable? Something more sensible might be here. millisecondsToTarget?
  • I would also replace TimeSpan.FromTicks(TimeSpan.TicksPerDay) with TimeSpan.FromDays(1) but that's pretty subjective, I can see a point in your version also.
share|improve this answer
add comment

On a normal day there are 43,200,000 ms. from midnight until noon. How many are there from midnight until noon on the days daylight savings time starts / ends, in other words, is there a problem using the difference of two DateTimes on those days?

If you are going to use the difference of two datetimes then use the UTC version of them.

share|improve this answer
add comment
  1. I don't like the way you interleave calls to DateTime.Now with the calculation. Makes it hard to test.

    I recommend separating logic from external state, such as a clock. I prefer to write the logic in a functional style: Simple inputs that produce an output without touching anything else.

  2. Calling DateTime.Now multiple times can give different results each time in principle, but that's unlikely in practice.

  3. Assuming you use System.Threading.Timer, you don't need milliseconds at all, since the Timer takes a TimeSpan.

    Similarly those roundtrips to Ticks are useless. Just use TimeSpan.FromDays(1) or TimeSpan.FromHours(12).

So I'd create one function containing the logic:

private static TimeSpan TimeUntilMidday(DateTime current)
{
    DateTime target = current.Date.AddHours(12);// today's midday 
    if(target < current)
      target = target.AddDays(1); // tomorrow's midday
    return target - current;
}

And then use it with:

new Timer(DoWork, null, TimeUntilMidday(DateTime.Now), TimeSpan.FromDays(1))

This makes it easy to add test-cases for corner cases:

Assert(TimeUntilMidday(new DateTime(2000, 1, 1, 12, 0, 0)) == TimeSpan.Zero)

Such a test can clearly document your intent for such a special case. Without it it's not clear what you want to do if it's currently exactly midday - run instantly or wait a full day.

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.