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
-
1\$\begingroup\$ I'd be thinking - where are the comments? \$\endgroup\$Raystafarian– Raystafarian2015年12月15日 16:53:41 +00:00Commented Dec 15, 2015 at 16:53
-
1\$\begingroup\$ The longest procedure is 17 lines of instructions. I *hope* the code should document itself. \$\endgroup\$Kaz– Kaz2015年12月15日 17:00:35 +00:00Commented 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\$πάντα ῥεῖ– πάντα ῥεῖ2015年12月15日 17:12:45 +00:00Commented Dec 15, 2015 at 17:12
1 Answer 1
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.
Calculate backwards 15 years.
Dim thisYear = Year(Now) For ixYear = (thisYear - 15) To thisYear
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.
-
\$\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\$Kaz– Kaz2015年12月17日 01:21:58 +00:00Commented 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\$RubberDuck– RubberDuck2015年12月17日 01:29:32 +00:00Commented Dec 17, 2015 at 1:29