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 have written the following code that tests if the current time is within a time window. It seems to work ok with my test cases, but I would be interested if anyone can see any possible problems or has any ideas for improvements?

Public Function IsCurrentTimeBetween(ByRef dteStartTime As DateTime, ByRef dteEndTime As DateTime) As Boolean

    Dim dteStart As DateTime, dteEnd As DateTime, dteCurrent As DateTime

    'strip off any date portion of the dates
    dteStart = Format(dteStartTime, "HH:mm:ss")
    dteEnd = Format(dteEndTime, "HH:mm:ss")
    dteCurrent = Format(DateTime.Now, "HH:mm:ss")

    If dteEnd < dteStart Then
        'the times span midnight
        If dteCurrent < dteStart And dteCurrent < dteEnd Then
            dteCurrent = dteCurrent.AddDays(1)
        End If
        dteEnd = dteEnd.AddDays(1)
    End If

    Return (dteStart <= dteCurrent AndAlso dteCurrent <= dteEnd)

End Function

Usage:

'Work out if we are between 5 to midnight and quarter past midnight    
Debug.WriteLine("Is in midnight window? " + IsCurrentTimeBetween("23:55", "00:15").ToString)
share|improve this question
    
Are you implying a date component to your times? e.g IF it is 00:01 on Jan 2 you are asking if that time is between 23:55 on Jan 1 and 00:15 on Jan 2. –  dbasnett May 23 '11 at 14:32
    
@dbasnett - Yes that is exactly what I want to find –  Matt Wilko May 23 '11 at 14:36
    
Is there some reason you can't use real dates? Also, as has been pointed out this code has a lot of implicit conversion occurring. –  dbasnett May 23 '11 at 14:55
    
@dbasnett - I have settings in the database to store the app sleep and wakeup time they are in the format "23:55" and "00:15" so I want to know if I am currently within the sleep window or not (the app stays running for days on end so I need some way of this being generic and working on any day) –  Matt Wilko May 23 '11 at 15:00
    
OK, I agree with Jeff about how this code should be done. See the code I posted below. –  dbasnett May 23 '11 at 15:12
add comment

4 Answers

up vote 4 down vote accepted

You are taking a DateTime, formatting it to a string, then assigning that to another datetime, just to get the time of day. That won't compile with Option Strict On. What you want is provided by DateTime.TimeOfDay.

I would also change ByRef to ByVal in the parameter list.

Once you are comparing timespan instead of date, you don't have to do date addition. You can just detect the wrap-around case and you only have to check endTime.

 Public Function IsCurrentTimeBetween(ByVal dteStartTime As DateTime, ByVal dteEndTime As DateTime) As Boolean

            Dim startTime, endTime, currentTime As TimeSpan

            startTime = dteStartTime.TimeOfDay
            endTime = dteEndTime.TimeOfDay
            currentTime = Now.TimeOfDay

            If endTime < startTime Then
                'the times span midnight     
                Return (currentTime <= endTime)
            Else
                'the times do not span midnight
                Return (startTime <= currentTime AndAlso currentTime <= endTime)
            End If

End Function
share|improve this answer
add comment

A couple of quick things:

You are passing in the parameters by reference unnecessarily. This method of passing is useful if you need to make modifications to variables that are observed at the callsite, but it should otherwise be avoided, as it could introduce a bug that may be more difficult to track if you make a mistake. Simply receive your parameters ByVal.

Secondly, I would encourage you to move away from hungarian notation. I've been there, I know all the arguments for keeping it. Hogwash. In your code snippet, you have variables like dteStartTime and dteStart. After you change the method parameters to be ByVal, you can then change your code to have a single variable for startTime and then another for endTime. If you give your variables meaningful names, you'll find that you never miss the old hungarian style. Indeed, I find that using hungarian often is accompanied by the rest of the variable name being an abbreviated, garbled mess.

share|improve this answer
    
The reason for passing ByRef was because I need to strip off any date portion of the datetime variable in order to leave just the time, and passing ByVal and then creating another local variable to hold the time portion doesn't really make any sense and isn't efficient. As for the notation there are arguments for and against. I work mostly in VB6 and so like to try and keep all the code consistent. –  Matt Wilko May 18 '11 at 12:14
add comment

i needed to be able to pass in a string that represents a time. . .so here's my solution (vb.net). Haven't come across a scenario this doesn't work for.

Function isBetween(ByVal timestart As String, ByVal timeEnd As String, ByVal checkDate As Date) As Boolean
    Dim dtBegin As Date = Date.Parse(Now.ToShortDateString & " " & timestart)
    Dim dtEnd As Date = Date.Parse(Now.ToShortDateString & " " & timeEnd)

    If dtBegin > dtEnd Then 'times span midnight
        dtEnd = dtEnd.AddDays(1)

        If dtBegin > checkDate And dtEnd > checkDate Then 'checkdate is after midnight, make adjustment
            dtBegin = dtBegin.AddDays(-1)
            dtEnd = dtEnd.AddDays(-1)
        End If
    End If

    If checkDate >= dtBegin AndAlso checkDate < dtEnd Then Return True

    Return False
End Function
share|improve this answer
add comment

Agreeing with Jeff P.

Private Sub Button2_Click(sender As System.Object, _
                          e As System.EventArgs) Handles Button2.Click

    'Work out if we are between 5 to midnight and quarter past midnight    
    Debug.WriteLine("Is in midnight window? " + IsCurrentTimeBetween(DateTime.Parse("23:55"), DateTime.Parse("00:15"), #12:01:00 AM#).ToString)

    Dim d As DateTime = #12:00:00 AM#
    Dim de As DateTime = #11:59:00 PM#

    Do While d < de
        Debug.WriteLine("{0:HH:mm}  {1:HH:mm}  {2}", d, d.AddMinutes(20), IsCurrentTimeBetween(d, d.AddMinutes(20), #12:01:00 AM#))
        d = d.AddMinutes(5)
    Loop
End Sub

Public Function IsCurrentTimeBetween(ByVal dteStartTime As DateTime, _
                                     ByVal dteEndTime As DateTime, _
                                     Optional curTime? As DateTime = Nothing) As Boolean
    'Jeff Paulsen's code with small change for testing
    Dim startTime, endTime, currentTime As TimeSpan

    startTime = dteStartTime.TimeOfDay
    endTime = dteEndTime.TimeOfDay
    If Not curTime.HasValue Then currentTime = DateTime.Now.TimeOfDay Else currentTime = curTime.Value.TimeOfDay

    If endTime < startTime Then
        'the times span midnight     
        Return (currentTime <= endTime)
    Else
        'the times do not span midnight
        Return (startTime <= currentTime AndAlso currentTime <= endTime)
    End If

End Function
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.