User Input - Date Range

Question

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

Answer 1

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.

• user79074: ...not commenting on that GoTo, you're trolling me :) +1 for the whole answer but mostly this sentence