6
\$\begingroup\$

Some fraction of a follow-up to The half-finished version.

What's changed: Added year as well as Day/Month. Added input Validation. Implemented a poor man's .EnableEvents = false for UserForms. Re-jigged the event heirarchy (Change year --> Repopulate Months or Days, Change Months --> Repopulate Days).

As always, all feedback welcomed.

In particular, if you were given this code to maintain, what would you be thinking as you read through it?

Initialisation and populating control values:

Option Explicit
Private Userform_EnableEvents As Boolean
Private Sub UserForm_Initialize()
 Userform_EnableEvents = True
 PopulateYearBox Me.UF_BankRec_cbx_Year
End Sub
Private Sub PopulateYearBox(ByRef yearBox As MSForms.ComboBox)
 DisableFormEvents
 Dim ixYear As Long
 For ixYear = 2000 To Year(Now)
 yearBox.AddItem ixYear
 Next ixYear
 EnableFormEvents
End Sub
Private Sub PopulateMonthBox(ByRef monthBox As MSForms.ComboBox, ByVal yearText As String)
 DisableFormEvents
 Dim ixYear As Long
 ixYear = CLng(yearText)
 Dim monthText As String
 monthText = monthBox.Text
 Dim ixMonth As Long, ixFinalMonth As Long
 If ixYear = Year(Now) Then
 ixFinalMonth = Month(Now)
 Else
 ixFinalMonth = 12
 End If
 monthBox.Clear
 For ixMonth = 1 To ixFinalMonth
 monthText = MonthName(ixMonth)
 monthBox.AddItem monthText
 Next ixMonth
 EnableFormEvents
End Sub
Private Sub PopulateDayBox(ByRef dayBox As MSForms.ComboBox, ByVal monthText As String, ByVal yearText As String)
 DisableFormEvents
 Dim dateCounter As Date, startDate As Date
 startDate = CDate("01/" & monthText & "/" & yearText)
 dateCounter = startDate
 dayBox.Clear
 dayBox.AddItem Day(dateCounter)
 dateCounter = dateCounter + 1
 Do While Month(dateCounter) = Month(dateCounter - 1)
 dayBox.AddItem Day(dateCounter)
 dateCounter = dateCounter + 1
 Loop
 EnableFormEvents
End Sub

Value_Change event triggers

Private Sub UF_BankRec_cbx_Year_Change()
 If Userform_EnableEvents Then
 DisableFormEvents
 Dim dayBox As MSForms.ComboBox
 Set dayBox = Me.UF_BankRec_cbx_EndDay
 Dim monthBox As MSForms.ComboBox
 Set monthBox = Me.UF_BankRec_cbx_Month
 Dim monthText As String
 monthText = monthBox.Text
 Dim yearText As String, ixYear As Long
 yearText = Me.UF_BankRec_cbx_Year.Text
 ixYear = CLng(yearText)
 If monthBox.ListCount <> 12 Or ixYear = Year(Now) Then
 PopulateMonthBox monthBox, yearText
 Else
 PopulateDayBox dayBox, monthText, yearText
 End If
 EnableFormEvents
 End If
End Sub
Private Sub UF_BankRec_cbx_Month_Change()
 If Userform_EnableEvents Then
 DisableFormEvents
 Dim dayBox As MSForms.ComboBox
 Set dayBox = Me.UF_BankRec_cbx_EndDay
 Dim monthBox As MSForms.ComboBox
 Set monthBox = Me.UF_BankRec_cbx_Month
 Dim yearBox As MSForms.ComboBox
 Set yearBox = Me.UF_BankRec_cbx_Year
 Dim monthText As String
 monthText = monthBox.Text
 Dim yearText As String
 yearText = yearBox.Text
 If yearBox.Text <> "" Then
 dayBox.Clear
 PopulateDayBox dayBox, monthText, yearText
 End If
 EnableFormEvents
 End If
End Sub

Private Sub DisableFormEvents()
 Userform_EnableEvents = False
End Sub
Private Sub EnableFormEvents()
 Userform_EnableEvents = True
End Sub

Exit Point

Private Sub UF_BankRec_btn_RetrieveData_Click()
 Dim yearBox As MSForms.ComboBox, monthBox As MSForms.ComboBox, dayBox As MSForms.ComboBox, cellSelectionBox As RefEdit.RefEdit
 Dim yearText As String, monthText As String, dayText As String
 Dim ixYear As Long, ixMonth As Long, ixDay As Long
 Dim startDate As Date, endDate As Long
 Set yearBox = Me.UF_BankRec_cbx_Year
 Set monthBox = Me.UF_BankRec_cbx_Month
 Set dayBox = Me.UF_BankRec_cbx_EndDay
 Set cellSelectionBox = Me.UF_BankRec_ref_TitleCell
 ValidateControlInputs dayBox, monthBox, yearBox, cellSelectionBox
 yearText = yearBox.Text
 monthText = monthBox.Text
 dayText = dayBox.Text
 ixYear = Year("01/01/" & yearText)
 ixMonth = Month("01/" & monthText & "/2000")
 ixDay = CLng(dayText)
 startDate = DateSerial(ixYear, ixMonth, 1)
 endDate = DateSerial(ixYear, ixMonth, ixDay)
 Dim cellAddress As String, rngTitleCell As Range
 cellAddress = cellSelectionBox.value
 Set rngTitleCell = Range(cellAddress)
 GetBankRecData 'rngTitleCell, startDate, endDate
End Sub

Data Validation

Private Sub ValidateControlInputs(ByRef dayBox As MSForms.ComboBox, ByRef monthBox As MSForms.ComboBox, ByRef yearBox As MSForms.ComboBox, ByRef cellSelectionBox As RefEdit.RefEdit)
 ValidateDayBox dayBox
 ValidateMonthBox monthBox
 ValidateYearBox yearBox
 ValidateCellSelectionBox cellSelectionBox
End Sub
Private Sub ValidateDayBox(ByRef dayBox As MSForms.ComboBox)
 Dim dayString As String
 dayString = dayBox.Text
 Dim passedValidation As Boolean
 passedValidation = False
 Dim finalDay As Long
 finalDay = (dayBox.ListCount - 1)
 Dim strErrorMessage As String
 passedValidation = dayString <= finalDay And (dayString Like "#" Or dayString Like "##")
 If Not passedValidation Then
 strErrorMessage = "The selected day is invalid. Please select a valid date."
 PrintErrorMessage strErrorMessage
 End If
End Sub
Private Sub ValidateMonthBox(ByRef monthBox As MSForms.ComboBox)
 Dim monthString As String
 monthString = monthBox.Text
 Dim passedValidation As Boolean
 passedValidation = False
 Dim strErrorMessage As String
 Dim i As Long, strMonth As String
 passedValidation = False
 For i = 1 To 12
 strMonth = MonthName(i)
 If strMonth = monthString Then passedValidation = True
 Next i
 If Not passedValidation Then
 strErrorMessage = "Please Select a valid month"
 PrintErrorMessage strErrorMessage
 End If
End Sub
Private Sub ValidateYearBox(ByRef yearBox As MSForms.ComboBox)
 Dim yearString As String
 yearString = yearBox.Text
 Dim passedValidation As Boolean
 passedValidation = False
 Dim lngYear As Long, currentYear As Long
 lngYear = CLng(yearString)
 currentYear = Year(Now)
 Dim strErrorMessage As String
 passedValidation = lngYear >= 2000 And lngYear <= currentYear
 If Not passedValidation Then
 strErrorMessage = "Please select a valid year"
 PrintErrorMessage strErrorMessage
 End If
End Sub
Private Sub ValidateCellSelectionBox(ByRef cellSelectionBox As RefEdit.RefEdit)
 Dim cellAddress As String
 cellAddress = cellSelectionBox.Text
 Dim passedValidation As Boolean
 passedValidation = False
 Dim testRange As Variant
 Set testRange = Range(cellAddress)
 Dim strErrorMessage As String
 passedValidation = TypeName(testRange) = "Range" And testRange.Count = 1
 If Not passedValidation Then
 strErrorMessage = "Please select a valid cell address"
 PrintErrorMessage strErrorMessage
 End If
End Sub
asked Dec 15, 2015 at 16:40
\$\endgroup\$
3
  • 1
    \$\begingroup\$ I'd be thinking - where are the comments? \$\endgroup\$ Commented Dec 15, 2015 at 16:53
  • 1
    \$\begingroup\$ The longest procedure is 17 lines of instructions. I *hope* the code should document itself. \$\endgroup\$ Commented Dec 15, 2015 at 17:00
  • 1
    \$\begingroup\$ I agree with @Raystafarian, there's still room to put some comments on a logical level. Also I'd put a short descriptive comment for each of the functions, and their expected inputs/outputs. The code isn't completely self descriptive. \$\endgroup\$ Commented Dec 15, 2015 at 17:12

1 Answer 1

3
\$\begingroup\$

This is going to make for either a poor UI or a change in behavior later. Better to just change its behavior now.

 Dim ixYear As Long
 For ixYear = 2000 To Year(Now)
 yearBox.AddItem ixYear
 Next ixYear

The problem is that this is always and forever going to start on the year 2000. The list will just continue to grow over time. That's fine if you need the year 2000 to always be there, but you should ask your users if that's the case. I see two options here.

  1. Calculate backwards 15 years.

    Dim thisYear = Year(Now)
    For ixYear = (thisYear - 15) To thisYear
    
  2. Make this year the first option in the list and populate the list in reverse.

    For ixYear = Year(Now) To 2000 Step -1
    

The approach you take will depend on your exact requirements. Of course, if you take the second route, you'll quickly find all the places where you hardcoded the number 2000.

The only other "issue" I see worth mentioning is this use of empty quotes.

 If yearBox.Text <> "" Then

You should use vbNullString wherever possible. It's intent is more clear and it uses less memory than the empty string literal. (Okay, so it's a negligible amount of memory, but still...)

All in all its good code, very self documenting as far as what it does, but some comments explaining why couldn't hurt. For example, why does the selection start at the year 2000?

I've got to say, I'm impressed at how far you've come since your first post here.

answered Dec 17, 2015 at 1:13
\$\endgroup\$
2
  • \$\begingroup\$ Honestly, 2000 is just a placeholder. The actual start of the data is much more recent than that. Good shout on populating in reverse order. Also, would you believe it's only been 3 months :p \$\endgroup\$ Commented Dec 17, 2015 at 1:21
  • \$\begingroup\$ That is hard to believe. Pretty soon you'll be discovering model view presenter. Then you'll start abusing the language until you just give up and move on to a language a bit newer. =;)- \$\endgroup\$ Commented Dec 17, 2015 at 1:29

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.