2
\$\begingroup\$

Description

A new utility I built to ask users to input a date range (StartDate + EndDate).

Function AskDateRange() that handles loading/unloading the form and passing back input.

The userform itself, ufAskDateRange. Which validates that the user has entered text that looks like a date.


Review Aims

These days, my focus is on ease-of-maintenance, so please critique accordingly.

If you saw this code in a legacy system, how would you be feeling?


Calling the Function

    Public Sub TestAskDateRange()

        Dim startDate As Date
        Dim endDate As Date
        Dim result As Boolean

        result = AskDateRange(startDate, endDate, CDate("01/01/2017"), CDate("07/07/2018"))

    End Sub

AskDateRange()

    Public Function AskDateRange(ByRef startDate As Date, ByRef endDate As Date, Optional ByVal defaultStartDate As Date, Optional ByVal defaultEndDate As Date) As Boolean

        '/  If default dates are supplied, insert them into the relevant textboxes
        '/  Show the ufAskDateRange Userform
        '/  Form handles user input

        '/  Check for form exit conditions
        '/  Returns TRUE if the form exited properly, FALSE otherwise
        '/  If TRUE, pass back inputs.
        '/  If FALSE, pass back 0-dates

        With ufAskDateRange

            '/ Set defaults
            If defaultStartDate > 0 Then .startDate = defaultStartDate
            If defaultEndDate > 0 Then .endDate = defaultEndDate

            .Show

            '/ Check for form Exit
            AskDateRange = .ValidExit
            If .ValidExit Then
                startDate = .startDate
                endDate = .endDate
            End If

        End With

        Unload ufAskDateRange

    End Function

ufAskDateRange()

enter image description here

    Private pDateFormat As String
    Private pValidExit As Boolean

    Public Property Get ValidExit() As Boolean
        ValidExit = pValidExit
    End Property

    Public Property Get startDate() As Date
        startDate = CDate(txtStartDate.Text)
    End Property
    Public Property Let startDate(ByVal startDate As Date)
        txtStartDate.Text = startDate
    End Property

    Public Property Get endDate() As Date
        endDate = CDate(txtEndDate.Text)
    End Property
    Public Property Let endDate(ByVal endDate As Date)
        txtEndDate.Text = endDate
    End Property

    Private Sub UserForm_Initialize()

        '/ Determine system date format and update userform display
        Select Case Application.International(xlDateOrder)

            Case 0
                '/ Month-Day-Year
                pDateFormat = "MM/DD/YYYY"

            Case 1
                '/ Day-Month-Year
                pDateFormat = "DD/MM/YYYY"

            Case 2
                '/ Year-Month-Day
                pDateFormat = "YYYY/MM/DD"

        End Select

        lbStartDateFormat.Caption = pDateFormat
        lbEndDateFormat.Caption = pDateFormat

        '/ Set exit check
        pValidExit = False

    End Sub

    Private Sub UserForm_Terminate()

        '/ Clear Input Boxes
        txtStartDate.Text = ""
        txtEndDate.Text = ""

    End Sub

    Private Sub UserForm_QueryClose(Cancel As Integer, CloseMode As Integer)

        '/ Try to force exit via the submit button
        If CloseMode = 0 Then
            cmdSubmitDates_Click
            Cancel = True
        End If

    End Sub

    Private Sub cmdSubmitDates_Click()

        '/ Validate input boxes as dates. Require 10 digits to catch potential missing digits/mistyped dates
        If Not IsDate(txtStartDate.Text) Or Len(txtStartDate.Text) <> 10 Then
            MsgBox "Start Date is not in a recognised date format. Please input a new date"
            txtStartDate.SetFocus
            GoTo FailedValidation
        End If

        If Not IsDate(txtEndDate.Text) Or Len(txtEndDate.Text) <> 10 Then
            MsgBox "End Date is not in a recognised date format. Please input a new date"
            txtEndDate.SetFocus
            GoTo FailedValidation
        End If

        pValidExit = True
        Me.Hide

FailedValidation:
        Exit Sub '/ return focus to userform, having been prompted by the MsgBox

    End Sub
\$\endgroup\$
2
\$\begingroup\$

I'd use VbQueryClose.vbFormControlMenu instead of a hard-coded 0 in the QueryClose handler. Also...

Order of date elements:

0 = month-day-year
1 = day-month-year
2 = year-month-day

(MSDN)

I think these magic values deserve their own constants / enum values:

'values returned by Application.International(xlDateOrder)
Public Enum xlDateOrderFormat
    MonthDayYear = 0
    DayMonthYear = 1
    YearMonthDay = 2
End Enum

And then this:

Select Case Application.International(xlDateOrder)

    Case 0
        '/ Month-Day-Year
        pDateFormat = "MM/DD/YYYY"

    Case 1
        '/ Day-Month-Year
        pDateFormat = "DD/MM/YYYY"

    Case 2
        '/ Year-Month-Day
        pDateFormat = "YYYY/MM/DD"

End Select

Becomes:

Select Case Application.International(xlDateOrder)

    Case MonthDayYear
        pDateFormat = "MM/DD/YYYY"

    Case DayMonthYear
        pDateFormat = "DD/MM/YYYY"

    Case YearMonthDay
        pDateFormat = "YYYY/MM/DD"

End Select

Nitpick, I'd probably rename AskDateRange to PromptDateRange, and ufAskDateRange would be DateRangePromptDialog - not a fan of the Hungarian uf for UserForm here.

UX nitpick - I'm not a fan of popping MsgBox for something like input validation; it feels a bit clunky IMO. A better way would be to have Ok and Cancel buttons, and only enable Ok when the form is in a valid state, and perhaps have a label that's only visible in a non-empty and invalid state, to replace the MsgBox message. Something like this:

better UX

And now the review.


Public Function AskDateRange(ByRef startDate As Date, ByRef endDate As Date, Optional ByVal defaultStartDate As Date, Optional ByVal defaultEndDate As Date) As Boolean

It took me a good 5 seconds to realize the two ByRef parameters were the returned values, and another 2 to realize that the Boolean return value actually means that despite the input validation, you're allowing the user to "submit" the form in an invalid state... wait no, you're not - the function can only ever return True, because .ValidExit can only be True if the form returned. Not intuitive.

With the UX I'm suggesting above, the form would have some IsCancelled state instead, set to True when the form is closed through either the control box (which you're disabling) or via the Cancel button. Not allowing the user to cancel via the control box is also unintuitive UX: users are used to dismiss prompts by clicking that "X" button, and you're denying them that - a lot will be surprised by this behavior.

I'd change the signature to this:

Public Function PromptDateRange(ByRef outStartDate As Date, ByRef outEndDate As Date, Optional ByVal defaultStartDate As Date, Optional ByVal defaultEndDate As Date) As Boolean

So it could be used like:

Dim startDate As Date, endDate As Date
If PromptDateRange(startDate, endDate) Then
    'dates are valid
Else
    'dialog was cancelled, dates are irrelevant
End If

Notice how the Boolean result is now meaningful at the call site.

As for the function itself, I don't like how you're using the form's default instance - forms are classes, treat them as such!

With New DateRangePromptDialog

    If defaultStartDate <> CDate(0) Then .StartDate = defaultStartDate
    If defaultEndDate <> CDate(0) Then .EndDate = defaultEndDate

    .Show
    If Not .IsCancelled Then
        outStartDate = .StartDate
        outEndDate = .EndDate
    End If

    PromptDateRange = Not .IsCancelled

End With

With every call being made with a new instance, you no longer need to worry about state persisting between calls - thus I'd remove the Class_Terminate handler.

I would also handle the two textboxes' Change event, and validate there (to display/hide the ValidationMessageLabel and enable/disable the Ok button).

This leaves what I'd have as AcceptButton_Click() handler with no logic other than Me.Hide ...not commenting on that GoTo, you're trolling me :)


Note: the form validation logic should probably also ensure that EndDate >= StartDate, once both fields contain valid dates.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ ...not commenting on that GoTo, you're trolling me :) +1 for the whole answer but mostly this sentence \$\endgroup\$ – puzzlepiece87 Jul 14 '17 at 16:46

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.